diff mbox series

[14/17,v1] gpio: ixp4xx: Add OF probing support

Message ID 20190203214205.13594-15-linus.walleij@linaro.org (mailing list archive)
State New, archived
Headers show
Series ARM: ixp4xx: Modernize and DT support | expand

Commit Message

Linus Walleij Feb. 3, 2019, 9:42 p.m. UTC
This adds device tree probe and registration support for
the IXP4xx GPIO driver.

Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Bartosz: looking for your ACK on this, it'd be good if
the other GPIO maintainer is aligned with my ideas here.
I intend to merge this through the ARM SoC tree.
---
 drivers/gpio/gpio-ixp4xx.c | 84 ++++++++++++++++++++++++++------------
 1 file changed, 57 insertions(+), 27 deletions(-)

Comments

Bartosz Golaszewski Feb. 6, 2019, 4:13 p.m. UTC | #1
niedz., 3 lut 2019 o 22:42 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> This adds device tree probe and registration support for
> the IXP4xx GPIO driver.
>

What is the reason for not merging it with the patch adding the driver?

> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Bartosz: looking for your ACK on this, it'd be good if
> the other GPIO maintainer is aligned with my ideas here.
> I intend to merge this through the ARM SoC tree.
> ---
>  drivers/gpio/gpio-ixp4xx.c | 84 ++++++++++++++++++++++++++------------
>  1 file changed, 57 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ixp4xx.c b/drivers/gpio/gpio-ixp4xx.c
> index 44c24948379d..38a77c8e0c9c 100644
> --- a/drivers/gpio/gpio-ixp4xx.c
> +++ b/drivers/gpio/gpio-ixp4xx.c
> @@ -11,6 +11,7 @@
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/irqchip.h>
> +#include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/bitops.h>
>  /* Include that go away with DT transition */
> @@ -305,6 +306,7 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
>  {
>         unsigned long flags;
>         struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
>         struct irq_domain *parent;
>         struct resource *res;
>         struct ixp4xx_gpio *g;
> @@ -381,11 +383,27 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
>          * from IRQCHIP_DECLARE(), then use of_node_to_fwnode() to get
>          * the fwnode. For now we need this boardfile style code.
>          */

Is this comment still valid after this patch?  Or is this maybe
incomplete DT support (probing only)? If so it need clarifying.

> -       parent = ixp4xx_get_irq_domain();
> -       g->fwnode = irq_domain_alloc_fwnode(g->base);
> -       if (!g->fwnode) {
> -               dev_err(dev, "no domain base\n");
> -               return -ENODEV;
> +       if (np) {
> +               struct device_node *irq_parent;
> +
> +               irq_parent = of_irq_find_parent(np);
> +               if (!irq_parent) {
> +                       dev_err(dev, "no IRQ parent node\n");
> +                       return -ENODEV;
> +               }
> +               parent = irq_find_host(irq_parent);
> +               if (!parent) {
> +                       dev_err(dev, "no IRQ parent domain\n");
> +                       return -ENODEV;
> +               }
> +               g->fwnode = of_node_to_fwnode(np);
> +       } else {
> +               parent = ixp4xx_get_irq_domain();
> +               g->fwnode = irq_domain_alloc_fwnode(g->base);
> +               if (!g->fwnode) {
> +                       dev_err(dev, "no domain base\n");
> +                       return -ENODEV;
> +               }
>         }
>         g->domain = irq_domain_create_hierarchy(parent,
>                                                 IRQ_DOMAIN_FLAG_HIERARCHY,
> @@ -403,28 +421,31 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
>          * After adding OF support, this is no longer needed: irqs
>          * will be allocated for the respective fwnodes.
>          */

Same with the comment here.

> -       for (i = 0; i < ARRAY_SIZE(ixp4xx_gpiomap); i++) {
> -               const struct ixp4xx_gpio_map *map = &ixp4xx_gpiomap[i];
> -               struct irq_fwspec fwspec;
> -
> -               fwspec.fwnode = g->fwnode;
> -               /* This is the hwirq for the GPIO line side of things */
> -               fwspec.param[0] = map->gpio_offset;
> -               fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> -               fwspec.param_count = 2;
> -               ret = __irq_domain_alloc_irqs(g->domain,
> -                                             -1, /* just pick something */
> -                                             1,
> -                                             NUMA_NO_NODE,
> -                                             &fwspec,
> -                                             false,
> -                                             NULL);
> -               if (ret < 0) {
> -                       irq_domain_free_fwnode(g->fwnode);
> -                       dev_err(dev,
> -                               "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
> -                               map->gpio_offset, map->parent_hwirq, ret);
> -                       return ret;
> +       if (!np) {
> +               for (i = 0; i < ARRAY_SIZE(ixp4xx_gpiomap); i++) {
> +                       const struct ixp4xx_gpio_map *map = &ixp4xx_gpiomap[i];
> +                       struct irq_fwspec fwspec;
> +
> +                       fwspec.fwnode = g->fwnode;
> +                       /* This is the hwirq for the GPIO line side of things */
> +                       fwspec.param[0] = map->gpio_offset;
> +                       fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +                       fwspec.param_count = 2;
> +                       ret = __irq_domain_alloc_irqs(g->domain,
> +                                                     -1, /* just pick something */
> +                                                     1,
> +                                                     NUMA_NO_NODE,
> +                                                     &fwspec,
> +                                                     false,
> +                                                     NULL);
> +                       if (ret < 0) {
> +                               irq_domain_free_fwnode(g->fwnode);
> +                               dev_err(dev,
> +                                       "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
> +                                       map->gpio_offset, map->parent_hwirq,
> +                                       ret);
> +                               return ret;
> +                       }
>                 }
>         }
>
> @@ -434,9 +455,18 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
>         return 0;
>  }
>
> +static const struct of_device_id ixp4xx_gpio_of_match[] = {
> +       {
> +               .compatible = "intel,ixp4xx-gpio",
> +       },
> +       {},
> +};
> +
> +
>  static struct platform_driver ixp4xx_gpio_driver = {
>         .driver = {
>                 .name           = "ixp4xx-gpio",
> +               .of_match_table = of_match_ptr(ixp4xx_gpio_of_match),
>         },
>         .probe = ixp4xx_gpio_probe,
>  };
> --
> 2.20.1
>

Bart
Linus Walleij Feb. 21, 2019, 8:55 a.m. UTC | #2
On Wed, Feb 6, 2019 at 5:13 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> niedz., 3 lut 2019 o 22:42 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > This adds device tree probe and registration support for
> > the IXP4xx GPIO driver.
>
> What is the reason for not merging it with the patch adding the driver?

Since I am not adding a new never before used gpio driver
but refactoring an existing system, I need to take one technical
step at the time for bisectability and readability.

If I was adding a driver for a totally new system indeed it would
be in the same patch.

> > @@ -381,11 +383,27 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
> >          * from IRQCHIP_DECLARE(), then use of_node_to_fwnode() to get
> >          * the fwnode. For now we need this boardfile style code.
> >          */
>
> Is this comment still valid after this patch?  Or is this maybe
> incomplete DT support (probing only)? If so it need clarifying.

We are supporting board files and device tree in parallel during
a transition period, the driver handled both static and dynamic
assignment (hence the complexity). This is the typical mess
with converting old systems in to new ones, a few kernels down
the road when all boardfiles are converted to device trees
the complexity can be removed.

But it should not say "after adding OF support" but rather
"when we are using OF exclusively".

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-ixp4xx.c b/drivers/gpio/gpio-ixp4xx.c
index 44c24948379d..38a77c8e0c9c 100644
--- a/drivers/gpio/gpio-ixp4xx.c
+++ b/drivers/gpio/gpio-ixp4xx.c
@@ -11,6 +11,7 @@ 
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/bitops.h>
 /* Include that go away with DT transition */
@@ -305,6 +306,7 @@  static int ixp4xx_gpio_probe(struct platform_device *pdev)
 {
 	unsigned long flags;
 	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct irq_domain *parent;
 	struct resource *res;
 	struct ixp4xx_gpio *g;
@@ -381,11 +383,27 @@  static int ixp4xx_gpio_probe(struct platform_device *pdev)
 	 * from IRQCHIP_DECLARE(), then use of_node_to_fwnode() to get
 	 * the fwnode. For now we need this boardfile style code.
 	 */
-	parent = ixp4xx_get_irq_domain();
-	g->fwnode = irq_domain_alloc_fwnode(g->base);
-	if (!g->fwnode) {
-		dev_err(dev, "no domain base\n");
-		return -ENODEV;
+	if (np) {
+		struct device_node *irq_parent;
+
+		irq_parent = of_irq_find_parent(np);
+		if (!irq_parent) {
+			dev_err(dev, "no IRQ parent node\n");
+			return -ENODEV;
+		}
+		parent = irq_find_host(irq_parent);
+		if (!parent) {
+			dev_err(dev, "no IRQ parent domain\n");
+			return -ENODEV;
+		}
+		g->fwnode = of_node_to_fwnode(np);
+	} else {
+		parent = ixp4xx_get_irq_domain();
+		g->fwnode = irq_domain_alloc_fwnode(g->base);
+		if (!g->fwnode) {
+			dev_err(dev, "no domain base\n");
+			return -ENODEV;
+		}
 	}
 	g->domain = irq_domain_create_hierarchy(parent,
 						IRQ_DOMAIN_FLAG_HIERARCHY,
@@ -403,28 +421,31 @@  static int ixp4xx_gpio_probe(struct platform_device *pdev)
 	 * After adding OF support, this is no longer needed: irqs
 	 * will be allocated for the respective fwnodes.
 	 */
-	for (i = 0; i < ARRAY_SIZE(ixp4xx_gpiomap); i++) {
-		const struct ixp4xx_gpio_map *map = &ixp4xx_gpiomap[i];
-		struct irq_fwspec fwspec;
-
-		fwspec.fwnode = g->fwnode;
-		/* This is the hwirq for the GPIO line side of things */
-		fwspec.param[0] = map->gpio_offset;
-		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
-		fwspec.param_count = 2;
-		ret = __irq_domain_alloc_irqs(g->domain,
-					      -1, /* just pick something */
-					      1,
-					      NUMA_NO_NODE,
-					      &fwspec,
-					      false,
-					      NULL);
-		if (ret < 0) {
-			irq_domain_free_fwnode(g->fwnode);
-			dev_err(dev,
-				"can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
-				map->gpio_offset, map->parent_hwirq, ret);
-			return ret;
+	if (!np) {
+		for (i = 0; i < ARRAY_SIZE(ixp4xx_gpiomap); i++) {
+			const struct ixp4xx_gpio_map *map = &ixp4xx_gpiomap[i];
+			struct irq_fwspec fwspec;
+
+			fwspec.fwnode = g->fwnode;
+			/* This is the hwirq for the GPIO line side of things */
+			fwspec.param[0] = map->gpio_offset;
+			fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+			fwspec.param_count = 2;
+			ret = __irq_domain_alloc_irqs(g->domain,
+						      -1, /* just pick something */
+						      1,
+						      NUMA_NO_NODE,
+						      &fwspec,
+						      false,
+						      NULL);
+			if (ret < 0) {
+				irq_domain_free_fwnode(g->fwnode);
+				dev_err(dev,
+					"can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
+					map->gpio_offset, map->parent_hwirq,
+					ret);
+				return ret;
+			}
 		}
 	}
 
@@ -434,9 +455,18 @@  static int ixp4xx_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id ixp4xx_gpio_of_match[] = {
+	{
+		.compatible = "intel,ixp4xx-gpio",
+	},
+	{},
+};
+
+
 static struct platform_driver ixp4xx_gpio_driver = {
 	.driver = {
 		.name		= "ixp4xx-gpio",
+		.of_match_table = of_match_ptr(ixp4xx_gpio_of_match),
 	},
 	.probe = ixp4xx_gpio_probe,
 };