On Fri, Nov 19, 2021 at 8:14 AM Hammer Hsieh [off-list ref] 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 [off-list ref]
+ * Tony Huang [off-list ref]
+ * Wells Lu [off-list ref]

+ *

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 (local)

...

+       /* 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