RE: [PATCH v4 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 :

Thanks for your review.
Please see my response in below mail.

Regards,
Hammer Hsieh

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Friday, December 3, 2021 4:03 AM
> 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 v4 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
> 
> On Thu, Dec 2, 2021 at 9:35 PM Hammer Hsieh <hammerh0314@xxxxxxxxx>
> wrote:
> >
> > Add Sunplus SoC UART Driver
> 
> ...
> 
> > +config SERIAL_SUNPLUS
> > +       tristate "Sunplus UART support"
> > +       depends on OF || COMPILE_TEST
> > +       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 ttySUPx.
> 
> What will be the module name in case of module build?
> 

will add info as below shows in Kconfig:
"This driver can also be built as a module. If so, the module will be called sunplus-uart."

> ...
> 
> > +/*
> > + * Sunplus SoC UART driver
> > + *
> > + * Author: Hammer Hsieh <hammer.hsieh@xxxxxxxxxxx>
> 
> Authors:
> 
> > + * Tony Huang <tony.huang@xxxxxxxxxxx>
> > + * Wells Lu <wells.lu@xxxxxxxxxxx>
> 
> And please indent names to be on the same column.
> 

I rewrite almost all uart driver.
The other authors ask me to remove their names on this driver.
Will modify it.

> > + */
> 
> ...
> 
> > +#define UART_AUTOSUSPEND_TIMEOUT       3000
> 
> Add units to the name.
> 

Will add it. /* units: ms */

> ...
> 
> > +static inline u32 sunplus_tx_buf_not_full(struct uart_port *port) {
> > +       unsigned int lsr = readl(port->membase + SUP_UART_LSR);
> > +
> > +       return ((lsr & SUP_UART_LSR_TX) ? SUP_UART_LSR_TX_NOT_FULL :
> > + 0);
> 
> Too many parentheses. Ditto for all similar cases.
> 
> > +}
> 

ok, I have found similar case. I will modify it.

> ...
> 
> > +       do {
> > +               sp_uart_put_char(port, xmit->buf[xmit->tail]);
> > +               xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> 
> "% UART_XMIT_SIZE" is more accurate since it doesn't require a value to be a
> power of 2. In case of power of 2 it will be properly optimized by a compiler.
> 

Currently, I have found drivers/tty/serial/mps2-uart.c use it only.
ok, will modify it.

> > +               port->icount.tx++;
> > +
> > +               if (uart_circ_empty(xmit))
> > +                       break;
> > +       } while (sunplus_tx_buf_not_full(port));
> 
> ...
> 
> > +       spin_lock_irqsave(&port->lock, flags);
> 
> Why irqsave here?
> 
> ...
> 
> > +       if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_RX)
> > +               receive_chars(port);
> > +
> > +       if (readl(port->membase + SUP_UART_ISC) & SUP_UART_ISC_TX)
> > +               transmit_chars(port);
> 
> Do you really need to perform two I/O against the very same register?
> 

I will rewrite as below, how do you think?
Static irqreturn_t sunplus_uart_irq(int irq, void *args)
{
 Struct uart_port *port = (struct uart_port *)args;
 unsigned int isc = readl(port->membase + SUP_UART_ISC);
 if (isc & SUP_UART_ISC_RX)
   receive_chars(port);
 if (isc & SUP_UART_ISC_TX)
   transmit_chars(port);
 return IRQ_HANDLED;
}

> ...
> 
> > +static int sunplus_startup(struct uart_port *port) {
> > +       unsigned int isc;
> > +       int ret;
> 
> > +#ifdef CONFIG_PM
> 
> Why is this ifdeffery around the driver?
> 
> > +       if (!uart_console(port)) {
> > +               ret = pm_runtime_get_sync(port->dev);
> > +               if (ret < 0)
> > +                       goto out;
> > +       }
> > +#endif
> > +       ret = request_irq(port->irq, sunplus_uart_irq, 0, "sunplus_uart",
> port);
> > +       if (ret)
> > +               return ret;
> > +
> > +       spin_lock_irq(&port->lock);
> > +
> > +       isc |= SUP_UART_ISC_RXM;
> > +       writel(isc, port->membase + SUP_UART_ISC);
> > +
> > +       spin_unlock_irq(&port->lock);
> 
> > +#ifdef CONFIG_PM
> > +       if (!uart_console(port))
> > +               pm_runtime_put(port->dev);
> 
> Why doesn't it set autosuspend, i.o.w. Why is it different from an error case?
> 

Autosuspend already init at probe.
I remove #ifdef CONFIG_PM code in sunplus_startup() and test runtime function.
linux-serial-test -y 0x55 -z 0x30 -p /dev/ttySUP1 -b 115200
runtime_resume and runtime_suspend still work.
I will remove it.

> > +       return 0;
> > +out:
> > +       if (!uart_console(port)) {
> > +               pm_runtime_mark_last_busy(port->dev);
> > +               pm_runtime_put_autosuspend(port->dev);
> > +       }
> > +#endif
> > +       return 0;
> > +}
> 
> ...
> 
> > +static void sunplus_set_termios(struct uart_port *port,
> > +                               struct ktermios *termios,
> > +                               struct ktermios *oldtermios) {
> > +       u32 clk, ext, div, div_l, div_h, baud;
> > +       u32 lcr, val;
> > +       unsigned long flags;
> 
> > +       clk = port->uartclk;
> 
> This can be done in the definition block above.
> 

I think you want the code like below, right ?
u32 ext, div, div_l, div_h, baud, lcr;
u32 clk = port->uartclk;
unsigned long flags;

> > +       baud = uart_get_baud_rate(port, termios, oldtermios, 0,
> > + port->uartclk / 16);
> > +
> > +       readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
> > +                                 (val & SUP_UART_LSR_TXE), 1,
> 10000);
> 
> No error check?
> 

