LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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

  reply	other threads:[~2018-05-14  7:36 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-08 11:43 [PATCH 0/8] " Mikko Perttunen
2018-05-08 11:43 ` [PATCH 1/8] dt-bindings: tegra186-hsp: Add shared interrupts Mikko Perttunen
2018-05-22 15:15   ` Jon Hunter
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-22 15:15   ` Jon Hunter
2018-05-22 20:18     ` 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 ` [PATCH 4/8] mailbox: tegra-hsp: Refactor in preparation of mailboxes Mikko Perttunen
2018-05-22 15:36   ` Jon Hunter
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-22 16:20   ` Jon Hunter
2018-05-08 11:44 ` [PATCH 6/8] serial: Add Tegra Combined UART driver Mikko Perttunen
2018-05-13 14:16   ` Andy Shevchenko
2018-05-13 18:04     ` Mikko Perttunen
2018-05-13 22:20       ` Andy Shevchenko
2018-05-14  7:36         ` Mikko Perttunen [this message]
2018-05-13 15:36   ` Jassi Brar
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-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-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 \
    --subject='Re: [PATCH 6/8] serial: Add Tegra Combined UART driver' \
    /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: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).