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
