| Message ID | 20171102174941.3461-1-thierry.reding@gmail.com |
|---|---|
| Headers | show |
| Series | gpio: Tight IRQ chip integration | expand |
Hi On 11/02/2017 12:49 PM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Hi Linus, > > here's the latest series of patches that implement the tighter IRQ chip > integration. I've dropped the banked infrastructure for now as per the > discussion with Grygorii. > > The first couple of patches are mostly preparatory work in order to > consolidate all IRQ chip related fields in a new structure and create > the base functionality for adding IRQ chips. > > After that, I've added the Tegra186 GPIO support patch that makes use of > the new tight integration. > > Changes in v6: > - rebased on latest linux-gpio devel branch > - one patch dropped due to rebase > > Changes in v5: > - dropped the banked infrastructure patches for now (Grygorii) > - allocate interrupts on demand, rather than upfront (Grygorii) > - split up the first patch further as requested by Grygorii > > Not sure what happened in between here. Notes in commit logs indicate > that this is actually version 5, but I can't find the cover letter for > v3 and v4. > > Changes in v2: > - rename pins to lines for consistent terminology > - rename gpio_irq_chip_banked_handler() to > gpio_irq_chip_banked_chained_handler() > Sry, for delayed reply, very sorry. Patches 1 - 9, 11 : looks good, so Acked-by: Grygorii Strashko <grygorii.strashko@ti.com> Patch 10 - unfortunately not all my comment were incorporated [1], so below diff on top of patch 10 which illustrates what i want and also converts gpio-omap to use new infra as test for this new infra. Pls, take a look [1] https://www.spinics.net/lists/linux-tegra/msg31145.html -- regards, -grygorii -----------><------------- From 210fe3ad7a642691a1b7603e441f6980b10ff2b4 Mon Sep 17 00:00:00 2001 From: Grygorii Strashko <grygorii.strashko@ti.com> Date: Fri, 3 Nov 2017 17:24:51 -0500 Subject: [PATCH] [not for merge] gpiolib: update and test new gpioirq chip infra changes in gpiolib: - move set_parent to gpiochip_irq_map() and use parents instead of map for only one parent - add gpio_irq_chip->first_irq for static IRQ allocation - fix nested = true if parent_handler not set - fix gpiochip_irqchip_remove() if parent_handler not set - get of_node from gpiodev - fix lock_key: drivers do not need to set it, but looks a bit overcomplicated as lock_class_key will be created for each gpiochip_add_data() call. Honestly, do not seem better way :( GPIO OMAP driver updated to use new gpioirq chip infra and tested on am335x-evm. seems it's working - can see irqs from keys. Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/gpio/gpio-omap.c | 36 ++++++++++++++-------------- drivers/gpio/gpiolib.c | 58 +++++++++++++++++++-------------------------- include/linux/gpio/driver.h | 32 ++++++++++++++++++++++++- 3 files changed, 73 insertions(+), 53 deletions(-) diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index ce27d6a..4048f18 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1054,6 +1054,7 @@ static void omap_gpio_mod_init(struct gpio_bank *bank) static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) { + struct gpio_irq_chip *irq; static int gpio; int irq_base = 0; int ret; @@ -1081,16 +1082,6 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) } bank->chip.ngpio = bank->width; - ret = gpiochip_add_data(&bank->chip, bank); - if (ret) { - dev_err(bank->chip.parent, - "Could not register gpio chip %d\n", ret); - return ret; - } - - if (!bank->is_mpuio) - gpio += bank->width; - #ifdef CONFIG_ARCH_OMAP1 /* * REVISIT: Once we have OMAP1 supporting SPARSE_IRQ, we can drop @@ -1111,25 +1102,34 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc) irqc->irq_set_wake = NULL; } - ret = gpiochip_irqchip_add(&bank->chip, irqc, - irq_base, handle_bad_irq, - IRQ_TYPE_NONE); + irq = &bank->chip.irq; + irq->chip = irqc; +// irq->domain_ops = not required; + irq->handler = handle_bad_irq; +// irq->lock_key = not required; + irq->default_type = IRQ_TYPE_NONE; +// irq->parent_handler = not used by this driver; +// irq->parent_handler_data = not used by this driver; + irq->num_parents = 1; + irq->parents = &bank->irq; + irq->first_irq = irq_base; + ret = gpiochip_add_data(&bank->chip, bank); if (ret) { dev_err(bank->chip.parent, - "Couldn't add irqchip to gpiochip %d\n", ret); - gpiochip_remove(&bank->chip); - return -ENODEV; + "Could not register gpio chip %d\n", ret); + return ret; } - gpiochip_set_chained_irqchip(&bank->chip, irqc, bank->irq, NULL); - ret = devm_request_irq(bank->chip.parent, bank->irq, omap_gpio_irq_handler, 0, dev_name(bank->chip.parent), bank); if (ret) gpiochip_remove(&bank->chip); + if (!bank->is_mpuio) + gpio += bank->width; + return ret; } diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 5bc99d0..d11b4df 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -72,7 +72,8 @@ static LIST_HEAD(gpio_lookup_list); LIST_HEAD(gpio_devices); static void gpiochip_free_hogs(struct gpio_chip *chip); -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip); +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, + struct lock_class_key *lock_key); static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip); static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip); static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip); @@ -1121,7 +1122,8 @@ static void gpiochip_setup_devs(void) * chip->base is invalid or already associated with a different chip. * Otherwise it returns zero as a success code. */ -int gpiochip_add_data(struct gpio_chip *chip, void *data) +int gpiochip_add_data_key(struct gpio_chip *chip, void *data, + struct lock_class_key *irq_lock_key) { unsigned long flags; int status = 0; @@ -1267,7 +1269,7 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data) if (status) goto err_remove_from_list; - status = gpiochip_add_irqchip(chip); + status = gpiochip_add_irqchip_key(chip, irq_lock_key); if (status) goto err_remove_chip; @@ -1642,6 +1644,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) { struct gpio_chip *chip = d->host_data; + int err = 0; if (!gpiochip_irqchip_irq_valid(chip, hwirq)) return -ENXIO; @@ -1658,6 +1661,12 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, irq_set_nested_thread(irq, 1); irq_set_noprobe(irq); + if (chip->irq.num_parents == 1) + err = irq_set_parent(irq, chip->irq.parents[0]); + else if (chip->irq.map) + err = irq_set_parent(irq, chip->irq.map[hwirq]); + if (err < 0) + return err; /* * No set-up of the hardware will happen if IRQ_TYPE_NONE * is passed as default type. @@ -1712,30 +1721,18 @@ static void gpiochip_irq_relres(struct irq_data *d) static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) { - unsigned int irq; - int err; - if (!gpiochip_irqchip_irq_valid(chip, offset)) return -ENXIO; - irq = irq_create_mapping(chip->irq.domain, offset); - if (!irq) - return 0; - - if (chip->irq.map) { - err = irq_set_parent(irq, chip->irq.map[offset]); - if (err < 0) - return err; - } - - return irq; + return irq_create_mapping(chip->irq.domain, offset); } /** * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip * @gpiochip: the GPIO chip to add the IRQ chip to */ -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, + struct lock_class_key *lock_key) { struct irq_chip *irqchip = gpiochip->irq.chip; const struct irq_domain_ops *ops; @@ -1753,17 +1750,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) } type = gpiochip->irq.default_type; - np = gpiochip->parent->of_node; - -#ifdef CONFIG_OF_GPIO - /* - * If the gpiochip has an assigned OF node this takes precedence - * FIXME: get rid of this and use gpiochip->parent->of_node - * everywhere - */ - if (gpiochip->of_node) - np = gpiochip->of_node; -#endif + /* called from gpiochip_add_data() and is set properly */ + np = gpiochip->gpiodev->dev.of_node; /* * Specifying a default trigger is a terrible idea if DT or ACPI is @@ -1789,7 +1777,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) ops = &gpiochip_domain_ops; gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, - 0, ops, gpiochip); + gpiochip->irq.first_irq, + ops, gpiochip); if (!gpiochip->irq.domain) return -EINVAL; @@ -1818,9 +1807,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) } gpiochip->irq.nested = false; - } else { - gpiochip->irq.nested = true; } + /* GPIO driver might not specify parent_handler, but it doesn't mean + * it's irq_nested, as driver may use request_irq() */ acpi_gpiochip_request_interrupts(gpiochip); @@ -1839,7 +1828,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) acpi_gpiochip_free_interrupts(gpiochip); - if (gpiochip->irq.chip) { + if (gpiochip->irq.chip && gpiochip->irq.parent_handler) { struct gpio_irq_chip *irq = &gpiochip->irq; unsigned int i; @@ -1972,7 +1961,8 @@ EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_key); #else /* CONFIG_GPIOLIB_IRQCHIP */ -static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip) +static inline int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, + struct lock_class_key *lock_key) { return 0; } diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 51fc7b0..3254996 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -128,6 +128,15 @@ struct gpio_irq_chip { * in IRQ domain of the chip. */ unsigned long *valid_mask; + + /** + * @first_irq: + * + * Required for static irq allocation. + * if set irq_domain_add_simple() will allocate and map all IRQs + * during initialization. + */ + unsigned int first_irq; }; static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip) @@ -312,8 +321,29 @@ struct gpio_chip { extern const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset); +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data, + struct lock_class_key *irq_lock_key); +#ifdef CONFIG_LOCKDEP +/* + * Lockdep requires that each irqchip instance be created with a + * unique key so as to avoid unnecessary warnings. This upfront + * boilerplate static inlines provides such a key for each + * unique instance which is created now from inside gpiochip_add_data_key(). + */ +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data) +{ + static struct lock_class_key key; + + return gpiochip_add_data_key(chip, data, key); +} + +#else +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data) +{ + return gpiochip_add_data_key(chip, data, NULL); +} +#endif /* CONFIG_LOCKDEP */ /* add/remove chips */ -extern int gpiochip_add_data(struct gpio_chip *chip, void *data); static inline int gpiochip_add(struct gpio_chip *chip) { return gpiochip_add_data(chip, NULL);
On Thu, Nov 2, 2017 at 6:49 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > here's the latest series of patches that implement the tighter IRQ chip > integration. I've dropped the banked infrastructure for now as per the > discussion with Grygorii. And I really liked that part. > The first couple of patches are mostly preparatory work in order to > consolidate all IRQ chip related fields in a new structure and create > the base functionality for adding IRQ chips. > > After that, I've added the Tegra186 GPIO support patch that makes use of > the new tight integration. > > Changes in v6: > - rebased on latest linux-gpio devel branch > - one patch dropped due to rebase I guess I will consider queueing them already for v4.15 if Torvalds churns out another -rc on sunday, I do not doubt that you will finish it as we have discussed with banks and all. If v4.14 is released on sunday let's queue this early for v4.16. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hi Linus, On 11/03/2017 05:50 PM, Linus Walleij wrote: > On Thu, Nov 2, 2017 at 6:49 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > >> here's the latest series of patches that implement the tighter IRQ chip >> integration. I've dropped the banked infrastructure for now as per the >> discussion with Grygorii. > > And I really liked that part. > >> The first couple of patches are mostly preparatory work in order to >> consolidate all IRQ chip related fields in a new structure and create >> the base functionality for adding IRQ chips. >> >> After that, I've added the Tegra186 GPIO support patch that makes use of >> the new tight integration. >> >> Changes in v6: >> - rebased on latest linux-gpio devel branch >> - one patch dropped due to rebase > > I guess I will consider queueing them already for v4.15 if > Torvalds churns out another -rc on sunday, I do not > doubt that you will finish it as we have discussed with banks and > all. If v4.14 is released on sunday let's queue this early for v4.16. Can you see my reply? https://lkml.org/lkml/2017/11/3/820
On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote: > Hi > > On 11/02/2017 12:49 PM, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Hi Linus, > > > > here's the latest series of patches that implement the tighter IRQ chip > > integration. I've dropped the banked infrastructure for now as per the > > discussion with Grygorii. > > > > The first couple of patches are mostly preparatory work in order to > > consolidate all IRQ chip related fields in a new structure and create > > the base functionality for adding IRQ chips. > > > > After that, I've added the Tegra186 GPIO support patch that makes use of > > the new tight integration. > > > > Changes in v6: > > - rebased on latest linux-gpio devel branch > > - one patch dropped due to rebase > > > > Changes in v5: > > - dropped the banked infrastructure patches for now (Grygorii) > > - allocate interrupts on demand, rather than upfront (Grygorii) > > - split up the first patch further as requested by Grygorii > > > > Not sure what happened in between here. Notes in commit logs indicate > > that this is actually version 5, but I can't find the cover letter for > > v3 and v4. > > > > Changes in v2: > > - rename pins to lines for consistent terminology > > - rename gpio_irq_chip_banked_handler() to > > gpio_irq_chip_banked_chained_handler() > > > > Sry, for delayed reply, very sorry. > > Patches 1 - 9, 11 : looks good, so > Acked-by: Grygorii Strashko <grygorii.strashko@ti.com> > > Patch 10 - unfortunately not all my comment were incorporated [1], > so below diff on top of patch 10 > which illustrates what i want and also converts gpio-omap to use new infra as > test for this new infra. > > Pls, take a look > > [1] https://www.spinics.net/lists/linux-tegra/msg31145.html Most of the changes I had deemed to be material for follow-up patches since they aren't immediately relevant to the gpio_irq_chip conversion nor needed by the Tegra186 patch. However, a couple of the hunks below seem like they should be part of the initial series. > -----------><------------- > From 210fe3ad7a642691a1b7603e441f6980b10ff2b4 Mon Sep 17 00:00:00 2001 > From: Grygorii Strashko <grygorii.strashko@ti.com> > Date: Fri, 3 Nov 2017 17:24:51 -0500 > Subject: [PATCH] [not for merge] gpiolib: update and test new gpioirq chip > infra > > changes in gpiolib: > - move set_parent to gpiochip_irq_map() and use parents instead of map for only > one parent > - add gpio_irq_chip->first_irq for static IRQ allocation > - fix nested = true if parent_handler not set > - fix gpiochip_irqchip_remove() if parent_handler not set > - get of_node from gpiodev > - fix lock_key: drivers do not need to set it, but looks a bit overcomplicated > as lock_class_key will be created for each gpiochip_add_data() call. > Honestly, do not seem better way :( > > GPIO OMAP driver updated to use new gpioirq chip infra and tested on am335x-evm. > seems it's working - can see irqs from keys. > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- > drivers/gpio/gpio-omap.c | 36 ++++++++++++++-------------- > drivers/gpio/gpiolib.c | 58 +++++++++++++++++++-------------------------- > include/linux/gpio/driver.h | 32 ++++++++++++++++++++++++- > 3 files changed, 73 insertions(+), 53 deletions(-) [...] > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c [...] > @@ -1642,6 +1644,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, > irq_hw_number_t hwirq) > { > struct gpio_chip *chip = d->host_data; > + int err = 0; > > if (!gpiochip_irqchip_irq_valid(chip, hwirq)) > return -ENXIO; > @@ -1658,6 +1661,12 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, > irq_set_nested_thread(irq, 1); > irq_set_noprobe(irq); > > + if (chip->irq.num_parents == 1) > + err = irq_set_parent(irq, chip->irq.parents[0]); > + else if (chip->irq.map) > + err = irq_set_parent(irq, chip->irq.map[hwirq]); > + if (err < 0) > + return err; Yeah, this looks sensible. Both in that this is a slightly better place to call it and that the handling for num_parents == 1 is necessary, too. > /* > * No set-up of the hardware will happen if IRQ_TYPE_NONE > * is passed as default type. > @@ -1712,30 +1721,18 @@ static void gpiochip_irq_relres(struct irq_data *d) > > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > { > - unsigned int irq; > - int err; > - > if (!gpiochip_irqchip_irq_valid(chip, offset)) > return -ENXIO; > > - irq = irq_create_mapping(chip->irq.domain, offset); > - if (!irq) > - return 0; > - > - if (chip->irq.map) { > - err = irq_set_parent(irq, chip->irq.map[offset]); > - if (err < 0) > - return err; > - } > - > - return irq; > + return irq_create_mapping(chip->irq.domain, offset); > } > > /** > * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip > * @gpiochip: the GPIO chip to add the IRQ chip to > */ > -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, > + struct lock_class_key *lock_key) > { > struct irq_chip *irqchip = gpiochip->irq.chip; > const struct irq_domain_ops *ops; > @@ -1753,17 +1750,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > } > > type = gpiochip->irq.default_type; > - np = gpiochip->parent->of_node; > - > -#ifdef CONFIG_OF_GPIO > - /* > - * If the gpiochip has an assigned OF node this takes precedence > - * FIXME: get rid of this and use gpiochip->parent->of_node > - * everywhere > - */ > - if (gpiochip->of_node) > - np = gpiochip->of_node; > -#endif > + /* called from gpiochip_add_data() and is set properly */ > + np = gpiochip->gpiodev->dev.of_node; Indeed, looks like we have this twice now. > > /* > * Specifying a default trigger is a terrible idea if DT or ACPI is > @@ -1789,7 +1777,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > ops = &gpiochip_domain_ops; > > gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, > - 0, ops, gpiochip); > + gpiochip->irq.first_irq, > + ops, gpiochip); > if (!gpiochip->irq.domain) > return -EINVAL; This seems backward. You just spent a lot of time getting rid of all users of first_irq (it's actually the reason why I dropped one of the patches from the series, since first_irq is no longer used), so why reintroduce it? > > @@ -1818,9 +1807,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > } > > gpiochip->irq.nested = false; > - } else { > - gpiochip->irq.nested = true; > } > + /* GPIO driver might not specify parent_handler, but it doesn't mean > + * it's irq_nested, as driver may use request_irq() */ That's certainly how irq_nested is used in gpiolib, though. Interrupts are considered either cascaded or nested. Maybe this needs clarification in general? > > acpi_gpiochip_request_interrupts(gpiochip); > > @@ -1839,7 +1828,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) > > acpi_gpiochip_free_interrupts(gpiochip); > > - if (gpiochip->irq.chip) { > + if (gpiochip->irq.chip && gpiochip->irq.parent_handler) { > struct gpio_irq_chip *irq = &gpiochip->irq; > unsigned int i; > Yeah, this seems like a good idea, though I think this is safe regardless of irq.parent_handler. > @@ -1972,7 +1961,8 @@ EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_key); > > #else /* CONFIG_GPIOLIB_IRQCHIP */ > > -static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > +static inline int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, > + struct lock_class_key *lock_key) > { > return 0; > } > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index 51fc7b0..3254996 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -128,6 +128,15 @@ struct gpio_irq_chip { > * in IRQ domain of the chip. > */ > unsigned long *valid_mask; > + > + /** > + * @first_irq: > + * > + * Required for static irq allocation. > + * if set irq_domain_add_simple() will allocate and map all IRQs > + * during initialization. > + */ > + unsigned int first_irq; Again, what was the point of removing all users of this if we need to add it again? It seems to me like dynamic allocation should be a prerequisite for drivers to use this new code, otherwise we'll just end up adding special cases to this otherwise generic code. > }; > > static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip) > @@ -312,8 +321,29 @@ struct gpio_chip { > extern const char *gpiochip_is_requested(struct gpio_chip *chip, > unsigned offset); > > +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data, > + struct lock_class_key *irq_lock_key); > +#ifdef CONFIG_LOCKDEP > +/* > + * Lockdep requires that each irqchip instance be created with a > + * unique key so as to avoid unnecessary warnings. This upfront > + * boilerplate static inlines provides such a key for each > + * unique instance which is created now from inside gpiochip_add_data_key(). > + */ > +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data) > +{ > + static struct lock_class_key key; > + > + return gpiochip_add_data_key(chip, data, key); > +} This looks like a neat improvement, but I think it can be done in a follow-up to remove the boilerplate in drivers. Thierry
On Sat, Nov 4, 2017 at 12:50 AM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > On 11/03/2017 05:50 PM, Linus Walleij wrote: >> I guess I will consider queueing them already for v4.15 if >> Torvalds churns out another -rc on sunday, I do not >> doubt that you will finish it as we have discussed with banks and >> all. If v4.14 is released on sunday let's queue this early for v4.16. > > Can you see my reply? > https://lkml.org/lkml/2017/11/3/820 Yes so I was considering queueing patches 1-9 with your ACK so Thierry can get his patch stack down and focus on the add-on stuff at the end of the series. Thierry, does it sound reasonable? Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 06, 2017 at 02:22:22PM +0100, Linus Walleij wrote: > On Sat, Nov 4, 2017 at 12:50 AM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > > On 11/03/2017 05:50 PM, Linus Walleij wrote: > > >> I guess I will consider queueing them already for v4.15 if > >> Torvalds churns out another -rc on sunday, I do not > >> doubt that you will finish it as we have discussed with banks and > >> all. If v4.14 is released on sunday let's queue this early for v4.16. > > > > Can you see my reply? > > https://lkml.org/lkml/2017/11/3/820 > > Yes so I was considering queueing patches 1-9 with your ACK so > Thierry can get his patch stack down and focus on the add-on > stuff at the end of the series. > > Thierry, does it sound reasonable? Sure. I'd still like to see Tegra186 support go into 4.15 to unblock a bunch of things. Like I mentioned in my reply to Grygorii earlier, the bigger part of the changes can be follow-on patches in my opinion since they become only relevant as other drivers are converted. I've been running with some changes proposed by Grygorii applied to my local tree and can send they can all be squashed into patch 10 to at least address most of Grygorii's comments. Should I send it as follow- up or do you want me to resend a v7 of 10-12 with those changes squashed in? Thierry
On 11/06/2017 05:18 AM, Thierry Reding wrote: > On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote: >> Hi >> >> On 11/02/2017 12:49 PM, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> Hi Linus, >>> >>> here's the latest series of patches that implement the tighter IRQ chip >>> integration. I've dropped the banked infrastructure for now as per the >>> discussion with Grygorii. >>> >>> The first couple of patches are mostly preparatory work in order to >>> consolidate all IRQ chip related fields in a new structure and create >>> the base functionality for adding IRQ chips. >>> >>> After that, I've added the Tegra186 GPIO support patch that makes use of >>> the new tight integration. >>> >>> Changes in v6: >>> - rebased on latest linux-gpio devel branch >>> - one patch dropped due to rebase >>> >>> Changes in v5: >>> - dropped the banked infrastructure patches for now (Grygorii) >>> - allocate interrupts on demand, rather than upfront (Grygorii) >>> - split up the first patch further as requested by Grygorii >>> >>> Not sure what happened in between here. Notes in commit logs indicate >>> that this is actually version 5, but I can't find the cover letter for >>> v3 and v4. >>> >>> Changes in v2: >>> - rename pins to lines for consistent terminology >>> - rename gpio_irq_chip_banked_handler() to >>> gpio_irq_chip_banked_chained_handler() >>> >> >> Sry, for delayed reply, very sorry. >> >> Patches 1 - 9, 11 : looks good, so >> Acked-by: Grygorii Strashko <grygorii.strashko@ti.com> >> >> Patch 10 - unfortunately not all my comment were incorporated [1], >> so below diff on top of patch 10 >> which illustrates what i want and also converts gpio-omap to use new infra as >> test for this new infra. >> >> Pls, take a look >> >> [1] https://www.spinics.net/lists/linux-tegra/msg31145.html > > Most of the changes I had deemed to be material for follow-up patches > since they aren't immediately relevant to the gpio_irq_chip conversion > nor needed by the Tegra186 patch. > > However, a couple of the hunks below seem like they should be part of > the initial series. > >> -----------><------------- >> From 210fe3ad7a642691a1b7603e441f6980b10ff2b4 Mon Sep 17 00:00:00 2001 >> From: Grygorii Strashko <grygorii.strashko@ti.com> >> Date: Fri, 3 Nov 2017 17:24:51 -0500 >> Subject: [PATCH] [not for merge] gpiolib: update and test new gpioirq chip >> infra >> >> changes in gpiolib: >> - move set_parent to gpiochip_irq_map() and use parents instead of map for only >> one parent >> - add gpio_irq_chip->first_irq for static IRQ allocation >> - fix nested = true if parent_handler not set >> - fix gpiochip_irqchip_remove() if parent_handler not set >> - get of_node from gpiodev >> - fix lock_key: drivers do not need to set it, but looks a bit overcomplicated >> as lock_class_key will be created for each gpiochip_add_data() call. >> Honestly, do not seem better way :( >> >> GPIO OMAP driver updated to use new gpioirq chip infra and tested on am335x-evm. >> seems it's working - can see irqs from keys. >> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- >> drivers/gpio/gpio-omap.c | 36 ++++++++++++++-------------- >> drivers/gpio/gpiolib.c | 58 +++++++++++++++++++-------------------------- >> include/linux/gpio/driver.h | 32 ++++++++++++++++++++++++- >> 3 files changed, 73 insertions(+), 53 deletions(-) > [...] >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > [...] >> @@ -1642,6 +1644,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, >> irq_hw_number_t hwirq) >> { >> struct gpio_chip *chip = d->host_data; >> + int err = 0; >> >> if (!gpiochip_irqchip_irq_valid(chip, hwirq)) >> return -ENXIO; >> @@ -1658,6 +1661,12 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, >> irq_set_nested_thread(irq, 1); >> irq_set_noprobe(irq); >> >> + if (chip->irq.num_parents == 1) >> + err = irq_set_parent(irq, chip->irq.parents[0]); >> + else if (chip->irq.map) >> + err = irq_set_parent(irq, chip->irq.map[hwirq]); >> + if (err < 0) >> + return err; > > Yeah, this looks sensible. Both in that this is a slightly better place > to call it and that the handling for num_parents == 1 is necessary, too. > >> /* >> * No set-up of the hardware will happen if IRQ_TYPE_NONE >> * is passed as default type. >> @@ -1712,30 +1721,18 @@ static void gpiochip_irq_relres(struct irq_data *d) >> >> static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) >> { >> - unsigned int irq; >> - int err; >> - >> if (!gpiochip_irqchip_irq_valid(chip, offset)) >> return -ENXIO; >> >> - irq = irq_create_mapping(chip->irq.domain, offset); >> - if (!irq) >> - return 0; >> - >> - if (chip->irq.map) { >> - err = irq_set_parent(irq, chip->irq.map[offset]); >> - if (err < 0) >> - return err; >> - } >> - >> - return irq; >> + return irq_create_mapping(chip->irq.domain, offset); >> } >> >> /** >> * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip >> * @gpiochip: the GPIO chip to add the IRQ chip to >> */ >> -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >> +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, >> + struct lock_class_key *lock_key) >> { >> struct irq_chip *irqchip = gpiochip->irq.chip; >> const struct irq_domain_ops *ops; >> @@ -1753,17 +1750,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >> } >> >> type = gpiochip->irq.default_type; >> - np = gpiochip->parent->of_node; >> - >> -#ifdef CONFIG_OF_GPIO >> - /* >> - * If the gpiochip has an assigned OF node this takes precedence >> - * FIXME: get rid of this and use gpiochip->parent->of_node >> - * everywhere >> - */ >> - if (gpiochip->of_node) >> - np = gpiochip->of_node; >> -#endif >> + /* called from gpiochip_add_data() and is set properly */ >> + np = gpiochip->gpiodev->dev.of_node; > > Indeed, looks like we have this twice now. > >> >> /* >> * Specifying a default trigger is a terrible idea if DT or ACPI is >> @@ -1789,7 +1777,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >> ops = &gpiochip_domain_ops; >> >> gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, >> - 0, ops, gpiochip); >> + gpiochip->irq.first_irq, >> + ops, gpiochip); >> if (!gpiochip->irq.domain) >> return -EINVAL; > > This seems backward. You just spent a lot of time getting rid of all > users of first_irq (it's actually the reason why I dropped one of the > patches from the series, since first_irq is no longer used), so why > reintroduce it? Yes. It required for HW/drivers not supported (or partially supported) SPARSE_IRQ. And it's not exactly the same as dropped base_irq. If SPARSE_IRQ not supported - driver should ensure that proper Linux IRQ range allocated (or reserved) and pass first_irq number to the gpiolib For example, in case of OMAP GPIO this is required for OMAP1 support and driver has code irq_base = devm_irq_alloc_descs(bank->chip.parent, -1, 0, bank->width, 0); irq_base makes no sense in case of SPARSE_IRQ compatible driver and therefore was removed from gpiolib to avoid mis-usage - drivers should maintain it by itself if requred. > >> >> @@ -1818,9 +1807,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >> } >> >> gpiochip->irq.nested = false; >> - } else { >> - gpiochip->irq.nested = true; >> } >> + /* GPIO driver might not specify parent_handler, but it doesn't mean >> + * it's irq_nested, as driver may use request_irq() */ > > That's certainly how irq_nested is used in gpiolib, though. Interrupts > are considered either cascaded or nested. Maybe this needs clarification > in general? No. Please, take closer look at gpiochip_set_nested_irqchip() gpiochip_set_cascaded_irqchip() gpiochip_set_chained_irqchip() gpiochip_set_nested_irqchip() and how they are used. Also, take a look on OMAP driver - it doesn't install chained handler. gpiolib now can discover automatically only when child irqs are not nested (nested here means - threaded nested), therefore different APIs were introduced gpiochip_set_chained_irqchip() gpiochip_set_nested_irqchip() So, with your change: - nested = false; can be set auto if parent_handler provided - nested = <set by driver> if no parent_handler provided because with this patch full GPIO IRQ configuration expected to be done from inside gpiochip_add_data(). > >> >> acpi_gpiochip_request_interrupts(gpiochip); >> >> @@ -1839,7 +1828,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) >> >> acpi_gpiochip_free_interrupts(gpiochip); >> >> - if (gpiochip->irq.chip) { >> + if (gpiochip->irq.chip && gpiochip->irq.parent_handler) { >> struct gpio_irq_chip *irq = &gpiochip->irq; >> unsigned int i; >> > > Yeah, this seems like a good idea, though I think this is safe > regardless of irq.parent_handler. > >> @@ -1972,7 +1961,8 @@ EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_key); >> >> #else /* CONFIG_GPIOLIB_IRQCHIP */ >> >> -static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >> +static inline int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, >> + struct lock_class_key *lock_key) >> { >> return 0; >> } >> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h >> index 51fc7b0..3254996 100644 >> --- a/include/linux/gpio/driver.h >> +++ b/include/linux/gpio/driver.h >> @@ -128,6 +128,15 @@ struct gpio_irq_chip { >> * in IRQ domain of the chip. >> */ >> unsigned long *valid_mask; >> + >> + /** >> + * @first_irq: >> + * >> + * Required for static irq allocation. >> + * if set irq_domain_add_simple() will allocate and map all IRQs >> + * during initialization. >> + */ >> + unsigned int first_irq; > > Again, what was the point of removing all users of this if we need to > add it again? > > It seems to me like dynamic allocation should be a prerequisite for > drivers to use this new code, otherwise we'll just end up adding special > cases to this otherwise generic code. The idea, as i see it, is to be able to re-use you change for as much drivers as possible (as I illustrated for OMAP) and in this case we need support backward compatibility with non SPARSE_IRQ compatible drivers. > >> }; >> >> static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip) >> @@ -312,8 +321,29 @@ struct gpio_chip { >> extern const char *gpiochip_is_requested(struct gpio_chip *chip, >> unsigned offset); >> >> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data, >> + struct *irq_lock_key); >> +#ifdef CONFIG_LOCKDEP >> +/* >> + * Lockdep requires that each irqchip instance be created with a >> + * unique key so as to avoid unnecessary warnings. This upfront >> + * boilerplate static inlines provides such a key for each >> + * unique instance which is created now from inside gpiochip_add_data_key(). >> + */ >> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data) >> +{ >> + static struct lock_class_key key; >> + >> + return gpiochip_add_data_key(chip, data, key); >> +} > > This looks like a neat improvement, but I think it can be done in a > follow-up to remove the boilerplate in drivers. Can't agree here - it better to be considered now. Now only two GPIO drivers define lock_class_key: ./drivers/gpio/gpio-bcm-kona.c:static struct lock_class_key gpio_lock_class; ./drivers/gpio/gpio-brcmstb.c:static struct lock_class_key brcmstb_gpio_irq_lock_class; and these drivers do not use gpioirq framework (your tegra driver will be the third). So, if proposed changes will be applied all drivers switched to use it will need to define its own lock_class_key again and it will be step back. Another question that solution I've proposed may be not ideal :(
On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote: > > > On 11/06/2017 05:18 AM, Thierry Reding wrote: > > On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote: > >> Hi > >> > >> On 11/02/2017 12:49 PM, Thierry Reding wrote: > >>> From: Thierry Reding <treding@nvidia.com> > >>> > >>> Hi Linus, > >>> > >>> here's the latest series of patches that implement the tighter IRQ chip > >>> integration. I've dropped the banked infrastructure for now as per the > >>> discussion with Grygorii. > >>> > >>> The first couple of patches are mostly preparatory work in order to > >>> consolidate all IRQ chip related fields in a new structure and create > >>> the base functionality for adding IRQ chips. > >>> > >>> After that, I've added the Tegra186 GPIO support patch that makes use of > >>> the new tight integration. > >>> > >>> Changes in v6: > >>> - rebased on latest linux-gpio devel branch > >>> - one patch dropped due to rebase > >>> > >>> Changes in v5: > >>> - dropped the banked infrastructure patches for now (Grygorii) > >>> - allocate interrupts on demand, rather than upfront (Grygorii) > >>> - split up the first patch further as requested by Grygorii > >>> > >>> Not sure what happened in between here. Notes in commit logs indicate > >>> that this is actually version 5, but I can't find the cover letter for > >>> v3 and v4. > >>> > >>> Changes in v2: > >>> - rename pins to lines for consistent terminology > >>> - rename gpio_irq_chip_banked_handler() to > >>> gpio_irq_chip_banked_chained_handler() > >>> > >> > >> Sry, for delayed reply, very sorry. > >> > >> Patches 1 - 9, 11 : looks good, so > >> Acked-by: Grygorii Strashko <grygorii.strashko@ti.com> > >> > >> Patch 10 - unfortunately not all my comment were incorporated [1], > >> so below diff on top of patch 10 > >> which illustrates what i want and also converts gpio-omap to use new infra as > >> test for this new infra. > >> > >> Pls, take a look > >> > >> [1] https://www.spinics.net/lists/linux-tegra/msg31145.html > > > > Most of the changes I had deemed to be material for follow-up patches > > since they aren't immediately relevant to the gpio_irq_chip conversion > > nor needed by the Tegra186 patch. > > > > However, a couple of the hunks below seem like they should be part of > > the initial series. > > > >> -----------><------------- > >> From 210fe3ad7a642691a1b7603e441f6980b10ff2b4 Mon Sep 17 00:00:00 2001 > >> From: Grygorii Strashko <grygorii.strashko@ti.com> > >> Date: Fri, 3 Nov 2017 17:24:51 -0500 > >> Subject: [PATCH] [not for merge] gpiolib: update and test new gpioirq chip > >> infra > >> > >> changes in gpiolib: > >> - move set_parent to gpiochip_irq_map() and use parents instead of map for only > >> one parent > >> - add gpio_irq_chip->first_irq for static IRQ allocation > >> - fix nested = true if parent_handler not set > >> - fix gpiochip_irqchip_remove() if parent_handler not set > >> - get of_node from gpiodev > >> - fix lock_key: drivers do not need to set it, but looks a bit overcomplicated > >> as lock_class_key will be created for each gpiochip_add_data() call. > >> Honestly, do not seem better way :( > >> > >> GPIO OMAP driver updated to use new gpioirq chip infra and tested on am335x-evm. > >> seems it's working - can see irqs from keys. > >> > >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > >> --- > >> drivers/gpio/gpio-omap.c | 36 ++++++++++++++-------------- > >> drivers/gpio/gpiolib.c | 58 +++++++++++++++++++-------------------------- > >> include/linux/gpio/driver.h | 32 ++++++++++++++++++++++++- > >> 3 files changed, 73 insertions(+), 53 deletions(-) > > [...] > >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > [...] > >> @@ -1642,6 +1644,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, > >> irq_hw_number_t hwirq) > >> { > >> struct gpio_chip *chip = d->host_data; > >> + int err = 0; > >> > >> if (!gpiochip_irqchip_irq_valid(chip, hwirq)) > >> return -ENXIO; > >> @@ -1658,6 +1661,12 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, > >> irq_set_nested_thread(irq, 1); > >> irq_set_noprobe(irq); > >> > >> + if (chip->irq.num_parents == 1) > >> + err = irq_set_parent(irq, chip->irq.parents[0]); > >> + else if (chip->irq.map) > >> + err = irq_set_parent(irq, chip->irq.map[hwirq]); > >> + if (err < 0) > >> + return err; > > > > Yeah, this looks sensible. Both in that this is a slightly better place > > to call it and that the handling for num_parents == 1 is necessary, too. > > > >> /* > >> * No set-up of the hardware will happen if IRQ_TYPE_NONE > >> * is passed as default type. > >> @@ -1712,30 +1721,18 @@ static void gpiochip_irq_relres(struct irq_data *d) > >> > >> static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > >> { > >> - unsigned int irq; > >> - int err; > >> - > >> if (!gpiochip_irqchip_irq_valid(chip, offset)) > >> return -ENXIO; > >> > >> - irq = irq_create_mapping(chip->irq.domain, offset); > >> - if (!irq) > >> - return 0; > >> - > >> - if (chip->irq.map) { > >> - err = irq_set_parent(irq, chip->irq.map[offset]); > >> - if (err < 0) > >> - return err; > >> - } > >> - > >> - return irq; > >> + return irq_create_mapping(chip->irq.domain, offset); > >> } > >> > >> /** > >> * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip > >> * @gpiochip: the GPIO chip to add the IRQ chip to > >> */ > >> -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > >> +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, > >> + struct lock_class_key *lock_key) > >> { > >> struct irq_chip *irqchip = gpiochip->irq.chip; > >> const struct irq_domain_ops *ops; > >> @@ -1753,17 +1750,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > >> } > >> > >> type = gpiochip->irq.default_type; > >> - np = gpiochip->parent->of_node; > >> - > >> -#ifdef CONFIG_OF_GPIO > >> - /* > >> - * If the gpiochip has an assigned OF node this takes precedence > >> - * FIXME: get rid of this and use gpiochip->parent->of_node > >> - * everywhere > >> - */ > >> - if (gpiochip->of_node) > >> - np = gpiochip->of_node; > >> -#endif > >> + /* called from gpiochip_add_data() and is set properly */ > >> + np = gpiochip->gpiodev->dev.of_node; > > > > Indeed, looks like we have this twice now. > > > >> > >> /* > >> * Specifying a default trigger is a terrible idea if DT or ACPI is > >> @@ -1789,7 +1777,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > >> ops = &gpiochip_domain_ops; > >> > >> gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, > >> - 0, ops, gpiochip); > >> + gpiochip->irq.first_irq, > >> + ops, gpiochip); > >> if (!gpiochip->irq.domain) > >> return -EINVAL; > > > > This seems backward. You just spent a lot of time getting rid of all > > users of first_irq (it's actually the reason why I dropped one of the > > patches from the series, since first_irq is no longer used), so why > > reintroduce it? > > Yes. It required for HW/drivers not supported (or partially supported) > SPARSE_IRQ. And it's not exactly the same as dropped base_irq. > If SPARSE_IRQ not supported - driver should ensure that proper > Linux IRQ range allocated (or reserved) and pass first_irq number to the gpiolib > For example, in case of OMAP GPIO this is required for OMAP1 support and > driver has code > irq_base = devm_irq_alloc_descs(bank->chip.parent, -1, 0, bank->width, 0); > > irq_base makes no sense in case of SPARSE_IRQ compatible driver and > therefore was removed from gpiolib to avoid mis-usage - drivers should > maintain it by itself if requred. But this is not something that is currently being used. I understand that you'll want to add this in order to support fixed IRQ numbers on OMAP, but I don't see why it would need to be part of this series. It will simply be dead code until the OMAP patches get merged. > >> @@ -1818,9 +1807,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > >> } > >> > >> gpiochip->irq.nested = false; > >> - } else { > >> - gpiochip->irq.nested = true; > >> } > >> + /* GPIO driver might not specify parent_handler, but it doesn't mean > >> + * it's irq_nested, as driver may use request_irq() */ > > > > That's certainly how irq_nested is used in gpiolib, though. Interrupts > > are considered either cascaded or nested. Maybe this needs clarification > > in general? > > No. Please, take closer look at > gpiochip_set_nested_irqchip() > gpiochip_set_cascaded_irqchip() > gpiochip_set_chained_irqchip() > gpiochip_set_nested_irqchip() > and how they are used. > Also, take a look on OMAP driver - it doesn't install chained handler. > > gpiolib now can discover automatically only when child irqs are not nested > (nested here means - threaded nested), therefore different APIs were introduced > gpiochip_set_chained_irqchip() > gpiochip_set_nested_irqchip() > > So, with your change: > - nested = false; can be set auto if parent_handler provided > - nested = <set by driver> if no parent_handler provided > because with this patch full GPIO IRQ configuration expected to be done > from inside gpiochip_add_data(). Sorry if I'm being dense. The only user-callable functions currently are gpiochip_set_chained_irqchip() and gpiochip_set_nested_irqchip(). Both internally call gpiochip_set_cascaded_irqchip() and the only difference is that the former passes a parent handler while the latter doesn't. So this seems to me like exactly the same thing that gpiochip_add_irqchip() does in my series. I think there's some ambiguity in how irq_nested is currently being used because it really means two things: a) not-chained and b) threaded. So I think what we want is to separate these. For a) I think it's pretty much solved because we have the parent handler that disambiguates. So instead of just setting irq.nested = false when a parent handler is available we should probably also WARN if the driver has set irq.nested = true and force it to false. Then, as you suggested, leave the else branch open to allow the driver to specify whether or not it uses threaded handlers so that gpiochip_irq_map() and gpiochip_irq_unmap() will know to call irq_set_nested_thread(). In this case, perhaps it would be better to rename irq.nested to irq.threaded. Then we no longer need to check/force irq.nested for chained IRQ chips and instead use only irq.threaded to have interrupts marked as nested/threaded. > >> > >> acpi_gpiochip_request_interrupts(gpiochip); > >> > >> @@ -1839,7 +1828,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) > >> > >> acpi_gpiochip_free_interrupts(gpiochip); > >> > >> - if (gpiochip->irq.chip) { > >> + if (gpiochip->irq.chip && gpiochip->irq.parent_handler) { > >> struct gpio_irq_chip *irq = &gpiochip->irq; > >> unsigned int i; > >> > > > > Yeah, this seems like a good idea, though I think this is safe > > regardless of irq.parent_handler. > > > >> @@ -1972,7 +1961,8 @@ EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_key); > >> > >> #else /* CONFIG_GPIOLIB_IRQCHIP */ > >> > >> -static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > >> +static inline int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, > >> + struct lock_class_key *lock_key) > >> { > >> return 0; > >> } > >> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > >> index 51fc7b0..3254996 100644 > >> --- a/include/linux/gpio/driver.h > >> +++ b/include/linux/gpio/driver.h > >> @@ -128,6 +128,15 @@ struct gpio_irq_chip { > >> * in IRQ domain of the chip. > >> */ > >> unsigned long *valid_mask; > >> + > >> + /** > >> + * @first_irq: > >> + * > >> + * Required for static irq allocation. > >> + * if set irq_domain_add_simple() will allocate and map all IRQs > >> + * during initialization. > >> + */ > >> + unsigned int first_irq; > > > > Again, what was the point of removing all users of this if we need to > > add it again? > > > > It seems to me like dynamic allocation should be a prerequisite for > > drivers to use this new code, otherwise we'll just end up adding special > > cases to this otherwise generic code. > > The idea, as i see it, is to be able to re-use you change for as much drivers > as possible (as I illustrated for OMAP) and in this case we need support backward > compatibility with non SPARSE_IRQ compatible drivers. Okay, understood. How about we focus on getting the current series merged and then we can add this hunk as a preparator patch for the OMAP conversion series? > > > >> }; > >> > >> static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip) > >> @@ -312,8 +321,29 @@ struct gpio_chip { > >> extern const char *gpiochip_is_requested(struct gpio_chip *chip, > >> unsigned offset); > >> > >> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data, > >> + struct *irq_lock_key); > >> +#ifdef CONFIG_LOCKDEP > >> +/* > >> + * Lockdep requires that each irqchip instance be created with a > >> + * unique key so as to avoid unnecessary warnings. This upfront > >> + * boilerplate static inlines provides such a key for each > >> + * unique instance which is created now from inside gpiochip_add_data_key(). > >> + */ > >> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data) > >> +{ > >> + static struct lock_class_key key; > >> + > >> + return gpiochip_add_data_key(chip, data, key); > >> +} > > > > This looks like a neat improvement, but I think it can be done in a > > follow-up to remove the boilerplate in drivers. > > Can't agree here - it better to be considered now. > Now only two GPIO drivers define lock_class_key: > ./drivers/gpio/gpio-bcm-kona.c:static struct lock_class_key gpio_lock_class; > ./drivers/gpio/gpio-brcmstb.c:static struct lock_class_key brcmstb_gpio_irq_lock_class; > > and these drivers do not use gpioirq framework (your tegra driver will be the third). > > So, if proposed changes will be applied all drivers switched to use it will need to define > its own lock_class_key again and it will be step back. I think this would be a minor, mostly mechanical refactoring to do as follow-up. But since you feel very strongly about it, I'll add that into the series. Thierry
On Tue, Nov 07, 2017 at 12:13:44PM +0100, Thierry Reding wrote: > On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote: > > On 11/06/2017 05:18 AM, Thierry Reding wrote: > > > On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote: [...] > > >> @@ -312,8 +321,29 @@ struct gpio_chip { > > >> extern const char *gpiochip_is_requested(struct gpio_chip *chip, > > >> unsigned offset); > > >> > > >> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data, > > >> + struct *irq_lock_key); > > >> +#ifdef CONFIG_LOCKDEP > > >> +/* > > >> + * Lockdep requires that each irqchip instance be created with a > > >> + * unique key so as to avoid unnecessary warnings. This upfront > > >> + * boilerplate static inlines provides such a key for each > > >> + * unique instance which is created now from inside gpiochip_add_data_key(). > > >> + */ > > >> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data) > > >> +{ > > >> + static struct lock_class_key key; > > >> + > > >> + return gpiochip_add_data_key(chip, data, key); > > >> +} > > > > > > This looks like a neat improvement, but I think it can be done in a > > > follow-up to remove the boilerplate in drivers. > > > > Can't agree here - it better to be considered now. > > Now only two GPIO drivers define lock_class_key: > > ./drivers/gpio/gpio-bcm-kona.c:static struct lock_class_key gpio_lock_class; > > ./drivers/gpio/gpio-brcmstb.c:static struct lock_class_key brcmstb_gpio_irq_lock_class; > > > > and these drivers do not use gpioirq framework (your tegra driver will be the third). > > > > So, if proposed changes will be applied all drivers switched to use it will need to define > > its own lock_class_key again and it will be step back. > > I think this would be a minor, mostly mechanical refactoring to do as > follow-up. But since you feel very strongly about it, I'll add that into > the series. After implementing this, I'm having second thoughts. We've got a bunch of drivers calling gpiochip_add_data() that never register an IRQ chip but which will each add a struct lock_class_key after this change, and it will never be used. Now, struct lock_class_key is only 8 bytes big, so maybe this isn't a big deal, but it still seems like a waste. Thierry
On 11/07/2017 05:52 AM, Thierry Reding wrote: > On Tue, Nov 07, 2017 at 12:13:44PM +0100, Thierry Reding wrote: >> On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote: >>> On 11/06/2017 05:18 AM, Thierry Reding wrote: >>>> On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote: > [...] >>>>> @@ -312,8 +321,29 @@ struct gpio_chip { >>>>> extern const char *gpiochip_is_requested(struct gpio_chip *chip, >>>>> unsigned offset); >>>>> >>>>> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data, >>>>> + struct *irq_lock_key); >>>>> +#ifdef CONFIG_LOCKDEP >>>>> +/* >>>>> + * Lockdep requires that each irqchip instance be created with a >>>>> + * unique key so as to avoid unnecessary warnings. This upfront >>>>> + * boilerplate static inlines provides such a key for each >>>>> + * unique instance which is created now from inside gpiochip_add_data_key(). >>>>> + */ >>>>> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data) >>>>> +{ >>>>> + static struct lock_class_key key; >>>>> + >>>>> + return gpiochip_add_data_key(chip, data, key); >>>>> +} >>>> >>>> This looks like a neat improvement, but I think it can be done in a >>>> follow-up to remove the boilerplate in drivers. >>> >>> Can't agree here - it better to be considered now. >>> Now only two GPIO drivers define lock_class_key: >>> ./drivers/gpio/gpio-bcm-kona.c:static struct lock_class_key gpio_lock_class; >>> ./drivers/gpio/gpio-brcmstb.c:static struct lock_class_key brcmstb_gpio_irq_lock_class; >>> >>> and these drivers do not use gpioirq framework (your tegra driver will be the third). >>> >>> So, if proposed changes will be applied all drivers switched to use it will need to define >>> its own lock_class_key again and it will be step back. >> >> I think this would be a minor, mostly mechanical refactoring to do as >> follow-up. But since you feel very strongly about it, I'll add that into >> the series. > > After implementing this, I'm having second thoughts. We've got a bunch > of drivers calling gpiochip_add_data() that never register an IRQ chip > but which will each add a struct lock_class_key after this change, and > it will never be used. Now, struct lock_class_key is only 8 bytes big, > so maybe this isn't a big deal, but it still seems like a waste. True. And this I've called my approach not ideal, but I do not see other way to do it :( - that's price to pay for gpioirq chip initialization integration in gpiochip_add_data() which limits APIs variation used by GPIO drivers. Any other opinions, thoughts?
On 11/07/2017 05:13 AM, Thierry Reding wrote: > On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote: >> >> >> On 11/06/2017 05:18 AM, Thierry Reding wrote: >>> On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote: >>>> Hi >>>> >>>> On 11/02/2017 12:49 PM, Thierry Reding wrote: >>>>> From: Thierry Reding <treding@nvidia.com> >>>>> >>>>> Hi Linus, >>>>> >>>>> here's the latest series of patches that implement the tighter IRQ chip >>>>> integration. I've dropped the banked infrastructure for now as per the >>>>> discussion with Grygorii. >>>>> >>>>> The first couple of patches are mostly preparatory work in order to >>>>> consolidate all IRQ chip related fields in a new structure and create >>>>> the base functionality for adding IRQ chips. >>>>> >>>>> After that, I've added the Tegra186 GPIO support patch that makes use of >>>>> the new tight integration. >>>>> >>>>> Changes in v6: >>>>> - rebased on latest linux-gpio devel branch >>>>> - one patch dropped due to rebase >>>>> >>>>> Changes in v5: >>>>> - dropped the banked infrastructure patches for now (Grygorii) >>>>> - allocate interrupts on demand, rather than upfront (Grygorii) >>>>> - split up the first patch further as requested by Grygorii >>>>> >>>>> Not sure what happened in between here. Notes in commit logs indicate >>>>> that this is actually version 5, but I can't find the cover letter for >>>>> v3 and v4. >>>>> >>>>> Changes in v2: >>>>> - rename pins to lines for consistent terminology >>>>> - rename gpio_irq_chip_banked_handler() to >>>>> gpio_irq_chip_banked_chained_handler() >>>>> >>>> >>>> Sry, for delayed reply, very sorry. >>>> >>>> Patches 1 - 9, 11 : looks good, so >>>> Acked-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> >>>> Patch 10 - unfortunately not all my comment were incorporated [1], >>>> so below diff on top of patch 10 >>>> which illustrates what i want and also converts gpio-omap to use new infra as >>>> test for this new infra. >>>> >>>> Pls, take a look >>>> >>>> [1] https://www.spinics.net/lists/linux-tegra/msg31145.html >>> >>> Most of the changes I had deemed to be material for follow-up patches >>> since they aren't immediately relevant to the gpio_irq_chip conversion >>> nor needed by the Tegra186 patch. >>> >>> However, a couple of the hunks below seem like they should be part of >>> the initial series. >>> >>>> -----------><------------- >>>> From 210fe3ad7a642691a1b7603e441f6980b10ff2b4 Mon Sep 17 00:00:00 2001 >>>> From: Grygorii Strashko <grygorii.strashko@ti.com> >>>> Date: Fri, 3 Nov 2017 17:24:51 -0500 >>>> Subject: [PATCH] [not for merge] gpiolib: update and test new gpioirq chip >>>> infra >>>> >>>> changes in gpiolib: >>>> - move set_parent to gpiochip_irq_map() and use parents instead of map for only >>>> one parent >>>> - add gpio_irq_chip->first_irq for static IRQ allocation >>>> - fix nested = true if parent_handler not set >>>> - fix gpiochip_irqchip_remove() if parent_handler not set >>>> - get of_node from gpiodev >>>> - fix lock_key: drivers do not need to set it, but looks a bit overcomplicated >>>> as lock_class_key will be created for each gpiochip_add_data() call. >>>> Honestly, do not seem better way :( >>>> >>>> GPIO OMAP driver updated to use new gpioirq chip infra and tested on am335x-evm. >>>> seems it's working - can see irqs from keys. >>>> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> --- >>>> drivers/gpio/gpio-omap.c | 36 ++++++++++++++-------------- >>>> drivers/gpio/gpiolib.c | 58 +++++++++++++++++++-------------------------- >>>> include/linux/gpio/driver.h | 32 ++++++++++++++++++++++++- >>>> 3 files changed, 73 insertions(+), 53 deletions(-) >>> [...] >>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >>> [...] >>>> @@ -1642,6 +1644,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, >>>> irq_hw_number_t hwirq) >>>> { >>>> struct gpio_chip *chip = d->host_data; >>>> + int err = 0; >>>> >>>> if (!gpiochip_irqchip_irq_valid(chip, hwirq)) >>>> return -ENXIO; >>>> @@ -1658,6 +1661,12 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, >>>> irq_set_nested_thread(irq, 1); >>>> irq_set_noprobe(irq); >>>> >>>> + if (chip->irq.num_parents == 1) >>>> + err = irq_set_parent(irq, chip->irq.parents[0]); >>>> + else if (chip->irq.map) >>>> + err = irq_set_parent(irq, chip->irq.map[hwirq]); >>>> + if (err < 0) >>>> + return err; >>> >>> Yeah, this looks sensible. Both in that this is a slightly better place >>> to call it and that the handling for num_parents == 1 is necessary, too. >>> >>>> /* >>>> * No set-up of the hardware will happen if IRQ_TYPE_NONE >>>> * is passed as default type. >>>> @@ -1712,30 +1721,18 @@ static void gpiochip_irq_relres(struct irq_data *d) >>>> >>>> static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) >>>> { >>>> - unsigned int irq; >>>> - int err; >>>> - >>>> if (!gpiochip_irqchip_irq_valid(chip, offset)) >>>> return -ENXIO; >>>> >>>> - irq = irq_create_mapping(chip->irq.domain, offset); >>>> - if (!irq) >>>> - return 0; >>>> - >>>> - if (chip->irq.map) { >>>> - err = irq_set_parent(irq, chip->irq.map[offset]); >>>> - if (err < 0) >>>> - return err; >>>> - } >>>> - >>>> - return irq; >>>> + return irq_create_mapping(chip->irq.domain, offset); >>>> } >>>> >>>> /** >>>> * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip >>>> * @gpiochip: the GPIO chip to add the IRQ chip to >>>> */ >>>> -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >>>> +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, >>>> + struct lock_class_key *lock_key) >>>> { >>>> struct irq_chip *irqchip = gpiochip->irq.chip; >>>> const struct irq_domain_ops *ops; >>>> @@ -1753,17 +1750,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >>>> } >>>> >>>> type = gpiochip->irq.default_type; >>>> - np = gpiochip->parent->of_node; >>>> - >>>> -#ifdef CONFIG_OF_GPIO >>>> - /* >>>> - * If the gpiochip has an assigned OF node this takes precedence >>>> - * FIXME: get rid of this and use gpiochip->parent->of_node >>>> - * everywhere >>>> - */ >>>> - if (gpiochip->of_node) >>>> - np = gpiochip->of_node; >>>> -#endif >>>> + /* called from gpiochip_add_data() and is set properly */ >>>> + np = gpiochip->gpiodev->dev.of_node; >>> >>> Indeed, looks like we have this twice now. >>> >>>> >>>> /* >>>> * Specifying a default trigger is a terrible idea if DT or ACPI is >>>> @@ -1789,7 +1777,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >>>> ops = &gpiochip_domain_ops; >>>> >>>> gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, >>>> - 0, ops, gpiochip); >>>> + gpiochip->irq.first_irq, >>>> + ops, gpiochip); >>>> if (!gpiochip->irq.domain) >>>> return -EINVAL; >>> >>> This seems backward. You just spent a lot of time getting rid of all >>> users of first_irq (it's actually the reason why I dropped one of the >>> patches from the series, since first_irq is no longer used), so why >>> reintroduce it? >> >> Yes. It required for HW/drivers not supported (or partially supported) >> SPARSE_IRQ. And it's not exactly the same as dropped base_irq. >> If SPARSE_IRQ not supported - driver should ensure that proper >> Linux IRQ range allocated (or reserved) and pass first_irq number to the gpiolib >> For example, in case of OMAP GPIO this is required for OMAP1 support and >> driver has code >> irq_base = devm_irq_alloc_descs(bank->chip.parent, -1, 0, bank->width, 0); >> >> irq_base makes no sense in case of SPARSE_IRQ compatible driver and >> therefore was removed from gpiolib to avoid mis-usage - drivers should >> maintain it by itself if requred. > > But this is not something that is currently being used. I understand > that you'll want to add this in order to support fixed IRQ numbers on > OMAP, but I don't see why it would need to be part of this series. It > will simply be dead code until the OMAP patches get merged. My opinion is that if this modification will be merged in gpiolib - drivers should be able to use it out of the box. Any way finial decision is up to Linus. > >>>> @@ -1818,9 +1807,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >>>> } >>>> >>>> gpiochip->irq.nested = false; >>>> - } else { >>>> - gpiochip->irq.nested = true; >>>> } >>>> + /* GPIO driver might not specify parent_handler, but it doesn't mean >>>> + * it's irq_nested, as driver may use request_irq() */ >>> >>> That's certainly how irq_nested is used in gpiolib, though. Interrupts >>> are considered either cascaded or nested. Maybe this needs clarification >>> in general? >> >> No. Please, take closer look at >> gpiochip_set_nested_irqchip() >> gpiochip_set_cascaded_irqchip() >> gpiochip_set_chained_irqchip() >> gpiochip_set_nested_irqchip() Sry, above is copy-paste err: gpiochip_set_chained_irqchip gpiochip_set_nested_irqchip gpiochip_irqchip_add gpiochip_irqchip_add_nested >> and how they are used. >> Also, take a look on OMAP driver - it doesn't install chained handler. >> >> gpiolib now can discover automatically only when child irqs are not nested >> (nested here means - threaded nested), therefore different APIs were introduced >> gpiochip_set_chained_irqchip() >> gpiochip_set_nested_irqchip() >> >> So, with your change: >> - nested = false; can be set auto if parent_handler provided >> - nested = <set by driver> if no parent_handler provided >> because with this patch full GPIO IRQ configuration expected to be done >> from inside gpiochip_add_data(). > > Sorry if I'm being dense. The only user-callable functions currently are > gpiochip_set_chained_irqchip() and gpiochip_set_nested_irqchip(). Both > internally call gpiochip_set_cascaded_irqchip() and the only difference > is that the former passes a parent handler while the latter doesn't. So > this seems to me like exactly the same thing that gpiochip_add_irqchip() > does in my series. > > I think there's some ambiguity in how irq_nested is currently being used > because it really means two things: a) not-chained and b) threaded. So I > think what we want is to separate these. For a) I think it's pretty much > solved because we have the parent handler that disambiguates. So instead > of just setting irq.nested = false when a parent handler is available we > should probably also WARN if the driver has set irq.nested = true and > force it to false. good point. > > Then, as you suggested, leave the else branch open to allow the driver > to specify whether or not it uses threaded handlers so that > gpiochip_irq_map() and gpiochip_irq_unmap() will know to call > irq_set_nested_thread(). correct > > In this case, perhaps it would be better to rename irq.nested to > irq.threaded. Then we no longer need to check/force irq.nested for > chained IRQ chips and instead use only irq.threaded to have interrupts > marked as nested/threaded. agree > >>>> >>>> acpi_gpiochip_request_interrupts(gpiochip); >>>> >>>> @@ -1839,7 +1828,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) >>>> >>>> acpi_gpiochip_free_interrupts(gpiochip); >>>> >>>> - if (gpiochip->irq.chip) { >>>> + if (gpiochip->irq.chip && gpiochip->irq.parent_handler) { >>>> struct gpio_irq_chip *irq = &gpiochip->irq; >>>> unsigned int i; >>>> >>> >>> Yeah, this seems like a good idea, though I think this is safe >>> regardless of irq.parent_handler. >>> >>>> @@ -1972,7 +1961,8 @@ EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_key); >>>> >>>> #else /* CONFIG_GPIOLIB_IRQCHIP */ >>>> >>>> -static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip) >>>> +static inline int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, >>>> + struct lock_class_key *lock_key) >>>> { >>>> return 0; >>>> } >>>> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h >>>> index 51fc7b0..3254996 100644 >>>> --- a/include/linux/gpio/driver.h >>>> +++ b/include/linux/gpio/driver.h >>>> @@ -128,6 +128,15 @@ struct gpio_irq_chip { >>>> * in IRQ domain of the chip. >>>> */ >>>> unsigned long *valid_mask; >>>> + >>>> + /** >>>> + * @first_irq: >>>> + * >>>> + * Required for static irq allocation. >>>> + * if set irq_domain_add_simple() will allocate and map all IRQs >>>> + * during initialization. >>>> + */ >>>> + unsigned int first_irq; >>> >>> Again, what was the point of removing all users of this if we need to >>> add it again? >>> >>> It seems to me like dynamic allocation should be a prerequisite for >>> drivers to use this new code, otherwise we'll just end up adding special >>> cases to this otherwise generic code. >> >> The idea, as i see it, is to be able to re-use you change for as much drivers >> as possible (as I illustrated for OMAP) and in this case we need support backward >> compatibility with non SPARSE_IRQ compatible drivers. > > Okay, understood. How about we focus on getting the current series > merged and then we can add this hunk as a preparator patch for the OMAP > conversion series? as above - finial decision is up to Linus. > >>> >>>> }; >>>> >>>> static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip) >>>> @@ -312,8 +321,29 @@ struct gpio_chip { >>>> extern const char *gpiochip_is_requested(struct gpio_chip *chip, >>>> unsigned offset); >>>> >>>> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data, >>>> + struct *irq_lock_key); >>>> +#ifdef CONFIG_LOCKDEP >>>> +/* >>>> + * Lockdep requires that each irqchip instance be created with a >>>> + * unique key so as to avoid unnecessary warnings. This upfront >>>> + * boilerplate static inlines provides such a key for each >>>> + * unique instance which is created now from inside gpiochip_add_data_key(). >>>> + */ >>>> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data) >>>> +{ >>>> + static struct lock_class_key key; >>>> + >>>> + return gpiochip_add_data_key(chip, data, key); >>>> +} >>> >>> This looks like a neat improvement, but I think it can be done in a >>> follow-up to remove the boilerplate in drivers. >> >> Can't agree here - it better to be considered now. >> Now only two GPIO drivers define lock_class_key: >> ./drivers/gpio/gpio-bcm-kona.c:static struct lock_class_key gpio_lock_class; >> ./drivers/gpio/gpio-brcmstb.c:static struct lock_class_key brcmstb_gpio_irq_lock_class; >> >> and these drivers do not use gpioirq framework (your tegra driver will be the third). >> >> So, if proposed changes will be applied all drivers switched to use it will need to define >> its own lock_class_key again and it will be step back. > > I think this would be a minor, mostly mechanical refactoring to do as > follow-up. But since you feel very strongly about it, I'll add that into > the series.
On Tue, Nov 07, 2017 at 11:00:41AM -0600, Grygorii Strashko wrote: > > > On 11/07/2017 05:13 AM, Thierry Reding wrote: > > On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote: > > > > > > > > > On 11/06/2017 05:18 AM, Thierry Reding wrote: > > > > On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote: > > > > > Hi > > > > > > > > > > On 11/02/2017 12:49 PM, Thierry Reding wrote: > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > > > > > Hi Linus, > > > > > > > > > > > > here's the latest series of patches that implement the tighter IRQ chip > > > > > > integration. I've dropped the banked infrastructure for now as per the > > > > > > discussion with Grygorii. > > > > > > > > > > > > The first couple of patches are mostly preparatory work in order to > > > > > > consolidate all IRQ chip related fields in a new structure and create > > > > > > the base functionality for adding IRQ chips. > > > > > > > > > > > > After that, I've added the Tegra186 GPIO support patch that makes use of > > > > > > the new tight integration. > > > > > > > > > > > > Changes in v6: > > > > > > - rebased on latest linux-gpio devel branch > > > > > > - one patch dropped due to rebase > > > > > > > > > > > > Changes in v5: > > > > > > - dropped the banked infrastructure patches for now (Grygorii) > > > > > > - allocate interrupts on demand, rather than upfront (Grygorii) > > > > > > - split up the first patch further as requested by Grygorii > > > > > > > > > > > > Not sure what happened in between here. Notes in commit logs indicate > > > > > > that this is actually version 5, but I can't find the cover letter for > > > > > > v3 and v4. > > > > > > > > > > > > Changes in v2: > > > > > > - rename pins to lines for consistent terminology > > > > > > - rename gpio_irq_chip_banked_handler() to > > > > > > gpio_irq_chip_banked_chained_handler() > > > > > > > > > > > > > > > > Sry, for delayed reply, very sorry. > > > > > > > > > > Patches 1 - 9, 11 : looks good, so > > > > > Acked-by: Grygorii Strashko <grygorii.strashko@ti.com> > > > > > > > > > > Patch 10 - unfortunately not all my comment were incorporated [1], > > > > > so below diff on top of patch 10 > > > > > which illustrates what i want and also converts gpio-omap to use new infra as > > > > > test for this new infra. > > > > > > > > > > Pls, take a look > > > > > > > > > > [1] https://www.spinics.net/lists/linux-tegra/msg31145.html > > > > > > > > Most of the changes I had deemed to be material for follow-up patches > > > > since they aren't immediately relevant to the gpio_irq_chip conversion > > > > nor needed by the Tegra186 patch. > > > > > > > > However, a couple of the hunks below seem like they should be part of > > > > the initial series. > > > > > > > > > -----------><------------- > > > > > From 210fe3ad7a642691a1b7603e441f6980b10ff2b4 Mon Sep 17 00:00:00 2001 > > > > > From: Grygorii Strashko <grygorii.strashko@ti.com> > > > > > Date: Fri, 3 Nov 2017 17:24:51 -0500 > > > > > Subject: [PATCH] [not for merge] gpiolib: update and test new gpioirq chip > > > > > infra > > > > > > > > > > changes in gpiolib: > > > > > - move set_parent to gpiochip_irq_map() and use parents instead of map for only > > > > > one parent > > > > > - add gpio_irq_chip->first_irq for static IRQ allocation > > > > > - fix nested = true if parent_handler not set > > > > > - fix gpiochip_irqchip_remove() if parent_handler not set > > > > > - get of_node from gpiodev > > > > > - fix lock_key: drivers do not need to set it, but looks a bit overcomplicated > > > > > as lock_class_key will be created for each gpiochip_add_data() call. > > > > > Honestly, do not seem better way :( > > > > > > > > > > GPIO OMAP driver updated to use new gpioirq chip infra and tested on am335x-evm. > > > > > seems it's working - can see irqs from keys. > > > > > > > > > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > > > > --- > > > > > drivers/gpio/gpio-omap.c | 36 ++++++++++++++-------------- > > > > > drivers/gpio/gpiolib.c | 58 +++++++++++++++++++-------------------------- > > > > > include/linux/gpio/driver.h | 32 ++++++++++++++++++++++++- > > > > > 3 files changed, 73 insertions(+), 53 deletions(-) > > > > [...] > > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > > > [...] > > > > > @@ -1642,6 +1644,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, > > > > > irq_hw_number_t hwirq) > > > > > { > > > > > struct gpio_chip *chip = d->host_data; > > > > > + int err = 0; > > > > > if (!gpiochip_irqchip_irq_valid(chip, hwirq)) > > > > > return -ENXIO; > > > > > @@ -1658,6 +1661,12 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq, > > > > > irq_set_nested_thread(irq, 1); > > > > > irq_set_noprobe(irq); > > > > > + if (chip->irq.num_parents == 1) > > > > > + err = irq_set_parent(irq, chip->irq.parents[0]); > > > > > + else if (chip->irq.map) > > > > > + err = irq_set_parent(irq, chip->irq.map[hwirq]); > > > > > + if (err < 0) > > > > > + return err; > > > > > > > > Yeah, this looks sensible. Both in that this is a slightly better place > > > > to call it and that the handling for num_parents == 1 is necessary, too. > > > > > > > > > /* > > > > > * No set-up of the hardware will happen if IRQ_TYPE_NONE > > > > > * is passed as default type. > > > > > @@ -1712,30 +1721,18 @@ static void gpiochip_irq_relres(struct irq_data *d) > > > > > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > > > > > { > > > > > - unsigned int irq; > > > > > - int err; > > > > > - > > > > > if (!gpiochip_irqchip_irq_valid(chip, offset)) > > > > > return -ENXIO; > > > > > - irq = irq_create_mapping(chip->irq.domain, offset); > > > > > - if (!irq) > > > > > - return 0; > > > > > - > > > > > - if (chip->irq.map) { > > > > > - err = irq_set_parent(irq, chip->irq.map[offset]); > > > > > - if (err < 0) > > > > > - return err; > > > > > - } > > > > > - > > > > > - return irq; > > > > > + return irq_create_mapping(chip->irq.domain, offset); > > > > > } > > > > > /** > > > > > * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip > > > > > * @gpiochip: the GPIO chip to add the IRQ chip to > > > > > */ > > > > > -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > > > > > +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip, > > > > > + struct lock_class_key *lock_key) > > > > > { > > > > > struct irq_chip *irqchip = gpiochip->irq.chip; > > > > > const struct irq_domain_ops *ops; > > > > > @@ -1753,17 +1750,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > > > > > } > > > > > type = gpiochip->irq.default_type; > > > > > - np = gpiochip->parent->of_node; > > > > > - > > > > > -#ifdef CONFIG_OF_GPIO > > > > > - /* > > > > > - * If the gpiochip has an assigned OF node this takes precedence > > > > > - * FIXME: get rid of this and use gpiochip->parent->of_node > > > > > - * everywhere > > > > > - */ > > > > > - if (gpiochip->of_node) > > > > > - np = gpiochip->of_node; > > > > > -#endif > > > > > + /* called from gpiochip_add_data() and is set properly */ > > > > > + np = gpiochip->gpiodev->dev.of_node; > > > > > > > > Indeed, looks like we have this twice now. > > > > > > > > > /* > > > > > * Specifying a default trigger is a terrible idea if DT or ACPI is > > > > > @@ -1789,7 +1777,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip) > > > > > ops = &gpiochip_domain_ops; > > > > > gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, > > > > > - 0, ops, gpiochip); > > > > > + gpiochip->irq.first_irq, > > > > > + ops, gpiochip); > > > > > if (!gpiochip->irq.domain) > > > > > return -EINVAL; > > > > > > > > This seems backward. You just spent a lot of time getting rid of all > > > > users of first_irq (it's actually the reason why I dropped one of the > > > > patches from the series, since first_irq is no longer used), so why > > > > reintroduce it? > > > > > > Yes. It required for HW/drivers not supported (or partially supported) > > > SPARSE_IRQ. And it's not exactly the same as dropped base_irq. > > > If SPARSE_IRQ not supported - driver should ensure that proper > > > Linux IRQ range allocated (or reserved) and pass first_irq number to the gpiolib > > > For example, in case of OMAP GPIO this is required for OMAP1 support and > > > driver has code > > > irq_base = devm_irq_alloc_descs(bank->chip.parent, -1, 0, bank->width, 0); > > > > > > irq_base makes no sense in case of SPARSE_IRQ compatible driver and > > > therefore was removed from gpiolib to avoid mis-usage - drivers should > > > maintain it by itself if requred. > > > > But this is not something that is currently being used. I understand > > that you'll want to add this in order to support fixed IRQ numbers on > > OMAP, but I don't see why it would need to be part of this series. It > > will simply be dead code until the OMAP patches get merged. > > My opinion is that if this modification will be merged in gpiolib - drivers > should be able to use it out of the box. > > Any way finial decision is up to Linus. Alright, I've sent v7 that should address all of the remaining issues. I've squashed a couple of the fixes you had pointed out into the tight integration patch and added another three patches on top for the first IRQ support, nested vs. threaded disambiguation and one for the automatic lock dep key creation. Thierry
From: Thierry Reding <treding@nvidia.com> Hi Linus, here's the latest series of patches that implement the tighter IRQ chip integration. I've dropped the banked infrastructure for now as per the discussion with Grygorii. The first couple of patches are mostly preparatory work in order to consolidate all IRQ chip related fields in a new structure and create the base functionality for adding IRQ chips. After that, I've added the Tegra186 GPIO support patch that makes use of the new tight integration. Changes in v6: - rebased on latest linux-gpio devel branch - one patch dropped due to rebase Changes in v5: - dropped the banked infrastructure patches for now (Grygorii) - allocate interrupts on demand, rather than upfront (Grygorii) - split up the first patch further as requested by Grygorii Not sure what happened in between here. Notes in commit logs indicate that this is actually version 5, but I can't find the cover letter for v3 and v4. Changes in v2: - rename pins to lines for consistent terminology - rename gpio_irq_chip_banked_handler() to gpio_irq_chip_banked_chained_handler() Thierry *** BLURB HERE *** Thierry Reding (12): gpio: Introduce struct gpio_irq_chip gpio: Move irqchip into struct gpio_irq_chip gpio: Move irqdomain into struct gpio_irq_chip gpio: Move irq_handler to struct gpio_irq_chip gpio: Move irq_default_type to struct gpio_irq_chip gpio: Move irq_chained_parent to struct gpio_irq_chip gpio: Move irq_nested into struct gpio_irq_chip gpio: Move irq_valid_mask into struct gpio_irq_chip gpio: Move lock_key into struct gpio_irq_chip gpio: Implement tighter IRQ chip integration gpio: Export gpiochip_irq_{map,unmap}() gpio: Add Tegra186 support Documentation/gpio/driver.txt | 6 +- drivers/bcma/driver_gpio.c | 2 +- drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-104-dio-48e.c | 2 +- drivers/gpio/gpio-104-idi-48.c | 2 +- drivers/gpio/gpio-104-idio-16.c | 2 +- drivers/gpio/gpio-adnp.c | 2 +- drivers/gpio/gpio-altera.c | 4 +- drivers/gpio/gpio-aspeed.c | 6 +- drivers/gpio/gpio-ath79.c | 2 +- drivers/gpio/gpio-crystalcove.c | 2 +- drivers/gpio/gpio-dln2.c | 2 +- drivers/gpio/gpio-ftgpio010.c | 2 +- drivers/gpio/gpio-ingenic.c | 2 +- drivers/gpio/gpio-intel-mid.c | 2 +- drivers/gpio/gpio-lynxpoint.c | 2 +- drivers/gpio/gpio-max732x.c | 2 +- drivers/gpio/gpio-merrifield.c | 2 +- drivers/gpio/gpio-omap.c | 2 +- drivers/gpio/gpio-pca953x.c | 2 +- drivers/gpio/gpio-pcf857x.c | 2 +- drivers/gpio/gpio-pci-idio-16.c | 2 +- drivers/gpio/gpio-pl061.c | 2 +- drivers/gpio/gpio-rcar.c | 2 +- drivers/gpio/gpio-reg.c | 4 +- drivers/gpio/gpio-stmpe.c | 6 +- drivers/gpio/gpio-tc3589x.c | 2 +- drivers/gpio/gpio-tegra186.c | 623 ++++++++++++++++++++++++++++ drivers/gpio/gpio-vf610.c | 2 +- drivers/gpio/gpio-wcove.c | 2 +- drivers/gpio/gpio-ws16c48.c | 2 +- drivers/gpio/gpio-xgene-sb.c | 2 +- drivers/gpio/gpio-xlp.c | 2 +- drivers/gpio/gpio-zx.c | 2 +- drivers/gpio/gpio-zynq.c | 2 +- drivers/gpio/gpiolib.c | 213 ++++++++-- drivers/pinctrl/bcm/pinctrl-bcm2835.c | 4 +- drivers/pinctrl/bcm/pinctrl-iproc-gpio.c | 2 +- drivers/pinctrl/intel/pinctrl-baytrail.c | 6 +- drivers/pinctrl/intel/pinctrl-cherryview.c | 6 +- drivers/pinctrl/intel/pinctrl-intel.c | 2 +- drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 2 +- drivers/pinctrl/nomadik/pinctrl-nomadik.c | 4 +- drivers/pinctrl/pinctrl-amd.c | 2 +- drivers/pinctrl/pinctrl-at91.c | 2 +- drivers/pinctrl/pinctrl-coh901.c | 2 +- drivers/pinctrl/pinctrl-mcp23s08.c | 2 +- drivers/pinctrl/pinctrl-oxnas.c | 2 +- drivers/pinctrl/pinctrl-pic32.c | 2 +- drivers/pinctrl/pinctrl-pistachio.c | 2 +- drivers/pinctrl/pinctrl-st.c | 2 +- drivers/pinctrl/pinctrl-sx150x.c | 2 +- drivers/pinctrl/qcom/pinctrl-msm.c | 2 +- drivers/pinctrl/sirf/pinctrl-atlas7.c | 2 +- drivers/pinctrl/sirf/pinctrl-sirf.c | 2 +- drivers/pinctrl/spear/pinctrl-plgpio.c | 2 +- drivers/platform/x86/intel_int0002_vgpio.c | 6 +- include/linux/gpio/driver.h | 154 +++++-- 59 files changed, 1004 insertions(+), 136 deletions(-) create mode 100644 drivers/gpio/gpio-tegra186.c