| 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 |
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
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 --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, };
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(-)