Re: [PATCH v5 5/5] drivers/tty/serial: add LiteUART driver

[Andy Shevchenko]

On Mon, May 4, 2020 at 4:44 PM Mateusz Holenko <mholenko@xxxxxxxxxxxx> wrote:
> On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@xxxxxxxxxxxx> wrote:

...

> > > Signed-off-by: Filip Kokosinski <fkokosinski@xxxxxxxxxxxx>
> > > Signed-off-by: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> >
> > Co-developed-by?
>
> Most of the coding here is done by Filip Kokosinski - I'm responsible
> for managing the patches and sending to LKML so I don't think I
> qualify as a co-developer :)

I see.

...

> > > +#define OFF_RXTX       0x00
> > > +#define SIZE_RXTX      1
> > > +#define OFF_TXFULL     0x04
> > > +#define SIZE_TXFULL    1
> > > +#define OFF_RXEMPTY    0x08
> > > +#define SIZE_RXEMPTY   1
> > > +#define OFF_EV_STATUS  0x0c
> > > +#define SIZE_EV_STATUS 1
> > > +#define OFF_EV_PENDING 0x10
> > > +#define SIZE_EV_PENDING        1
> > > +#define OFF_EV_ENABLE  0x14
> > > +#define SIZE_EV_ENABLE 1
> >
> > Why do you need all those SIZE_*?

> This is related to how LiteX peripherals (LiteUART being one of them)
> handle register access.
> The LiteX HW splits a classic 32-bit register into 4 32-bit registers,
> each one containing only 8-bit part of it.
>
> SIZE in this context means how many of those "subregisters" (still
> 32-bit wide, but with only 8-bit of meaningful data) to read/write.
> The "litex.h" header (patch 3 of this patchset) provides common
> functions for doing it, but it must know the size for each register.

So, can't you simple use them as is? I still didn't get how SIZE helps here.

...

> > > +static struct uart_driver liteuart_driver = {
> > > +       .owner = THIS_MODULE,
> > > +       .driver_name = DRIVER_NAME,
> > > +       .dev_name = DEV_NAME,
> >
> > Much easier to see if any name collisions are happen by grepping
> > similar struct definitions, but these macros are making life harder.
>
> Do you mean to avoid indirection caused by defines and write e.g.,
> `.driver_name = "liteuart"`?
>
> OK, but the reason we have defines in the first place is because we
> use the same name in many places and we want to avoid inconsistencies
> (typos, partial rename, etc.).
> What's more, looking at other serial drivers I see the notation is not
> consistent - many of them use defines for name/major/minor as well.

The problem here that .driver_name is a part of user visible
interface, so, when you rename it it will affect the module alias.
How DEV_NAME is shared? It should not be, otherwise it will collide
with other drivers.

> > > +       .major = DRIVER_MAJOR,
> > > +       .minor = DRIVER_MINOR,
> >
> > Ditto.

Ditto.

> > > +};

...

> > > +static const char *liteuart_type(struct uart_port *port)
> > > +{
> > > +       return (port->type == PORT_LITEUART) ? DRIVER_NAME : NULL;
> > > +}
> >
> > Do we need this check? Do we need a port type at all?

> This is inspired by serial_core.c and other serial drivers.
> We don't support any alternative `port->types` values so it's probably
> not necessary for us, but it seems that this is how other serial
> drivers are written too.

Legacy drivers are not the best example to take. So, if you can
survive without UART type, please go with it. Otherwise commit message
should point out why it's needed so eagerly.

...

> > > +       /* look for aliases; auto-enumerate for free index if not found */
> > > +       dev_id = of_alias_get_id(np, "serial");
> > > +       if (dev_id < 0)
> > > +               dev_id = find_first_zero_bit(liteuart_ports_in_use,
> > > +                                            CONFIG_SERIAL_LITEUART_MAX_PORTS);
> >
> > Racy.
>
> We'll protect it with a mutex to avoid race conditions.

Rather consider to use xArray API.

...

> > > +               .of_match_table = of_match_ptr(liteuart_of_match),
> >
> > of_match_ptr() makes no sense (you have depends on OF).
>
> You mean that `of_match_ptr(X)` resolves simply to `X` when
> `CONFIG_OF` is defined?
> In this context it surely can be simplified.

Yes.

...

> > > +static int __init liteuart_console_init(void)
> > > +{

> > > +}
> >
> > > +
> >
> > Extra blank line.
>
> You mean we should remove an empty line between the definition of
> liteuart_console_init() and the call to console_initcall()? It seems
> to be inconsistent across different drivers, but sure - no problem.

Less LOCs is good (but keep common sense applied).

...

> > > +/* LiteUART */
> > > +#define PORT_LITEUART  123
> >
> > We have holes in the list, use them.
> >
> > And again why we need this?
>
> This is inspired by other serial drivers that also reserves
> identifiers in this file and handles them the same way we do. We
> simply followed the convention.

See above. This ID is a part of UAPI which is kinda redundant nowadays.
You need to provide a good argument for that. Otherwise, get rid of it.

-- 
With Best Regards,
Andy Shevchenko