RE: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
From: Hammer Hsieh 謝宏孟 <hidden>
Date: 2021-11-23 04:14:18
Also in:
linux-devicetree, lkml
Hi, Andy Shevchenko : I may not fully understand all your comments. But most of them I will modify when next submit. I try to answer all your comments below. And also have some question about some of your comments. Thanks Regards, Hammer
-----Original Message----- From: Andy Shevchenko <redacted> Sent: Friday, November 19, 2021 5:35 PM To: Hammer Hsieh <hammerh0314@gmail.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring [off-list ref]; open list:SERIAL DRIVERS [off-list ref]; devicetree [off-list ref]; Linux Kernel Mailing List [off-list ref]; Jiri Slaby [off-list ref]; Philipp Zabel [off-list ref]; Tony Huang 黃懷厚 [off-list ref]; Wells Lu 呂芳騰 [off-list ref]; Hammer Hsieh 謝宏孟 [off-list ref] 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 [off-list ref] wrote:quoted
Add Sunplus SoC UART DriverThanks for update, my comments below. ...quoted
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). ...quoted
include/soc/sunplus/sp_uart.h | 93 ++++Why do you need this header?
Ok, will remove header file, reg define will move to sunplus-uart.c
...quoted
+config SERIAL_SUNPLUS + bool "Sunplus UART support"No module? Why?
Will add tristate for it. I will confirm load/unload *.ko for it.
quoted
+ depends on OFNo COMPILE_TEST, why?
Will add COMPILE_TEST for it.
quoted
+ 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?
No , it is a single chip, not 8250 compatible. I plan to use 'ttySUP' for Sunplus SoC UART. We define PORT_SUNPLUS 123 at /include/uapi/linux/serial_core.h Is that ok?
...quoted
+/* + * Sunplus SoC UART driver + * + * Author: Hammer Hsieh [off-list ref] + * Tony Huang [off-list ref] + * Wells Lu [off-list ref]quoted
+ *Redundant.
Ok , will remove it.
quoted
+ */...quoted
+#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.quoted
+#include <soc/sunplus/sp_uart.h>
Ok , will do it.
...quoted
+#define UART_NR 5We have this as a config option, why do you need a separate one?
Can I keep UART_NR in sunplus-uart.c ? If all uart driver define CONFIG_xxx_UART_NR in Kconfig. Kconfig will get big very fast. In sunplus-uart.c just only one line code. In Kconfig takes 8 ~ 10 line code.
...quoted
+static struct uart_driver sunplus_uart_driver;Why global variables?
It is define for struct console { }
I will move it to close to struct console { }
...quoted
+struct sunplus_uart_port { + char name[16];uart_port has a name, what is this one for?
Ok , will remove it.
quoted
+ struct uart_port port;It's better to make it first in the structure to optimize container_of() away.quoted
+ struct clk *clk; + struct reset_control *rstc; +};
Ok, thanks for the information.
...quoted
+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)quoted
+ ? SP_UART_LSR_TX_NOT_FULL : 0);Use temporary variables for better reading. Here and everywhere else where it's applicable.quoted
+}
Ok, will modify it.
...quoted
+ writel(mcr, port->membase + SP_UART_MCR); +Redundant blank line. Check everywhere you have no such waste space.
Ok, will remove it.
...quoted
+static void sunplus_enable_ms(struct uart_port *port) { + /* Do nothing */ +}Is this stub needed at all?
No, will remove it.
...quoted
+ +One blank line is enough.
Ok, will remove it.
...quoted
+ if (port->cons == NULL)Don't we have a special API to check if the port is a console or not?
Can you show me the API ?
quoted
+ dev_err(port->dev,"UART%d,quoted
+ SP_UART_LSR_FE\n", port->line);...quoted
+#ifdef CONFIG_PM_RUNTIME_UART + if (port->line > 0) { + ret = pm_runtime_get_sync(port->dev); + if (ret < 0) + goto out; + } +#endifCan 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.c om/T/#u
I don't understand what's going on this issue?
...quoted
+ /* 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 + mightthe module unless in an idlequoted
+ * cause bus to hang. + */...quoted
+ /* + * 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))quoted
+ ;We do not allow busyloops in the kernel. Consider readl_poll_timeout() or its atomic variant.
Ok, will modify for it.
...quoted
+ 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.
Ok, will add explanation for it.
...quoted
+static void sunplus_release_port(struct uart_port *port) { } + +static int sunplus_request_port(struct uart_port *port) { + return 0; +}quoted
+static int sunplus_verify_port(struct uart_port *port, struct +serial_struct *serial) { + return -EINVAL; +}Why the stubs?
Will modify it.
Like below:
if((type!= PORT_UNKNOWN) && (type != PORT_SUNPLUS))
return -EINVAL;
return 0;
...quoted
+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.
Ok, will modify it.
quoted
+}...quoted
+ if (pdev->dev.of_node) {Redundant check
Sure, will remove it.
quoted
+ 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");quoted
+ }quoted
+Redundant blank line.
Ok, will remove it.
quoted
+ if (pdev->id < 0 || pdev->id >= UART_NR) + return -EINVAL;...quoted
+ sup = devm_kzalloc(&pdev->dev, sizeof(struct sunplus_uart_port), + GFP_KERNEL);sizeof(*sup) and make it one line.
Ok, will modify it.
quoted
+ if (!sup) + return -ENOMEM;...quoted
+ 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(...);
Ok, will modify it.
quoted
+ }...quoted
+ res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);quoted
+ if (!res_mem) + return -ENODEV; +Redundant check, besides that...
Ok, will remove it.
quoted
+ port->membase = devm_ioremap_resource(&pdev->dev,res_mem);quoted
+ if (IS_ERR(port->membase)) + return PTR_ERR(port->membase);...there is an API that does these two in one.
API of_ioremap(res, offset, sizeof(xx), "xxx") can do it. But we didn't use reg-names for it in dts. I think it is better keep orginal platform_get_resource() + devm_ioremap_resource().
...quoted
+ port->uartclk = clk_get_rate(sup->clk); + if (!port->uartclk) { + ret = -EINVAL;quoted
+ goto err_disable_clk;Instead use devm_add_action_or_reset() as many other drivers do in the kernel.quoted
+ }
Ok, will modify it.
...quoted
+ irq = platform_get_irq(pdev, 0); + if (irq < 0) + return -ENODEV;What's wrong with the error code in the irq variable?
Will change 'return -ENODEV' to 'return irq'.
...quoted
+ port->private_data = container_of(&sup->port, + struct sunplus_uart_port, port);What does this mean?
Previous code use it to store 'struct sunplus_uart_port' pointer, will remove it.
...quoted
+static const struct of_device_id sp_uart_of_match[] = { + { .compatible = "sunplus,sp7021-uart" },quoted
+ {},No comma for terminator entries.
Ok, will remove it.
quoted
+};...quoted
+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",quoted
+ .owner = THIS_MODULE,This is done by registration call, no?
Ok, will remove it.
quoted
+ .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.
Ok, thanks for the information. I will add COMPILE_TEST in Kconfig.
quoted
+#ifdef CONFIG_PM_RUNTIME_UART + .pm = sunplus_uart_pm_ops, +#endif + } +};...quoted
+ ret = platform_driver_register(&sunplus_uart_platform_driver);quoted
+ if (ret != 0) {Keep the same style over the driver.
Ok, will modify it.
quoted
+ uart_unregister_driver(&sunplus_uart_driver); + return ret; + }...quoted
+ for (;;) { + status = readl(port->membase + SP_UART_LSR); + if ((status & SP_UART_LSR_TXE) == SP_UART_LSR_TXE) + break; + cpu_relax(); + }real_poll_timeout*()
Ok, will modify it.
-- With Best Regards, Andy Shevchenko