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@xxxxxxxxxx> > > > > 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@xxxxxx> > > 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@xxxxxx> > 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@xxxxxx> > --- > 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
Attachment:
signature.asc
Description: PGP signature
