MIME-Version: 1.0
References: <1635752903-14968-1-git-send-email-hammer.hsieh@sunplus.com>
 <1637299163-6460-1-git-send-email-hammer.hsieh@sunplus.com> <1637299163-6460-3-git-send-email-hammer.hsieh@sunplus.com>
In-Reply-To: <1637299163-6460-3-git-send-email-hammer.hsieh@sunplus.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 19 Nov 2021 11:35:07 +0200
Message-ID: <CAHp75Vca4r6uZm2Gdas+_Aj5pNgemKjZJj0U35A_g3_sbmCjSg@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
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@sunplus.com,
        wells.lu@sunplus.com, Hammer Hsieh <hammer.hsieh@sunplus.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

...

> +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 <hammer.hsieh@sunplus.com>
> + * Tony Huang <tony.huang@sunplus.com>
> + * Wells Lu <wells.lu@sunplus.com>

> + *

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

...

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