Re: [PATCH] serial: 8250_dw: Use device tree match data

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Emil,

kernel@esmil.dk wrote on Fri, 11 Mar 2022 15:48:14 +0100:

> ..rather than multiple calls to of_device_is_compatible().
> 
> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
> ---
> 
> Hi Miquel,
> 
> > > > > --- a/drivers/tty/serial/8250/8250_dma.c
> > > > > +++ b/drivers/tty/serial/8250/8250_dma.c =20  
> > > > =20  
> > > > > @@ -501,6 +589,8 @@ static int dw8250_probe(struct platform_device *p=  
> > dev)  
> > > > >                 data->msr_mask_off |=3D UART_MSR_TERI;
> > > > >         }
> > > > >
> > > > > +       data->is_rzn1 =3D of_device_is_compatible(dev->of_node, "rene=  
> > sas,rzn1-uart"); =20  
> > > >
> > > > Explicit checks for compatible values are frowned upon if you have
> > > > a match table.
> > > > Please handle this through of_device.data, cfr. the various quirks. =20  
> > >=20
> > > Oops, these are not yet upstream, but present in my tree due to including
> > > support for StarLight, cfr.
> > > https://github.com/esmil/linux/commits/visionfive/drivers/tty/serial/8250=  
> > /8250_dw.c
> > 
> > Oh thanks for pointing it! Too bad that these quirks were not
> > introduced inside a wider structure, I think it's always a must even if
> > there is only one parameter there. Anyway, I'll introduce a wider
> > specific structure and use it.  
> 
> For reference this is the patch I wrote for the StarFive JH7100 tree.
> Feel free to use it or do something better as you see fit.

Thanks for the pointers, I've fetched the three 8250_dw patches from
your tree directly, and will build on top of them!

Cheers,
Miquèl

> 
> /Emil
> 
>  drivers/tty/serial/8250/8250_dw.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 1769808031c5..f564a019a7be 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -37,6 +37,11 @@
>  /* DesignWare specific register fields */
>  #define DW_UART_MCR_SIRE		BIT(6)
>  
> +/* Quirks */
> +#define DW_UART_QUIRK_OCTEON		BIT(0)
> +#define DW_UART_QUIRK_ARMADA_38X	BIT(1)
> +#define DW_UART_QUIRK_SKIP_SET_RATE	BIT(2)
> +
>  struct dw8250_data {
>  	struct dw8250_port_data	data;
>  
> @@ -389,6 +394,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  	struct device_node *np = p->dev->of_node;
>  
>  	if (np) {
> +		unsigned long quirks = (unsigned long)of_device_get_match_data(p->dev);
>  		int id;
>  
>  		/* get index of serial line, if found in DT aliases */
> @@ -396,7 +402,7 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  		if (id >= 0)
>  			p->line = id;
>  #ifdef CONFIG_64BIT
> -		if (of_device_is_compatible(np, "cavium,octeon-3860-uart")) {
> +		if (quirks & DW_UART_QUIRK_OCTEON) {
>  			p->serial_in = dw8250_serial_inq;
>  			p->serial_out = dw8250_serial_outq;
>  			p->flags = UPF_SKIP_TEST | UPF_SHARE_IRQ | UPF_FIXED_TYPE;
> @@ -412,9 +418,9 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  			p->serial_out = dw8250_serial_out32be;
>  		}
>  
> -		if (of_device_is_compatible(np, "marvell,armada-38x-uart"))
> +		if (quirks & DW_UART_QUIRK_ARMADA_38X)
>  			p->serial_out = dw8250_serial_out38x;
> -		if (of_device_is_compatible(np, "starfive,jh7100-uart"))
> +		if (quirks & DW_UART_QUIRK_SKIP_SET_RATE)
>  			p->set_termios = dw8250_do_set_termios;
>  
>  	} else if (acpi_dev_present("APMC0D08", NULL, -1)) {
> @@ -695,10 +701,10 @@ static const struct dev_pm_ops dw8250_pm_ops = {
>  
>  static const struct of_device_id dw8250_of_match[] = {
>  	{ .compatible = "snps,dw-apb-uart" },
> -	{ .compatible = "cavium,octeon-3860-uart" },
> -	{ .compatible = "marvell,armada-38x-uart" },
> +	{ .compatible = "cavium,octeon-3860-uart", .data = (void *)DW_UART_QUIRK_OCTEON },
> +	{ .compatible = "marvell,armada-38x-uart", .data = (void *)DW_UART_QUIRK_ARMADA_38X },
>  	{ .compatible = "renesas,rzn1-uart" },
> -	{ .compatible = "starfive,jh7100-uart" },
> +	{ .compatible = "starfive,jh7100-uart",    .data = (void *)DW_UART_QUIRK_SKIP_SET_RATE },
>  	{ /* Sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, dw8250_of_match);


Thanks,
Miquèl