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

From: Geert Uytterhoeven
Date: Fri Sep 25 2020 - 09:41:13 EST


Hi Mateusz,

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

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@xxxxxxxxxxxxxx

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