diff mbox series

[v5,4/7] gpio: vf610: add i.MX8ULP of_device_id entry

Message ID 20231001-vf610-gpio-v5-4-8d873a8f224a@nxp.com
State New
Headers show
Series None | expand

Commit Message

Peng Fan (OSS) Oct. 1, 2023, 8:27 a.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

i.MX8ULP/93 GPIO supports similar feature as i.MX7ULP GPIO, but i.MX8ULP is
actually not hardware compatible with i.MX7ULP. i.MX8ULP only has one
register base, not two bases. i.MX8ULP and i.MX93 actually has two
interrupts for each gpio controller, one for Trustzone non-secure world,
one for secure world.

Although the Linux Kernel driver gpio-vf610.c could work with
fsl,imx7ulp-gpio compatible, it is based on some tricks did in device tree
with some offset added to base address.

Add a new of_device_id entry for i.MX8ULP. But to make the driver could
also support old bindings, check the compatible string first, before
check the device data.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/gpio/gpio-vf610.c | 47 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 40 insertions(+), 7 deletions(-)

Comments

Marco Felsch Oct. 4, 2023, 5:04 a.m. UTC | #1 
Hi Peng,

On 23-10-02, Peng Fan wrote:
> > Subject: Re: [PATCH v5 4/7] gpio: vf610: add i.MX8ULP of_device_id entry
> > 
> > On Sun, Oct 1, 2023 at 10:23 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > i.MX8ULP/93 GPIO supports similar feature as i.MX7ULP GPIO, but
> > > i.MX8ULP is actually not hardware compatible with i.MX7ULP. i.MX8ULP
> > > only has one register base, not two bases. i.MX8ULP and i.MX93
> > > actually has two interrupts for each gpio controller, one for
> > > Trustzone non-secure world, one for secure world.
> > >
> > > Although the Linux Kernel driver gpio-vf610.c could work with
> > > fsl,imx7ulp-gpio compatible, it is based on some tricks did in device
> > > tree with some offset added to base address.
> > >
> > > Add a new of_device_id entry for i.MX8ULP. But to make the driver
> > > could also support old bindings, check the compatible string first,
> > > before check the device data.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/gpio/gpio-vf610.c | 47
> > > ++++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 40 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> > > index dbc7ba0ee72c..8e12706c0b22 100644
> > > --- a/drivers/gpio/gpio-vf610.c
> > > +++ b/drivers/gpio/gpio-vf610.c
> > > @@ -25,6 +25,7 @@
> > >  struct fsl_gpio_soc_data {
> > >         /* SoCs has a Port Data Direction Register (PDDR) */
> > >         bool have_paddr;
> > > +       bool have_dual_base;
> > >  };
> > >
> > >  struct vf610_gpio_port {
> > > @@ -60,13 +61,26 @@ struct vf610_gpio_port {
> > >  #define PORT_INT_EITHER_EDGE   0xb
> > >  #define PORT_INT_LOGIC_ONE     0xc
> > >
> > > +#define IMX8ULP_GPIO_BASE_OFF  0x40
> > > +#define IMX8ULP_BASE_OFF       0x80
> > > +
> > > +static const struct fsl_gpio_soc_data vf610_data = {
> > > +       .have_dual_base = true,
> > > +};
> > > +
> > >  static const struct fsl_gpio_soc_data imx_data = {
> > >         .have_paddr = true,
> > > +       .have_dual_base = true,
> > > +};
> > > +
> > > +static const struct fsl_gpio_soc_data imx8ulp_data = {
> > > +       .have_paddr = true,
> > >  };
> > >
> > >  static const struct of_device_id vf610_gpio_dt_ids[] = {
> > > -       { .compatible = "fsl,vf610-gpio",       .data = NULL, },
> > > +       { .compatible = "fsl,vf610-gpio",       .data = &vf610_data },
> > >         { .compatible = "fsl,imx7ulp-gpio",     .data = &imx_data, },
> > > +       { .compatible = "fsl,imx8ulp-gpio",     .data = &imx8ulp_data, },
> > >         { /* sentinel */ }
> > >  };
> > >
> > > @@ -263,19 +277,38 @@ static int vf610_gpio_probe(struct
> > platform_device *pdev)
> > >         struct gpio_irq_chip *girq;
> > >         int i;
> > >         int ret;
> > > +       bool dual_base;
> > >
> > >         port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> > >         if (!port)
> > >                 return -ENOMEM;
> > >
> > >         port->sdata = of_device_get_match_data(dev);
> > > -       port->base = devm_platform_ioremap_resource(pdev, 0);
> > > -       if (IS_ERR(port->base))
> > > -               return PTR_ERR(port->base);
> > >
> > > -       port->gpio_base = devm_platform_ioremap_resource(pdev, 1);
> > > -       if (IS_ERR(port->gpio_base))
> > > -               return PTR_ERR(port->gpio_base);
> > > +       dual_base = port->sdata->have_dual_base;
> > > +
> > > +       /* support old compatible strings */
> > > +       if (device_is_compatible(dev, "fsl,imx7ulp-gpio") &&
> > > +           (device_is_compatible(dev, "fsl,imx93-gpio") ||
> > 
> > Why not just add this compatible to vf610_gpio_dt_ids?
> 
> "fsl,imx93-gpio", "fsl,imx7ulp-gpio" is not a correct entry
> combination. This is to support legacy old compatible
> strings.

The "/* support old compatible strings */" may a bit misleading here?
Should we be a bit more verbose for the reader of the code, e.g.:

		/*
		 * Handle legacy compatible combinations which used two
		 * reg values for the i.MX8ULP and i.MX93.
		 */

Regards,
  Marco

> 
> Thanks,
> Peng.
> > 
> > Bart
> > 
> > > +           (device_is_compatible(dev, "fsl,imx8ulp-gpio"))))
> > > +               dual_base = true;
> > > +
> > > +       if (dual_base) {
> > > +               port->base = devm_platform_ioremap_resource(pdev, 0);
> > > +               if (IS_ERR(port->base))
> > > +                       return PTR_ERR(port->base);
> > > +
> > > +               port->gpio_base = devm_platform_ioremap_resource(pdev, 1);
> > > +               if (IS_ERR(port->gpio_base))
> > > +                       return PTR_ERR(port->gpio_base);
> > > +       } else {
> > > +               port->base = devm_platform_ioremap_resource(pdev, 0);
> > > +               if (IS_ERR(port->base))
> > > +                       return PTR_ERR(port->base);
> > > +
> > > +               port->gpio_base = port->base + IMX8ULP_GPIO_BASE_OFF;
> > > +               port->base = port->base + IMX8ULP_BASE_OFF;
> > > +       }
> > >
> > >         port->irq = platform_get_irq(pdev, 0);
> > >         if (port->irq < 0)
> > >
> > > --
> > > 2.37.1
> > >
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
index dbc7ba0ee72c..8e12706c0b22 100644
--- a/drivers/gpio/gpio-vf610.c
+++ b/drivers/gpio/gpio-vf610.c
@@ -25,6 +25,7 @@ 
 struct fsl_gpio_soc_data {
 	/* SoCs has a Port Data Direction Register (PDDR) */
 	bool have_paddr;
+	bool have_dual_base;
 };
 
 struct vf610_gpio_port {
@@ -60,13 +61,26 @@  struct vf610_gpio_port {
 #define PORT_INT_EITHER_EDGE	0xb
 #define PORT_INT_LOGIC_ONE	0xc
 
+#define IMX8ULP_GPIO_BASE_OFF	0x40
+#define IMX8ULP_BASE_OFF	0x80
+
+static const struct fsl_gpio_soc_data vf610_data = {
+	.have_dual_base = true,
+};
+
 static const struct fsl_gpio_soc_data imx_data = {
 	.have_paddr = true,
+	.have_dual_base = true,
+};
+
+static const struct fsl_gpio_soc_data imx8ulp_data = {
+	.have_paddr = true,
 };
 
 static const struct of_device_id vf610_gpio_dt_ids[] = {
-	{ .compatible = "fsl,vf610-gpio",	.data = NULL, },
+	{ .compatible = "fsl,vf610-gpio",	.data = &vf610_data },
 	{ .compatible = "fsl,imx7ulp-gpio",	.data = &imx_data, },
+	{ .compatible = "fsl,imx8ulp-gpio",	.data = &imx8ulp_data, },
 	{ /* sentinel */ }
 };
 
@@ -263,19 +277,38 @@  static int vf610_gpio_probe(struct platform_device *pdev)
 	struct gpio_irq_chip *girq;
 	int i;
 	int ret;
+	bool dual_base;
 
 	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
 	if (!port)
 		return -ENOMEM;
 
 	port->sdata = of_device_get_match_data(dev);
-	port->base = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(port->base))
-		return PTR_ERR(port->base);
 
-	port->gpio_base = devm_platform_ioremap_resource(pdev, 1);
-	if (IS_ERR(port->gpio_base))
-		return PTR_ERR(port->gpio_base);
+	dual_base = port->sdata->have_dual_base;
+
+	/* support old compatible strings */
+	if (device_is_compatible(dev, "fsl,imx7ulp-gpio") &&
+	    (device_is_compatible(dev, "fsl,imx93-gpio") ||
+	    (device_is_compatible(dev, "fsl,imx8ulp-gpio"))))
+		dual_base = true;
+
+	if (dual_base) {
+		port->base = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(port->base))
+			return PTR_ERR(port->base);
+
+		port->gpio_base = devm_platform_ioremap_resource(pdev, 1);
+		if (IS_ERR(port->gpio_base))
+			return PTR_ERR(port->gpio_base);
+	} else {
+		port->base = devm_platform_ioremap_resource(pdev, 0);
+		if (IS_ERR(port->base))
+			return PTR_ERR(port->base);
+
+		port->gpio_base = port->base + IMX8ULP_GPIO_BASE_OFF;
+		port->base = port->base + IMX8ULP_BASE_OFF;
+	}
 
 	port->irq = platform_get_irq(pdev, 0);
 	if (port->irq < 0)