remove this code in set_termios( ).
I think it is not necessary to check tx empty here.

> > +       /*
> > +        * baud rate = uartclk / ((16 * div[15:0] + 1) + div_ext[3:0])
> > +        * get target baud rate and uartclk
> > +        * auto calculate div and div_ext
> > +        * div_h = (div[15:8] >> 8); div_l = (div_ext[3:0] << 12) +
> > + div[7:0]
> 
> There is no need to explain the code, please add excerpts from the data sheet
> on how the divisors and baud rate are calculated, use mathematical language,
> and not programming in the comment.
> 

Ok, will modify it. Which one is better?
/* baud rate = uartclk / ((16 * divisor + 1) + divisor_ext) */
Or 
/*                   uartclk
 * baud rate = -------------------------------------------
 *           (16 * divisor + 1) + divisor_ext
 */

> > +        */
> > +       clk += baud >> 1;
> > +       div = clk / baud;
> > +       ext = div & 0x0F;
> > +       div = (div >> 4) - 1;
> > +       div_l = (div & 0xFF) | (ext << 12);
> > +       div_h = div >> 8;
> 
> ...
> 
> > +static const char *sunplus_type(struct uart_port *port) {
> > +       struct sunplus_uart_port *sup = to_sunplus_uart(port);
> > +
> > +       return sup->port.type == PORT_SUNPLUS ? "sunplus_uart" : NULL;
> > +}
> 
> 
> What do we achieve with this? Who and how will see this information?
> Under which circumstances the port type is not SUNPLUS?
> 
> ...
> 
> > +static void sunplus_release_port(struct uart_port *port) { }
> > +
> > +static int sunplus_request_port(struct uart_port *port) {
> > +       return 0;
> > +}
> 
> Are these stubs mandatory?
> 
> ...
> 
> > +static void sunplus_config_port(struct uart_port *port, int type) {
> 
> > +       if (type & UART_CONFIG_TYPE) {
> > +               port->type = PORT_SUNPLUS;
> > +               sunplus_request_port(port);
> > +       }
> 
> if (!(type & ...))
>   return;
> 
> ?
> 

About these functions
sunplus_type / sunplus_release_port / sunplus_request_port / sunplus_config_port

Almost all uart driver have these function, but actually I don't know when/how to use it.
I will study it.
If you have more information, please share it to me.

> > +}
> 
> ...
> 
> > +static int sunplus_poll_init(struct uart_port *port) {
> > +       return 0;
> > +}
> 
> Why is this stub needed?
> 

Will remove it.

> ...
> 
> > +static inline void wait_for_xmitr(struct uart_port *port) {
> > +       unsigned int val;
> > +
> > +       readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
> > +                                 (val & SUP_UART_LSR_TX), 1,
> 10000);
> 
> Error handling?
> Or explain why it's not needed.
> 
> > +}
> 

I refer to /drivers/tty/serial/owl-uart.c
Will add error handling for it as below , is that OK?

static inline void wait_for_xmitr(struct uart_port *port) {
  unsigned int val;
  int ret;
  /*Wait for FIFO is full or timeout */
  ret = readl_poll_timeout_atomic(port->membase + SUP_UART_LSR, val,
                      (val & SUP_UART_LSR_TX), 1, 10000);
  If (ret == -ETIMEOUT) {
    dev_err(port->dev, "Timeout waiting while UART TX FULL);
    return;
   }
}

> ...
> 
> > +       sup->clk = devm_clk_get(&pdev->dev, NULL);
> > +       if (!IS_ERR(sup->clk)) {
> 
> Instead use devm_clk_get_optional().
> 
> > +               ret = clk_prepare_enable(sup->clk);
> > +               if (ret)
> > +                       return ret;
> 
> > +               ret = devm_add_action_or_reset(&pdev->dev,
> > +                                              (void(*)(void
> *))clk_disable_unprepare,
> > +                                              sup->clk);
> 
> Look at the examples of how other drivers do this (that were submitted more
> or less recently).
> 
> > +               if (ret)
> > +                       return ret;
> > +       } else {
> 
> > +               if (PTR_ERR(sup->clk) == -EPROBE_DEFER)
> > +                       return -EPROBE_DEFER;
> 
> Why?!
> 
> > +               return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk),
> "clk not found\n");
> > +       }
> 
> ...

In case of without deferred probe.
I refer to /drivers/tty/serial/meson_uart.c
And I will modify as below, is that ok?
   sup->clk = devm_clk_get_optional(&pdev->dev, NULL);
   if (IS_ERR(sup->clk))
     return dev_err_probe(&pdev->dev, PTR_ERR(sup->clk), "clk not found\n");

   ret = clk_prepare_enable(sup->clk);
   if (ret)
     return ret;
   ret = devm_add_action_or_reset(&pdev->dev, (void(*)(void*))clk_disable_unprepare,
         sup->clk);
   if (ret)
     return ret;

As your comment last patch v3 , you said "think of deferred".
So I refer to /drivers/tty/serial/sccnxp.c
Maybe I just need to do it without deferred probe.

> 
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       port->membase = devm_ioremap_resource(&pdev->dev, res);
> 
> We have an API that does both at once. Use it.
> 

ok, will modify for it.
devm_platform_get_and_ioremap_resource(pdev, 0, &res);

> > +       if (IS_ERR(port->membase))
> > +               return dev_err_probe(&pdev->dev,
> > + PTR_ERR(port->membase), "membase not found\n");
> 
> ...
> 
> > +       ret = reset_control_deassert(sup->rstc);
> > +       if (ret)
> > +               return ret;
> 
> From here no reset assertion on error? Why?
> 

I found I didn't add devm_add_action_or_reset( ) to run reset_control_assert( ) for it.
I will add it as bleow.

   ret = devm_add_action_or_reset(&pdev->dev, (void(*)(void*))reset_control_assert,
         sup->rstc);
   if (ret)
     return ret;

> ...
> 
> > +#ifdef CONFIG_SERIAL_SUNPLUS_CONSOLE
> > +       if (pdev->id == 0)
> > +               port->cons = &sunplus_uart_console;
> > +       sunplus_console_ports[sup->port.line] = sup; #endif
> 
> Actually why don't you use register_console() ?
> 

I will think how to do it.

> ...
> 
> > +static struct platform_driver sunplus_uart_platform_driver = {
> > +       .probe          = sunplus_uart_probe,
> > +       .remove         = sunplus_uart_remove,
> > +       .driver = {
> > +               .name   = "sunplus_uart",
> 
> > +               .of_match_table = of_match_ptr(sp_uart_of_match),
> 
> How did you test this when OF=n and COMPILE_TEST=y?
> Hint: Compiler will warn about unused variables (you need W=1).
> 
> > +               .pm     = &sunplus_uart_pm_ops,
> > +       }
> > +};
> 

I have found many patch drop of_match_ptr( ) cause warning unused variable.
Now I know what you means at last PATCH v3 comment.
I set OF=n and COMPILE_TEST=y, but other error come out first.
I didn't confirm it further.
Will remove of_match_ptr( ).

> ...
> 
> > +static void __exit sunplus_uart_exit(void) {
> > +       platform_driver_unregister(&sunplus_uart_platform_driver);
> > +       uart_unregister_driver(&sunplus_uart_driver);
> > +}
> 
> > +
> 
> Dtop this blank line...
> 
> > +module_init(sunplus_uart_init);
> > +module_exit(sunplus_uart_exit);
> 
> ...and attach each of the calls to the implemented function.
> 

Ok , will modify it.

> ...
> 
> > +static int __init
> > +sunplus_uart_early_setup(struct earlycon_device *dev, const char
> > +*opt) {
> > +       if (!(dev->port.membase || dev->port.iobase))
> > +               return -ENODEV;
> > +
> > +       dev->con->write = sunplus_uart_early_write;
> > +
> > +       return 0;
> > +}
> 
> > +
> 
> No blank line.
> 

Use scripts/checkpatch.pl --strict -f drivers/tty/serial/sunplus-uart.c
It will show "CHECK: Please use a blank line after function/struct/union/enum declarations".
That's why I confuse it and add a blank for it.
ok, will remove the blank.

> > +OF_EARLYCON_DECLARE(sunplus_uart, "sunplus,sp7021-uart",
> > +sunplus_uart_early_setup);
> 
> --
> 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