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

From: Andy Shevchenko
Date: Tue Apr 28 2020 - 11:50:40 EST


On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@xxxxxxxxxxxx> wrote:
>
> From: Filip Kokosinski <fkokosinski@xxxxxxxxxxxx>
>
> This commit adds driver for the FPGA-based LiteUART serial controller
> from LiteX SoC builder.
>
> The current implementation supports LiteUART configured
> for 32 bit data width and 8 bit CSR bus width.
>
> It does not support IRQ.
>
> Signed-off-by: Filip Kokosinski <fkokosinski@xxxxxxxxxxxx>
> Signed-off-by: Mateusz Holenko <mholenko@xxxxxxxxxxxx>

Co-developed-by?

...

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9731,6 +9731,7 @@ S: Maintained
> F: Documentation/devicetree/bindings/*/litex,*.yaml
> F: drivers/soc/litex/litex_soc_ctrl.c
> F: include/linux/litex.h
> +F: drivers/tty/serial/liteuart.c

Ordering issue, run latest checkpatch.pl and parse-maintaners.pl to fix.

...

> +config SERIAL_LITEUART
> + tristate "LiteUART serial port support"
> + depends on HAS_IOMEM

> + depends on OF

|| COMPILE_TEST ?

> + depends on LITEX_SOC_CONTROLLER
> + select SERIAL_CORE

...

> +/*
> + * CSRs definitions
> + * (base address offsets + width)
> + *
> + * The definitions below are true for
> + * LiteX SoC configured for
> + * 8-bit CSR Bus, 32-bit aligned.
> + *
> + * Supporting other configurations
> + * might require new definitions
> + * or a more generic way of indexing
> + * the LiteX CSRs.
> + *
> + * For more details on how CSRs
> + * are defined and handled in LiteX,
> + * see comments in the LiteX SoC Driver:
> + * drivers/soc/litex/litex_soc_ctrl.c
> + */

Can you use some like 76 characters per line?

...

> +#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_*?

...

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

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

Ditto.

> + .nr = CONFIG_SERIAL_LITEUART_MAX_PORTS,

> +#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
> + .cons = &liteuart_console,
> +#endif

> +};

...

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

...

> +static int liteuart_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct liteuart_port *uart;
> + struct uart_port *port;
> + int dev_id;
> +
> + if (!litex_check_accessors())
> + return -EPROBE_DEFER;
> +

> + /* no device tree */
> + if (!np)
> + return -ENODEV;

I guess it should go first, otherwise potentially you may end up with
deferred module above.

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

> + /* get {map,mem}base */
> + port->mapbase = platform_get_resource(pdev, IORESOURCE_MEM, 0)->start;
> + port->membase = of_iomap(np, 0);

Can't you use devm_platform_get_and_ioremap_resource() ?

> + if (!port->membase)
> + return -ENXIO;

> +}

...

> +static struct platform_driver liteuart_platform_driver = {
> + .probe = liteuart_probe,
> + .remove = liteuart_remove,
> + .driver = {
> + .name = DRIVER_NAME,

> + .of_match_table = of_match_ptr(liteuart_of_match),

of_match_ptr() makes no sense (you have depends on OF).

> + },
> +};

...


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

Missed spin lock initialization.

> + register_console(&liteuart_console);
> +
> + return 0;
> +}

> +

Extra blank line.

> +console_initcall(liteuart_console_init);

...

> +/* LiteUART */
> +#define PORT_LITEUART 123

We have holes in the list, use them.

And again why we need this?

--
With Best Regards,
Andy Shevchenko