Re: [PATCH 6/8] serial: Add Tegra Combined UART driver

[Andy Shevchenko]

On Sun, May 13, 2018 at 9:04 PM, Mikko Perttunen <cyndis@xxxxxxxx> wrote:
> On 05/13/2018 05:16 PM, Andy Shevchenko wrote:
>>
>> On Tue, May 8, 2018 at 2:44 PM, Mikko Perttunen <mperttunen@xxxxxxxxxx>
>> 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.

>>> +}

>>> +               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.

>>> +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.

>
>>
>>> +       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.

-- 
With Best Regards,
Andy Shevchenko