RE: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> Sent: Friday, November 19, 2021 5:35 PM
> To: Hammer Hsieh <hammerh0314@xxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; open list:SERIAL DRIVERS
> <linux-serial@xxxxxxxxxxxxxxx>; devicetree <devicetree@xxxxxxxxxxxxxxx>;
> Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; Jiri Slaby
> <jirislaby@xxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>; Tony Huang
> 黃懷厚 <tony.huang@xxxxxxxxxxx>; Wells Lu 呂芳騰
> <wells.lu@xxxxxxxxxxx>; Hammer Hsieh 謝宏孟
> <hammer.hsieh@xxxxxxxxxxx>
> 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@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?
> 
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@xxxxxxxxxxx>
> > + * Tony Huang <tony.huang@xxxxxxxxxxx>
> > + * Wells Lu <wells.lu@xxxxxxxxxxx>
> 
> > + *
> 
> 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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux