Re: [PATCH v4 3/9] serial: 8250: dw: Create a more generic platform data structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 30, 2022 at 03:20:32PM +0200, Miquel Raynal wrote:
> Before adding more platform data information, let's turn the quirks
> information as being a member of a wider structure. More fields will be
> added later.
> 
> At the same time, change the quirks type to unsigned int, as its size
> won't change between setups and we don't really need this variable to be
> more than a few bits wide anyway.
> 
> Provide a stub to the compatibles without quirks to simplify handling.
> Keep two different empty structure for the base DW compatible and the
> Renesas one because the latter will soon be fulfilled anyway.

I'm almost fine with it,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

Please, see below remarks.

> Cc: Emil Renner Berthing <kernel@xxxxxxxx>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> ---
>  drivers/tty/serial/8250/8250_dw.c    | 32 ++++++++++++++++++++++------
>  drivers/tty/serial/8250/8250_dwlib.h |  5 +++++
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index b6238b9bf0b2..70a843e31ffd 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -19,6 +19,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/workqueue.h>
>  #include <linux/notifier.h>
>  #include <linux/slab.h>
> @@ -371,7 +372,7 @@ static bool dw8250_idma_filter(struct dma_chan *chan, void *param)
>  static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  {
>  	struct device_node *np = p->dev->of_node;
> -	unsigned long quirks = (unsigned long)device_get_match_data(p->dev);
> +	unsigned int quirks = data->pdata->quirks;
>  
>  	if (np) {
>  		int id;
> @@ -462,6 +463,7 @@ static int dw8250_probe(struct platform_device *pdev)
>  
>  	data->data.dma.fn = dw8250_fallback_dma_filter;
>  	data->usr_reg = DW_UART_USR;

(1)

> +	data->pdata = device_get_match_data(p->dev);
>  	p->private_data = &data->data;
>  
>  	data->uart_16550_compatible = device_property_read_bool(dev,
> @@ -678,13 +680,29 @@ static const struct dev_pm_ops dw8250_pm_ops = {
>  	SET_RUNTIME_PM_OPS(dw8250_runtime_suspend, dw8250_runtime_resume, NULL)
>  };
>  
> +static const struct dw8250_platform_data dw8250_dw_apb = {};

(2)


' = {}' part is actually redundant in (2), but can we move (1) to pdata
(perhaps as a separate patch)?

In such case I would change (2) to

  static const struct dw8250_platform_data dw8250_dw_apb = {
  };

> +static const struct dw8250_platform_data dw8250_octeon_3860_data = {
> +	.quirks = DW_UART_QUIRK_OCTEON,
> +};
> +
> +static const struct dw8250_platform_data dw8250_armada_38x_data = {
> +	.quirks = DW_UART_QUIRK_ARMADA_38X,
> +};
> +
> +static const struct dw8250_platform_data dw8250_renesas_rzn1_data = {};
> +
> +static const struct dw8250_platform_data dw8250_starfive_jh7100_data = {
> +	.quirks = DW_UART_QUIRK_SKIP_SET_RATE,
> +};
> +
>  static const struct of_device_id dw8250_of_match[] = {
> -	{ .compatible = "snps,dw-apb-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-hsuart",  .data = (void *)DW_UART_QUIRK_SKIP_SET_RATE },
> -	{ .compatible = "starfive,jh7100-uart",    .data = (void *)DW_UART_QUIRK_SKIP_SET_RATE },
> +	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
> +	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
> +	{ .compatible = "marvell,armada-38x-uart", .data = &dw8250_armada_38x_data },
> +	{ .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },
> +	{ .compatible = "starfive,jh7100-hsuart", .data = &dw8250_starfive_jh7100_data },
> +	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_starfive_jh7100_data },
>  	{ /* Sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, dw8250_of_match);
> diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
> index 72e7dbcccad0..68bb81bee660 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.h
> +++ b/drivers/tty/serial/8250/8250_dwlib.h
> @@ -21,8 +21,13 @@ struct dw8250_port_data {
>  	u8			dlf_size;
>  };
>  
> +struct dw8250_platform_data {
> +	unsigned int quirks;
> +};
> +
>  struct dw8250_data {
>  	struct dw8250_port_data	data;
> +	const struct dw8250_platform_data *pdata;
>  
>  	u8			usr_reg;
>  	int			msr_mask_on;
> -- 
> 2.27.0
> 

-- 
With Best Regards,
Andy Shevchenko





[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux