Re: [PATCH v5 05/11] serial: 8250: dw: Allow to use a fallback CPR value if not synthesized

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Apr 13, 2022 at 09:51:35AM +0200, Miquel Raynal wrote:
> DW UART controllers can be synthesized without the CPR register.
> In this case, allow to the platform information to provide a CPR value.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Some nit-picks below.

> Co-developed-by: Phil Edworthy <phil.edworthy@renesas.com>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/tty/serial/8250/8250_dwlib.c | 10 +++++++---
>  drivers/tty/serial/8250/8250_dwlib.h |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
> index 622d3b0d89e7..22154b5848d5 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.c
> +++ b/drivers/tty/serial/8250/8250_dwlib.c
> @@ -89,6 +89,8 @@ EXPORT_SYMBOL_GPL(dw8250_do_set_termios);
>  
>  void dw8250_setup_port(struct uart_port *p)
>  {
> +	struct dw8250_port_data *pd = p->private_data;
> +	struct dw8250_data *data = to_dw8250_data(pd);
>  	struct uart_8250_port *up = up_to_u8250p(p);
>  	u32 reg;
>  
> @@ -108,14 +110,16 @@ void dw8250_setup_port(struct uart_port *p)
>  	dw8250_writel_ext(p, DW_UART_DLF, 0);
>  
>  	if (reg) {
> -		struct dw8250_port_data *d = p->private_data;
> -
> -		d->dlf_size = fls(reg);
> +		pd->dlf_size = fls(reg);
>  		p->get_divisor = dw8250_get_divisor;
>  		p->set_divisor = dw8250_set_divisor;
>  	}
>  
>  	reg = dw8250_readl_ext(p, DW_UART_CPR);
> +	if (!reg) {
> +		reg = data->pdata->cpr;
> +		dev_dbg(p->dev, "CPR is not available, using 0x%08x instead\n", reg);
> +	}
>  	if (!reg)
>  		return;
>  
> diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
> index 0df6baa6eaee..412f4e83e0b3 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.h
> +++ b/drivers/tty/serial/8250/8250_dwlib.h
> @@ -24,6 +24,7 @@ struct dw8250_port_data {
>  struct dw8250_platform_data {
>  	u8 usr_reg;
>  	unsigned int quirks;
> +	u32 cpr;

I would probably name it cpr_val (to be in align with _reg above) and move it
after the usr_reg member. But it's minor, up to you.

>  };
>  
>  struct dw8250_data {

-- 
With Best Regards,
Andy Shevchenko