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

On Mon, May 4, 2020 at 4:44 PM Mateusz Holenko [off-list ref] wrote:

On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
[off-list ref] wrote:

quoted

On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko [off-list ref] wrote:

...

quoted

quoted

Signed-off-by: Filip Kokosinski <redacted>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>

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.

...

quoted

quoted

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

...

quoted

quoted

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

quoted

quoted

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

Ditto.

Ditto.

quoted

quoted

+};

...

quoted

quoted

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

...

quoted

quoted

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

...

quoted

quoted

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

...

quoted

quoted

+static int __init liteuart_console_init(void)
+{

quoted

quoted

+}

quoted

+

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

...

quoted

quoted

+/* 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