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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
>
> From: Filip Kokosinski <fkokosinski@antmicro.com>
>
> 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@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>

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