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

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Mateusz,

On Wed, Sep 23, 2020 at 12:12 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>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/tty/serial/liteuart.c

> +static int liteuart_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct liteuart_port *uart;
> +       struct uart_port *port;
> +       struct xa_limit limit;
> +       int dev_id, ret;
> +
> +       /* no device tree */
> +       if (!np)
> +               return -ENODEV;
> +
> +       /* look for aliases; auto-enumerate for free index if not found */
> +       dev_id = of_alias_get_id(np, "serial");
> +       if (dev_id < 0)
> +               limit = XA_LIMIT(0, CONFIG_SERIAL_LITEUART_MAX_PORTS);
> +       else
> +               limit = XA_LIMIT(dev_id, dev_id);
> +
> +       uart = kzalloc(sizeof(struct liteuart_port), GFP_KERNEL);

Who frees this memory? Use devm_kzalloc()?

> +       if (!uart)
> +               return -ENOMEM;
> +
> +       ret = xa_alloc(&liteuart_array, &dev_id, uart, limit, GFP_KERNEL);

Who frees this entry?

> +       if (ret)
> +               return ret;
> +
> +       port = &uart->port;
> +
> +       /* get membase */
> +       port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +       if (!port->membase)
> +               return -ENXIO;
> +
> +       /* values not from device tree */
> +       port->dev = &pdev->dev;
> +       port->iotype = UPIO_MEM;
> +       port->flags = UPF_BOOT_AUTOCONF;
> +       port->ops = &liteuart_ops;
> +       port->regshift = 2;
> +       port->fifosize = 16;
> +       port->iobase = 1;
> +       port->type = PORT_UNKNOWN;
> +       port->line = dev_id;
> +       spin_lock_init(&port->lock);
> +
> +       return uart_add_one_port(&liteuart_driver, &uart->port);
> +}

> +static int __init liteuart_init(void)
> +{
> +       int res;
> +
> +       res = uart_register_driver(&liteuart_driver);
> +       if (res)
> +               return res;
> +
> +       res = platform_driver_register(&liteuart_platform_driver);
> +       if (res) {
> +               uart_unregister_driver(&liteuart_driver);
> +               return res;
> +       }
> +
> +       return 0;
> +}
> +
> +static void __exit liteuart_exit(void)
> +{
> +       platform_driver_unregister(&liteuart_platform_driver);
> +       uart_unregister_driver(&liteuart_driver);
> +}
> +
> +module_init(liteuart_init);
> +module_exit(liteuart_exit);

Several drivers call uart_{,un}register_driver() from their .probe()
resp. .remove() callbacks, so they can use module_platform_driver()
instead of the above boilerplate.  Greg, what's your stance on that?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds