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

[Mikko Perttunen]

On 14.05.2018 01:20, Andy Shevchenko wrote:
> 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.

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