Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 19 Nov 2021 11:35:07 +0200 | Subject | Re: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver |
| |
On Fri, Nov 19, 2021 at 8:14 AM Hammer Hsieh <hammerh0314@gmail.com> 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@sunplus.com> > + * Tony Huang <tony.huang@sunplus.com> > + * Wells Lu <wells.lu@sunplus.com>
> + *
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@atomide.com/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
| |