Re: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
From: Andy Shevchenko
Date: Fri Nov 19 2021 - 04:36:02 EST
On Fri, Nov 19, 2021 at 8:14 AM Hammer Hsieh <hammerh0314@xxxxxxxxx> wrote:
>
> Add Sunplus SoC UART Driver
Thanks for update, my comments below.
...
> drivers/tty/serial/sunplus-uart.c | 903 ++++++++++++++++++++++++++++++++++++++
I believe 50 LOCs easily can be removed (see below for a few examples
I caught just by looking into this for less than 1 minute).
...
> include/soc/sunplus/sp_uart.h | 93 ++++
Why do you need this header?
...
> +config SERIAL_SUNPLUS
> + bool "Sunplus UART support"
No module? Why?
> + depends on OF
No COMPILE_TEST, why?
> + select SERIAL_CORE
> + help
> + Select this option if you would like to use Sunplus serial port on
> + Sunplus SoC SP7021.
> + If you enable this option, Sunplus serial ports in the system will
> + be registered as ttySx.
If it's ttySx, it most probably 8250 compatible, no?
...
> +/*
> + * Sunplus SoC UART driver
> + *
> + * Author: Hammer Hsieh <hammer.hsieh@xxxxxxxxxxx>
> + * Tony Huang <tony.huang@xxxxxxxxxxx>
> + * Wells Lu <wells.lu@xxxxxxxxxxx>
> + *
Redundant.
> + */
...
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/console.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/of_platform.h>
> +#include <asm/irq.h>
> +#include <linux/sysrq.h>
> +#include <linux/serial_core.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/io.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of.h>
> +#include <linux/delay.h>
Sort above alphabetically and get rid of unneeded headers.
> +#include <soc/sunplus/sp_uart.h>
...
> +#define UART_NR 5
We have this as a config option, why do you need a separate one?
...
> +static struct uart_driver sunplus_uart_driver;
Why global variables?
...
> +struct sunplus_uart_port {
> + char name[16];
uart_port has a name, what is this one for?
> + struct uart_port port;
It's better to make it first in the structure to optimize container_of() away.
> + struct clk *clk;
> + struct reset_control *rstc;
> +};
...
> +static inline u32 sp_uart_line_status_tx_buf_not_full(struct uart_port *port)
> +{
> + return ((readl(port->membase + SP_UART_LSR) & SP_UART_LSR_TX)
> + ? SP_UART_LSR_TX_NOT_FULL : 0);
Use temporary variables for better reading. Here and everywhere else
where it's applicable.
> +}
...
> + writel(mcr, port->membase + SP_UART_MCR);
> +
Redundant blank line. Check everywhere you have no such waste space.
...
> +static void sunplus_enable_ms(struct uart_port *port)
> +{
> + /* Do nothing */
> +}
Is this stub needed at all?
...
> +
> +
One blank line is enough.
...
> + if (port->cons == NULL)
Don't we have a special API to check if the port is a console or not?
> + dev_err(port->dev, "UART%d, SP_UART_LSR_FE\n", port->line);
...
> +#ifdef CONFIG_PM_RUNTIME_UART
> + if (port->line > 0) {
> + ret = pm_runtime_get_sync(port->dev);
> + if (ret < 0)
> + goto out;
> + }
> +#endif
Can we postpone implementation of it right now, please?
Can you test this [1] series instead?
[1]: https://lore.kernel.org/linux-serial/20211115084203.56478-1-tony@xxxxxxxxxxx/T/#u
...
> + /* Disable flow control of Tx, so that queued data can be sent out
> + * There is no way for s/w to let h/w abort in the middle of
> + * transaction.
> + * Don't reset module except it's in idle state. Otherwise, it might
the module unless
in an idle
> + * cause bus to hang.
> + */
...
> + /*
> + * Send all data in Tx FIFO before changing clock source,
> + * it should be UART0 only
> + */
> + while (!(readl(port->membase + SP_UART_LSR) & SP_UART_LSR_TXE))
> + ;
We do not allow busyloops in the kernel. Consider readl_poll_timeout()
or its atomic variant.
...
> + clk += baud >> 1;
> + div = clk / baud;
> + ext = div & 0x0F;
> + div = (div >> 4) - 1;
> + div_l = (div & 0xFF) | (ext << 12);
> + div_h = div >> 8;
Divisor voodoo should be explained in the comment.
...
> +static void sunplus_release_port(struct uart_port *port)
> +{
> +}
> +
> +static int sunplus_request_port(struct uart_port *port)
> +{
> + return 0;
> +}
> +static int sunplus_verify_port(struct uart_port *port, struct serial_struct *serial)
> +{
> + return -EINVAL;
> +}
Why the stubs?
...
> +static inline void wait_for_xmitr(struct uart_port *port)
> +{
> + while (1) {
> + if (sp_uart_line_status_tx_buf_not_full(port))
> + break;
> + }
read_poll_timeout() or its atomic variant.
> +}
...
> + if (pdev->dev.of_node) {
Redundant check
> + pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
> + if (pdev->id < 0)
> + pdev->id = of_alias_get_id(pdev->dev.of_node, "uart");
> + }
> +
Redundant blank line.
> + if (pdev->id < 0 || pdev->id >= UART_NR)
> + return -EINVAL;
...
> + sup = devm_kzalloc(&pdev->dev, sizeof(struct sunplus_uart_port),
> + GFP_KERNEL);
sizeof(*sup) and make it one line.
> + if (!sup)
> + return -ENOMEM;
...
> + sup->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(sup->clk)) {
> + dev_err(&pdev->dev, "unable to get UART clock\n");
> + return PTR_ERR(sup->clk);
Respect deferred probe by
return dev_err_probe(...);
> + }
...
> + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res_mem)
> + return -ENODEV;
> +
Redundant check, besides that...
> + port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
> + if (IS_ERR(port->membase))
> + return PTR_ERR(port->membase);
...there is an API that does these two in one.
...
> + port->uartclk = clk_get_rate(sup->clk);
> + if (!port->uartclk) {
> + ret = -EINVAL;
> + goto err_disable_clk;
Instead use devm_add_action_or_reset() as many other drivers do in the kernel.
> + }
...
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return -ENODEV;
What's wrong with the error code in the irq variable?
...
> + port->private_data = container_of(&sup->port,
> + struct sunplus_uart_port, port);
What does this mean?
...
> +static const struct of_device_id sp_uart_of_match[] = {
> + { .compatible = "sunplus,sp7021-uart" },
> + {},
No comma for terminator entries.
> +};
...
> +static struct platform_driver sunplus_uart_platform_driver = {
> + .probe = sunplus_uart_probe,
> + .remove = sunplus_uart_remove,
> + .suspend = sunplus_uart_suspend,
> + .resume = sunplus_uart_resume,
> + .driver = {
> + .name = "sunplus-uart",
> + .owner = THIS_MODULE,
This is done by registration call, no?
> + .of_match_table = of_match_ptr(sp_uart_of_match),
Effectively a warning (but you don't see it since COMPILE_TEST is missed).
Hint: drop of_match_ptr() completely.
> +#ifdef CONFIG_PM_RUNTIME_UART
> + .pm = sunplus_uart_pm_ops,
> +#endif
> + }
> +};
...
> + ret = platform_driver_register(&sunplus_uart_platform_driver);
> + if (ret != 0) {
Keep the same style over the driver.
> + uart_unregister_driver(&sunplus_uart_driver);
> + return ret;
> + }
...
> + for (;;) {
> + status = readl(port->membase + SP_UART_LSR);
> + if ((status & SP_UART_LSR_TXE) == SP_UART_LSR_TXE)
> + break;
> + cpu_relax();
> + }
real_poll_timeout*()
--
With Best Regards,
Andy Shevchenko