From: Mikko Perttunen <cyndis@kapsi.fi> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Mikko Perttunen <mperttunen@nvidia.com>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Jassi Brar <jassisinghbrar@gmail.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Thierry Reding <thierry.reding@gmail.com>, Jon Hunter <jonathanh@nvidia.com>, araza@nvidia.com, devicetree <devicetree@vger.kernel.org>, "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>, linux-tegra@vger.kernel.org, linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 6/8] serial: Add Tegra Combined UART driver Date: Mon, 14 May 2018 10:36:35 +0300 [thread overview] Message-ID: <513a6287-9bb3-1441-b3ea-6866f2e74c99@kapsi.fi> (raw) In-Reply-To: <CAHp75VeJcqaiWUVr=oXEH5YurY3yCDdE1eeoWumo6jCa_4PWEQ@mail.gmail.com> On 14.05.2018 01:20, Andy Shevchenko wrote: > On Sun, May 13, 2018 at 9:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >> On 05/13/2018 05:16 PM, Andy Shevchenko wrote: >>> >>> On Tue, May 8, 2018 at 2:44 PM, Mikko Perttunen <mperttunen@nvidia.com> >>> wrote: >>>> >>>> The Tegra Combined UART (TCU) is a mailbox-based mechanism that allows >>>> multiplexing multiple "virtual UARTs" into a single hardware serial >>>> port. The TCU is the primary serial port on Tegra194 devices. >>>> >>>> Add a TCU driver utilizing the mailbox framework, as the used mailboxes >>>> are part of Tegra HSP blocks that are already controlled by the Tegra >>>> HSP mailbox driver. > >>>> +static void tegra_tcu_uart_set_mctrl(struct uart_port *port, unsigned >>>> int mctrl) >>>> +{ >>> >>> >>>> + (void)port; >>>> + (void)mctrl; >>> >>> >>> Huh? >> >> >> The serial core calls these callbacks without checking if they are set. They >> don't make sense for this driver so they are stubbed out. > > My question why do you need these ugly lines? I'm pretty sure no other > driver with stubs using such style. It's my personal style, being explicit about unused variables in this way - I don't consider them ugly. But I can certainly remove them for the next version. > >>>> +} > >>>> + if (written == 3) { >>>> + value |= 3 << 24; >>>> + value |= BIT(26); >>>> + mbox_send_message(tcu->tx, &value); >>> >>> >>>> + } >>> >>> >>> (1) >>> >>>> + } >>>> + >>>> + if (written) { >>>> + value |= written << 24; >>>> + value |= BIT(26); >>>> + mbox_send_message(tcu->tx, &value); >>>> + } >>> >>> >>> (2) >>> >>> These are code duplications. >> >> >> Indeed - the length of the duplicated code is so short, and the instances >> are so close to each other, that I don't find it necessary (or clearer) to >> have an extra function. > > It makes sense. Consider to refactor other way w/o duplication then. I'll see if I can refactor it out. > >>>> +static void tegra_tcu_uart_set_termios(struct uart_port *port, >>>> + struct ktermios *new, >>>> + struct ktermios *old) >>>> +{ >>>> + (void)port; >>>> + (void)new; >>>> + (void)old; >>>> +} >>> >>> >>> Remove those unused stub contents. >> >> >> Sure. I had these here so that we don't get unused parameter warnings, but I >> can just as well remove the parameter names. > > What warnings? How did you get them? We have them disabled as far as I > know even with W=1. May be - it's just a habit, maybe from other projects where the warning is enabled. > >> >>> >>>> + return uart_set_options(&tegra_tcu_uart_port, cons, >>>> + 115200, 'n', 8, 'n'); >>> >>> >>> Can't it be one line? >> >> >> It would be a total of 81 characters in length on one line, so no. > > So, yes. 1 character doesn't prevent us make the readability better. > Please, put to one line. > Ok, I'll change this. Thanks, Mikko
WARNING: multiple messages have this Message-ID (diff)
From: cyndis@kapsi.fi (Mikko Perttunen) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 6/8] serial: Add Tegra Combined UART driver Date: Mon, 14 May 2018 10:36:35 +0300 [thread overview] Message-ID: <513a6287-9bb3-1441-b3ea-6866f2e74c99@kapsi.fi> (raw) In-Reply-To: <CAHp75VeJcqaiWUVr=oXEH5YurY3yCDdE1eeoWumo6jCa_4PWEQ@mail.gmail.com> On 14.05.2018 01:20, Andy Shevchenko wrote: > On Sun, May 13, 2018 at 9:04 PM, Mikko Perttunen <cyndis@kapsi.fi> wrote: >> On 05/13/2018 05:16 PM, Andy Shevchenko wrote: >>> >>> On Tue, May 8, 2018 at 2:44 PM, Mikko Perttunen <mperttunen@nvidia.com> >>> wrote: >>>> >>>> The Tegra Combined UART (TCU) is a mailbox-based mechanism that allows >>>> multiplexing multiple "virtual UARTs" into a single hardware serial >>>> port. The TCU is the primary serial port on Tegra194 devices. >>>> >>>> Add a TCU driver utilizing the mailbox framework, as the used mailboxes >>>> are part of Tegra HSP blocks that are already controlled by the Tegra >>>> HSP mailbox driver. > >>>> +static void tegra_tcu_uart_set_mctrl(struct uart_port *port, unsigned >>>> int mctrl) >>>> +{ >>> >>> >>>> + (void)port; >>>> + (void)mctrl; >>> >>> >>> Huh? >> >> >> The serial core calls these callbacks without checking if they are set. They >> don't make sense for this driver so they are stubbed out. > > My question why do you need these ugly lines? I'm pretty sure no other > driver with stubs using such style. It's my personal style, being explicit about unused variables in this way - I don't consider them ugly. But I can certainly remove them for the next version. > >>>> +} > >>>> + if (written == 3) { >>>> + value |= 3 << 24; >>>> + value |= BIT(26); >>>> + mbox_send_message(tcu->tx, &value); >>> >>> >>>> + } >>> >>> >>> (1) >>> >>>> + } >>>> + >>>> + if (written) { >>>> + value |= written << 24; >>>> + value |= BIT(26); >>>> + mbox_send_message(tcu->tx, &value); >>>> + } >>> >>> >>> (2) >>> >>> These are code duplications. >> >> >> Indeed - the length of the duplicated code is so short, and the instances >> are so close to each other, that I don't find it necessary (or clearer) to >> have an extra function. > > It makes sense. Consider to refactor other way w/o duplication then. I'll see if I can refactor it out. > >>>> +static void tegra_tcu_uart_set_termios(struct uart_port *port, >>>> + struct ktermios *new, >>>> + struct ktermios *old) >>>> +{ >>>> + (void)port; >>>> + (void)new; >>>> + (void)old; >>>> +} >>> >>> >>> Remove those unused stub contents. >> >> >> Sure. I had these here so that we don't get unused parameter warnings, but I >> can just as well remove the parameter names. > > What warnings? How did you get them? We have them disabled as far as I > know even with W=1. May be - it's just a habit, maybe from other projects where the warning is enabled. > >> >>> >>>> + return uart_set_options(&tegra_tcu_uart_port, cons, >>>> + 115200, 'n', 8, 'n'); >>> >>> >>> Can't it be one line? >> >> >> It would be a total of 81 characters in length on one line, so no. > > So, yes. 1 character doesn't prevent us make the readability better. > Please, put to one line. > Ok, I'll change this. Thanks, Mikko
next prev parent reply other threads:[~2018-05-14 7:36 UTC|newest] Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-08 11:43 [PATCH 0/8] Tegra Combined UART driver Mikko Perttunen 2018-05-08 11:43 ` Mikko Perttunen 2018-05-08 11:43 ` Mikko Perttunen 2018-05-08 11:43 ` [PATCH 1/8] dt-bindings: tegra186-hsp: Add shared interrupts Mikko Perttunen 2018-05-08 11:43 ` Mikko Perttunen 2018-05-08 11:43 ` Mikko Perttunen 2018-05-22 15:15 ` Jon Hunter 2018-05-22 15:15 ` Jon Hunter 2018-05-22 15:15 ` Jon Hunter 2018-06-19 12:41 ` Mikko Perttunen 2018-06-19 12:41 ` Mikko Perttunen 2018-05-08 11:43 ` [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia,tegra194-tcu Mikko Perttunen 2018-05-08 11:43 ` [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia, tegra194-tcu Mikko Perttunen 2018-05-08 11:43 ` [PATCH 2/8] dt-bindings: serial: Add bindings for nvidia,tegra194-tcu Mikko Perttunen 2018-05-22 15:15 ` Jon Hunter 2018-05-22 15:15 ` Jon Hunter 2018-05-22 15:15 ` Jon Hunter 2018-05-22 20:18 ` Rob Herring 2018-05-22 20:18 ` Rob Herring 2018-05-22 20:20 ` Rob Herring 2018-05-22 20:20 ` Rob Herring 2018-05-08 11:43 ` [PATCH 3/8] mailbox: Add transmit done by blocking option Mikko Perttunen 2018-05-08 11:43 ` Mikko Perttunen 2018-05-08 11:43 ` Mikko Perttunen 2018-05-08 11:43 ` [PATCH 4/8] mailbox: tegra-hsp: Refactor in preparation of mailboxes Mikko Perttunen 2018-05-08 11:43 ` Mikko Perttunen 2018-05-08 11:43 ` Mikko Perttunen 2018-05-22 15:36 ` Jon Hunter 2018-05-22 15:36 ` Jon Hunter 2018-05-22 15:36 ` Jon Hunter 2018-06-19 12:52 ` Mikko Perttunen 2018-06-19 12:52 ` Mikko Perttunen 2018-05-08 11:44 ` [PATCH 5/8] mailbox: tegra-hsp: Add support for shared mailboxes Mikko Perttunen 2018-05-08 11:44 ` Mikko Perttunen 2018-05-08 11:44 ` Mikko Perttunen 2018-05-22 16:20 ` Jon Hunter 2018-05-22 16:20 ` Jon Hunter 2018-05-22 16:20 ` Jon Hunter 2018-05-08 11:44 ` [PATCH 6/8] serial: Add Tegra Combined UART driver Mikko Perttunen 2018-05-08 11:44 ` Mikko Perttunen 2018-05-08 11:44 ` Mikko Perttunen 2018-05-13 14:16 ` Andy Shevchenko 2018-05-13 14:16 ` Andy Shevchenko 2018-05-13 18:04 ` Mikko Perttunen 2018-05-13 18:04 ` Mikko Perttunen 2018-05-13 22:20 ` Andy Shevchenko 2018-05-13 22:20 ` Andy Shevchenko 2018-05-14 7:36 ` Mikko Perttunen [this message] 2018-05-14 7:36 ` Mikko Perttunen 2018-05-13 15:36 ` Jassi Brar 2018-05-13 15:36 ` Jassi Brar 2018-05-13 18:06 ` Mikko Perttunen 2018-05-13 18:06 ` Mikko Perttunen 2018-05-08 11:44 ` [PATCH 7/8] arm64: tegra: Add nodes for tcu on Tegra194 Mikko Perttunen 2018-05-08 11:44 ` Mikko Perttunen 2018-05-08 11:44 ` Mikko Perttunen 2018-05-22 16:28 ` Jon Hunter 2018-05-22 16:28 ` Jon Hunter 2018-05-22 16:28 ` Jon Hunter 2018-05-08 11:44 ` [PATCH 8/8] arm64: tegra: Mark tcu as primary serial port on Tegra194 P2888 Mikko Perttunen 2018-05-08 11:44 ` Mikko Perttunen 2018-05-08 11:44 ` Mikko Perttunen 2018-05-22 16:29 ` Jon Hunter 2018-05-22 16:29 ` Jon Hunter 2018-05-22 16:29 ` Jon Hunter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=513a6287-9bb3-1441-b3ea-6866f2e74c99@kapsi.fi \ --to=cyndis@kapsi.fi \ --cc=andy.shevchenko@gmail.com \ --cc=araza@nvidia.com \ --cc=devicetree@vger.kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=jassisinghbrar@gmail.com \ --cc=jonathanh@nvidia.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-serial@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mperttunen@nvidia.com \ --cc=robh+dt@kernel.org \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.