Messages in this thread | | | From | Hammer Hsieh 謝宏孟 <> | Subject | RE: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver | Date | Tue, 23 Nov 2021 03:43:24 +0000 |
| |
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 <andy.shevchenko@gmail.com> > Sent: Friday, November 19, 2021 5:35 PM > To: Hammer Hsieh <hammerh0314@gmail.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > <robh+dt@kernel.org>; open list:SERIAL DRIVERS > <linux-serial@vger.kernel.org>; devicetree <devicetree@vger.kernel.org>; > Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Jiri Slaby > <jirislaby@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; Tony Huang > 黃懷厚 <tony.huang@sunplus.com>; Wells Lu 呂芳騰 > <wells.lu@sunplus.com>; Hammer Hsieh 謝宏孟 > <hammer.hsieh@sunplus.com> > 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? > Ok, will remove header file, reg define will move to sunplus-uart.c
> ... > > > +config SERIAL_SUNPLUS > > + bool "Sunplus UART support" > > No module? Why?
Will add tristate for it. I will confirm load/unload *.ko for it.
> > > + depends on OF > > No COMPILE_TEST, why?
Will add COMPILE_TEST for it.
> > > + 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?
> ... > > > +/* > > + * Sunplus SoC UART driver > > + * > > + * Author: Hammer Hsieh <hammer.hsieh@sunplus.com> > > + * Tony Huang <tony.huang@sunplus.com> > > + * Wells Lu <wells.lu@sunplus.com> > > > + * > > Redundant. >
Ok , will remove it.
> > + */ > > ... > > > +#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> >
Ok , will do it.
> ... > > > +#define UART_NR 5 > > We 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.
> ... > > > +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 { }
> ... > > +struct sunplus_uart_port { > > + char name[16]; > > uart_port has a name, what is this one for? >
Ok , will remove it.
> > + 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; > > +}; >
Ok, thanks for the information.
> ... > > > +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. > > > +} >
Ok, will modify it.
> ... > > > + writel(mcr, port->membase + SP_UART_MCR); > > + > > Redundant blank line. Check everywhere you have no such waste space. >
Ok, will remove it.
> ... > > > +static void sunplus_enable_ms(struct uart_port *port) { > > + /* Do nothing */ > > +} > > Is this stub needed at all? >
No, will remove it.
> ... > > > + > > + > > One blank line is enough. >
Ok, will remove it.
> ... > > > + 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 ?
> > + 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.c > om/T/#u >
I don't understand what's going on this issue?
> ... > > > + /* 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. >
Ok, will modify for it.
> ... > > > + 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.
> ... > > > +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? >
Will modify it. Like below: if((type!= PORT_UNKNOWN) && (type != PORT_SUNPLUS)) return -EINVAL; return 0;
> ... > > > +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.
> > +} > > ... > > > + if (pdev->dev.of_node) { > > Redundant check
Sure, will remove it.
> > > + 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.
Ok, will remove it.
> > > + 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. >
Ok, will modify it.
> > + 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(...); >
Ok, will modify it.
> > + } > > ... > > > + res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + if (!res_mem) > > + return -ENODEV; > > + > > Redundant check, besides that...
Ok, will remove it.
> > > + 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. >
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().
> ... > > > + 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. > > > + } >
Ok, will modify it.
> ... > > > + 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'.
> ... > > > + 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.
> ... > > > +static const struct of_device_id sp_uart_of_match[] = { > > + { .compatible = "sunplus,sp7021-uart" }, > > > + {}, > > No comma for terminator entries. >
Ok, will remove it.
> > +}; > > ... > > > +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? >
Ok, will remove it.
> > + .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.
> > +#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. >
Ok, will modify it.
> > + 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*() >
Ok, will modify it.
> -- > With Best Regards, > Andy Shevchenko
| |