Re: [PATCH v5 2/3] serial: 8250: Add Loongson uart driver support

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3608 bytes --]

On Thu, 9 Oct 2025, Binbin Zhou wrote:

> Hi Ilpo:
> 
> Sorry for the late reply and thanks for your detailed review.
> 
> On Tue, Sep 30, 2025 at 7:58 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Wed, 24 Sep 2025, Binbin Zhou wrote:
> >
> > > Add the driver for on-chip UART used on Loongson family chips.
> > >
> > > The hardware is similar to 8250, but there are the following
> > > differences:
> > >  - Some chips (such as Loongson-2K2000) have added a fractional division
> > >    register to obtain the required baud rate accurately, so the
> > >    {get,set}_divisor callback is overridden.
> > >  - Due to hardware defects, quirk handling is required for
> > >    UART_MCR/UART_MSR.
> > >
> > > Co-developed-by: Haowei Zheng <zhenghaowei@loongson.cn>
> > > Signed-off-by: Haowei Zheng <zhenghaowei@loongson.cn>
> > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> > > ---

> > > +static unsigned int serial_fixup(struct uart_port *p, unsigned int offset, unsigned int val)
> > > +{
> > > +     struct loongson_uart_data *ddata = p->private_data;
> > > +
> > > +     if (offset == UART_MCR)
> > > +             val ^= ddata->mcr_invert;
> > > +
> > > +     if (offset == UART_MSR)
> > > +             val ^= ddata->msr_invert;

One additional thing, this could use switch/case.

I'd just do:

	...
	case UART_MSR:
		return val ^ ddata->msr_invert;
	default:
		return val;
	}

That way you don't need to use breaks.

> > > +     if (flags & LOONGSON_UART_HAS_FRAC) {
> > > +             uart.port.get_divisor = loongson_frac_get_divisor;
> > > +             uart.port.set_divisor = loongson_frac_set_divisor;
> > > +     }
> > > +
> > > +     if (flags & LOONGSON_UART_QUIRK_MCR)
> > > +             ddata->mcr_invert |= (UART_MCR_RTS | UART_MCR_DTR);
> > > +
> > > +     if (flags & LOONGSON_UART_QUIRK_MSR)
> > > +             ddata->msr_invert |= (UART_MSR_CTS | UART_MSR_DSR);
> >
> > I think it would be better to put these invert masks directly into a
> > struct which is then put into .data. LOONGSON_UART_HAS_FRAC can be bool
> > in that struct.
> 
> I attempted the following refactoring:
> 
> struct loongson_uart_ddata {
>         bool has_frac;
>         u8 mcr_invert;
>         u8 msr_invert;
> };
> 
> static const struct loongson_uart_ddata ls2k0500_uart_data {
>         .has_frac = false,
>         .mcr_invert = UART_MCR_RTS | UART_MCR_DTR,
>         .msr_invert = UART_MSR_CTS | UART_MSR_DSR,
> };
> 
> static const struct loongson_uart_ddata ls2k1500_uart_data {
>         .has_frac = true,
>         .mcr_invert = UART_MCR_RTS | UART_MCR_DTR,
>         .msr_invert = 0,
> };
> 
> struct loongson_uart_priv {
>         int line;
>         struct clk *clk;
>         struct resource *res;
>         struct reset_control *rst;
>         struct loongson_uart_ddata *ddata;

This can and should be const struct as well as those underlying data 
structs are const too.

> };
> 
> .............
> In loongson_uart_probe():
>        priv->ddata = device_get_match_data(dev);
> 
>         if (priv->ddata->has_frac) {
>                 port->get_divisor = loongson_frac_get_divisor;
>                 port->set_divisor = loongson_frac_set_divisor;
>         }
> 
> 
> .............
> static const struct of_device_id loongson_uart_of_ids[] = {
>         { .compatible = "loongson,ls2k0500-uart", .data = ls2k0500_uart_data },
>         { .compatible = "loongson,ls2k1500-uart", .data = ls2k1500_uart_data },
>         { },
> };


-- 
 i.