| Message ID | 8f580295-9742-0896-7a11-346f8f74e95e@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] gpiolib: chain irq_request/release_resources() and irq_dis/enable() | expand |
Ping! Regards, Hans On 07/23/2018 05:03 PM, Hans Verkuil wrote: > Hi all, > > Here is yet another attempt to allow drivers to disable the irq and drive > the gpio as an output. > > Please be gentle with me: I am neither an expert on the gpio internals, nor on > the irq internals. > > This patch lets gpiolib override the irq_chip's irq_request/release_resources and > irq_en/disable hooks. > > The old hooks are stored and called by gpiolib. > > As a result, the gpiochip_(un)lock_as_irq functions can become static (not yet > implemented in this patch) and these calls should be removed from all drivers. > > I haven't done that since I first want to know if what I am doing here even > makes sense. > > Reviewing the removal of those calls in drivers should be fairly easy. > > Regards, > > Hans > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/gpio/gpiolib.c | 78 +++++++++++++++++++++++-------------- > include/linux/gpio/driver.h | 5 +++ > 2 files changed, 53 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index e11a3bb03820..20429197756f 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1800,28 +1800,48 @@ static const struct irq_domain_ops gpiochip_domain_ops = { > static int gpiochip_irq_reqres(struct irq_data *d) > { > struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + int res = 0; > > if (!try_module_get(chip->gpiodev->owner)) > return -ENODEV; > - > - if (gpiochip_lock_as_irq(chip, d->hwirq)) { > - chip_err(chip, > - "unable to lock HW IRQ %lu for IRQ\n", > - d->hwirq); > + if (chip->irq.chip->irq_request_resources) > + res = chip->irq.chip->irq_request_resources(d); > + if (res) > module_put(chip->gpiodev->owner); > - return -EINVAL; > - } > - return 0; > + return res; > } > > static void gpiochip_irq_relres(struct irq_data *d) > { > struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > > - gpiochip_unlock_as_irq(chip, d->hwirq); > + if (chip->irq.chip->irq_release_resources) > + chip->irq.chip->irq_release_resources(d); > module_put(chip->gpiodev->owner); > } > > +static void gpiochip_irq_enable(struct irq_data *d) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + > + WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq)); > + if (chip->irq.chip->irq_enable) > + chip->irq.chip->irq_enable(d); > + else > + chip->irq.chip->irq_unmask(d); > +} > + > +static void gpiochip_irq_disable(struct irq_data *d) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + > + gpiochip_unlock_as_irq(chip, d->hwirq); > + if (chip->irq.chip->irq_disable) > + chip->irq.chip->irq_disable(d); > + else > + chip->irq.chip->irq_mask(d); > +} > + > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > { > if (!gpiochip_irqchip_irq_valid(chip, offset)) > @@ -1889,16 +1909,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, > if (!gpiochip->irq.domain) > return -EINVAL; > > - /* > - * It is possible for a driver to override this, but only if the > - * alternative functions are both implemented. > - */ > - if (!irqchip->irq_request_resources && > - !irqchip->irq_release_resources) { > - irqchip->irq_request_resources = gpiochip_irq_reqres; > - irqchip->irq_release_resources = gpiochip_irq_relres; > - } > - > if (gpiochip->irq.parent_handler) { > void *data = gpiochip->irq.parent_handler_data ?: gpiochip; > > @@ -1956,8 +1966,16 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) > } > > if (gpiochip->irq.chip) { > - gpiochip->irq.chip->irq_request_resources = NULL; > - gpiochip->irq.chip->irq_release_resources = NULL; > + gpiochip->irq.chip->irq_request_resources = > + gpiochip->irq.irq_request_resources; > + gpiochip->irq.chip->irq_release_resources = > + gpiochip->irq.irq_release_resources; > + gpiochip->irq.chip->irq_enable = gpiochip->irq.irq_enable; > + gpiochip->irq.chip->irq_disable = gpiochip->irq.irq_disable; > + gpiochip->irq.irq_request_resources = NULL; > + gpiochip->irq.irq_release_resources = NULL; > + gpiochip->irq.irq_enable = NULL; > + gpiochip->irq.irq_disable = NULL; > gpiochip->irq.chip = NULL; > } > > @@ -2048,15 +2066,15 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, > return -EINVAL; > } > > - /* > - * It is possible for a driver to override this, but only if the > - * alternative functions are both implemented. > - */ > - if (!irqchip->irq_request_resources && > - !irqchip->irq_release_resources) { > - irqchip->irq_request_resources = gpiochip_irq_reqres; > - irqchip->irq_release_resources = gpiochip_irq_relres; > - } > + gpiochip->irq.irq_request_resources = irqchip->irq_request_resources; > + gpiochip->irq.irq_release_resources = irqchip->irq_release_resources; > + gpiochip->irq.irq_enable = irqchip->irq_enable; > + gpiochip->irq.irq_disable = irqchip->irq_disable; > + > + irqchip->irq_request_resources = gpiochip_irq_reqres; > + irqchip->irq_release_resources = gpiochip_irq_relres; > + irqchip->irq_enable = gpiochip_irq_enable; > + irqchip->irq_disable = gpiochip_irq_disable; > > acpi_gpiochip_request_interrupts(gpiochip); > > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index 5382b5183b7e..e352dd160424 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -138,6 +138,11 @@ struct gpio_irq_chip { > * will allocate and map all IRQs during initialization. > */ > unsigned int first; > + > + int (*irq_request_resources)(struct irq_data *data); > + void (*irq_release_resources)(struct irq_data *data); > + void (*irq_enable)(struct irq_data *data); > + void (*irq_disable)(struct irq_data *data); > }; > > static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip) > -- 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
Re-ping! If I don't see any replies in the next few days I'll just go ahead and make a proper patch series of this. Regards, Hans On 02/08/18 09:00, Hans Verkuil wrote: > Ping! > > Regards, > > Hans > > On 07/23/2018 05:03 PM, Hans Verkuil wrote: >> Hi all, >> >> Here is yet another attempt to allow drivers to disable the irq and drive >> the gpio as an output. >> >> Please be gentle with me: I am neither an expert on the gpio internals, nor on >> the irq internals. >> >> This patch lets gpiolib override the irq_chip's irq_request/release_resources and >> irq_en/disable hooks. >> >> The old hooks are stored and called by gpiolib. >> >> As a result, the gpiochip_(un)lock_as_irq functions can become static (not yet >> implemented in this patch) and these calls should be removed from all drivers. >> >> I haven't done that since I first want to know if what I am doing here even >> makes sense. >> >> Reviewing the removal of those calls in drivers should be fairly easy. >> >> Regards, >> >> Hans >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> drivers/gpio/gpiolib.c | 78 +++++++++++++++++++++++-------------- >> include/linux/gpio/driver.h | 5 +++ >> 2 files changed, 53 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index e11a3bb03820..20429197756f 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -1800,28 +1800,48 @@ static const struct irq_domain_ops gpiochip_domain_ops = { >> static int gpiochip_irq_reqres(struct irq_data *d) >> { >> struct gpio_chip *chip = irq_data_get_irq_chip_data(d); >> + int res = 0; >> >> if (!try_module_get(chip->gpiodev->owner)) >> return -ENODEV; >> - >> - if (gpiochip_lock_as_irq(chip, d->hwirq)) { >> - chip_err(chip, >> - "unable to lock HW IRQ %lu for IRQ\n", >> - d->hwirq); >> + if (chip->irq.chip->irq_request_resources) >> + res = chip->irq.chip->irq_request_resources(d); >> + if (res) >> module_put(chip->gpiodev->owner); >> - return -EINVAL; >> - } >> - return 0; >> + return res; >> } >> >> static void gpiochip_irq_relres(struct irq_data *d) >> { >> struct gpio_chip *chip = irq_data_get_irq_chip_data(d); >> >> - gpiochip_unlock_as_irq(chip, d->hwirq); >> + if (chip->irq.chip->irq_release_resources) >> + chip->irq.chip->irq_release_resources(d); >> module_put(chip->gpiodev->owner); >> } >> >> +static void gpiochip_irq_enable(struct irq_data *d) >> +{ >> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); >> + >> + WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq)); >> + if (chip->irq.chip->irq_enable) >> + chip->irq.chip->irq_enable(d); >> + else >> + chip->irq.chip->irq_unmask(d); >> +} >> + >> +static void gpiochip_irq_disable(struct irq_data *d) >> +{ >> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); >> + >> + gpiochip_unlock_as_irq(chip, d->hwirq); >> + if (chip->irq.chip->irq_disable) >> + chip->irq.chip->irq_disable(d); >> + else >> + chip->irq.chip->irq_mask(d); >> +} >> + >> static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) >> { >> if (!gpiochip_irqchip_irq_valid(chip, offset)) >> @@ -1889,16 +1909,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, >> if (!gpiochip->irq.domain) >> return -EINVAL; >> >> - /* >> - * It is possible for a driver to override this, but only if the >> - * alternative functions are both implemented. >> - */ >> - if (!irqchip->irq_request_resources && >> - !irqchip->irq_release_resources) { >> - irqchip->irq_request_resources = gpiochip_irq_reqres; >> - irqchip->irq_release_resources = gpiochip_irq_relres; >> - } >> - >> if (gpiochip->irq.parent_handler) { >> void *data = gpiochip->irq.parent_handler_data ?: gpiochip; >> >> @@ -1956,8 +1966,16 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) >> } >> >> if (gpiochip->irq.chip) { >> - gpiochip->irq.chip->irq_request_resources = NULL; >> - gpiochip->irq.chip->irq_release_resources = NULL; >> + gpiochip->irq.chip->irq_request_resources = >> + gpiochip->irq.irq_request_resources; >> + gpiochip->irq.chip->irq_release_resources = >> + gpiochip->irq.irq_release_resources; >> + gpiochip->irq.chip->irq_enable = gpiochip->irq.irq_enable; >> + gpiochip->irq.chip->irq_disable = gpiochip->irq.irq_disable; >> + gpiochip->irq.irq_request_resources = NULL; >> + gpiochip->irq.irq_release_resources = NULL; >> + gpiochip->irq.irq_enable = NULL; >> + gpiochip->irq.irq_disable = NULL; >> gpiochip->irq.chip = NULL; >> } >> >> @@ -2048,15 +2066,15 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, >> return -EINVAL; >> } >> >> - /* >> - * It is possible for a driver to override this, but only if the >> - * alternative functions are both implemented. >> - */ >> - if (!irqchip->irq_request_resources && >> - !irqchip->irq_release_resources) { >> - irqchip->irq_request_resources = gpiochip_irq_reqres; >> - irqchip->irq_release_resources = gpiochip_irq_relres; >> - } >> + gpiochip->irq.irq_request_resources = irqchip->irq_request_resources; >> + gpiochip->irq.irq_release_resources = irqchip->irq_release_resources; >> + gpiochip->irq.irq_enable = irqchip->irq_enable; >> + gpiochip->irq.irq_disable = irqchip->irq_disable; >> + >> + irqchip->irq_request_resources = gpiochip_irq_reqres; >> + irqchip->irq_release_resources = gpiochip_irq_relres; >> + irqchip->irq_enable = gpiochip_irq_enable; >> + irqchip->irq_disable = gpiochip_irq_disable; >> >> acpi_gpiochip_request_interrupts(gpiochip); >> >> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h >> index 5382b5183b7e..e352dd160424 100644 >> --- a/include/linux/gpio/driver.h >> +++ b/include/linux/gpio/driver.h >> @@ -138,6 +138,11 @@ struct gpio_irq_chip { >> * will allocate and map all IRQs during initialization. >> */ >> unsigned int first; >> + >> + int (*irq_request_resources)(struct irq_data *data); >> + void (*irq_release_resources)(struct irq_data *data); >> + void (*irq_enable)(struct irq_data *data); >> + void (*irq_disable)(struct irq_data *data); >> }; >> >> static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip) >> >
On Mon, Aug 13, 2018 at 11:40 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > Re-ping! > > If I don't see any replies in the next few days I'll just go ahead and make > a proper patch series of this. Sorry for slowness, I was on vacation and then tossed into the merge window. Reading back now! Yours, Linus Walleij
On 08/20/2018 09:55 AM, Linus Walleij wrote: > On Mon, Aug 13, 2018 at 11:40 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> Re-ping! >> >> If I don't see any replies in the next few days I'll just go ahead and make >> a proper patch series of this. > > Sorry for slowness, I was on vacation and then tossed into the merge > window. No problem! I haven't posted any patches yet, BTW. I did prepare patches, but I had no time to test them. Regards, Hans
On Mon, Jul 23, 2018 at 5:03 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > Here is yet another attempt to allow drivers to disable the irq and drive > the gpio as an output. This is more like it! > This patch lets gpiolib override the irq_chip's irq_request/release_resources and > irq_en/disable hooks. > > The old hooks are stored and called by gpiolib. OK this seems reasonable. > static void gpiochip_irq_relres(struct irq_data *d) > { > struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > > - gpiochip_unlock_as_irq(chip, d->hwirq); > + if (chip->irq.chip->irq_release_resources) > + chip->irq.chip->irq_release_resources(d); > module_put(chip->gpiodev->owner); > } > > +static void gpiochip_irq_enable(struct irq_data *d) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + > + WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq)); > + if (chip->irq.chip->irq_enable) > + chip->irq.chip->irq_enable(d); > + else > + chip->irq.chip->irq_unmask(d); > +} > + > +static void gpiochip_irq_disable(struct irq_data *d) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + > + gpiochip_unlock_as_irq(chip, d->hwirq); > + if (chip->irq.chip->irq_disable) > + chip->irq.chip->irq_disable(d); > + else > + chip->irq.chip->irq_mask(d); > +} This does the right thing. The only problem I see is that this kind of re-implements the core semantics of the irqhchip to call disable/mask or enable/unmask. But it's fine if Marc is OK with it, it does the trick. Yours, Linus Walleij
On Mon, Aug 20, 2018 at 10:20 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > I haven't posted any patches yet, BTW. I did prepare patches, but I had no > time to test them. I'm curious about it. This change will remove the slowpath as I wanted so I see it as a very desireable improvement even if it's mostly there to fix a specific CEC usecase! Yours, Linus Walleij
On 08/20/2018 11:10 AM, Linus Walleij wrote: > On Mon, Aug 20, 2018 at 10:20 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> I haven't posted any patches yet, BTW. I did prepare patches, but I had no >> time to test them. > > I'm curious about it. This change will remove the slowpath as I wanted > so I see it as a very desireable improvement even if it's mostly there > to fix a specific CEC usecase! > > Yours, > Linus Walleij > It's available here: https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=gpio-test The newest patch removes gpiochip_(un)lock_as_irq from all drivers (and often whole callbacks could be removed since that was the only reason for having the callback in the first place). The patch before that makes the core gpiolib change. I'm uncertain about the gpiolib-sysfs.c and -acpi.c changes and also the hid-cp2112.c changes. This newest patch is just compile tested. The core gpiolib patch has been tested with the BeagleBone Black and the CEC driver. Regards, Hans
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e11a3bb03820..20429197756f 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1800,28 +1800,48 @@ static const struct irq_domain_ops gpiochip_domain_ops = { static int gpiochip_irq_reqres(struct irq_data *d) { struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + int res = 0; if (!try_module_get(chip->gpiodev->owner)) return -ENODEV; - - if (gpiochip_lock_as_irq(chip, d->hwirq)) { - chip_err(chip, - "unable to lock HW IRQ %lu for IRQ\n", - d->hwirq); + if (chip->irq.chip->irq_request_resources) + res = chip->irq.chip->irq_request_resources(d); + if (res) module_put(chip->gpiodev->owner); - return -EINVAL; - } - return 0; + return res; } static void gpiochip_irq_relres(struct irq_data *d) { struct gpio_chip *chip = irq_data_get_irq_chip_data(d); - gpiochip_unlock_as_irq(chip, d->hwirq); + if (chip->irq.chip->irq_release_resources) + chip->irq.chip->irq_release_resources(d); module_put(chip->gpiodev->owner); } +static void gpiochip_irq_enable(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + + WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq)); + if (chip->irq.chip->irq_enable) + chip->irq.chip->irq_enable(d); + else + chip->irq.chip->irq_unmask(d); +} + +static void gpiochip_irq_disable(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + + gpiochip_unlock_as_irq(chip, d->hwirq); + if (chip->irq.chip->irq_disable) + chip->irq.chip->irq_disable(d); + else + chip->irq.chip->irq_mask(d); +} + static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) { if (!gpiochip_irqchip_irq_valid(chip, offset)) @@ -1889,16 +1909,6 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, if (!gpiochip->irq.domain) return -EINVAL; - /* - * It is possible for a driver to override this, but only if the - * alternative functions are both implemented. - */ - if (!irqchip->irq_request_resources && - !irqchip->irq_release_resources) { - irqchip->irq_request_resources = gpiochip_irq_reqres; - irqchip->irq_release_resources = gpiochip_irq_relres; - } - if (gpiochip->irq.parent_handler) { void *data = gpiochip->irq.parent_handler_data ?: gpiochip; @@ -1956,8 +1966,16 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) } if (gpiochip->irq.chip) { - gpiochip->irq.chip->irq_request_resources = NULL; - gpiochip->irq.chip->irq_release_resources = NULL; + gpiochip->irq.chip->irq_request_resources = + gpiochip->irq.irq_request_resources; + gpiochip->irq.chip->irq_release_resources = + gpiochip->irq.irq_release_resources; + gpiochip->irq.chip->irq_enable = gpiochip->irq.irq_enable; + gpiochip->irq.chip->irq_disable = gpiochip->irq.irq_disable; + gpiochip->irq.irq_request_resources = NULL; + gpiochip->irq.irq_release_resources = NULL; + gpiochip->irq.irq_enable = NULL; + gpiochip->irq.irq_disable = NULL; gpiochip->irq.chip = NULL; } @@ -2048,15 +2066,15 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, return -EINVAL; } - /* - * It is possible for a driver to override this, but only if the - * alternative functions are both implemented. - */ - if (!irqchip->irq_request_resources && - !irqchip->irq_release_resources) { - irqchip->irq_request_resources = gpiochip_irq_reqres; - irqchip->irq_release_resources = gpiochip_irq_relres; - } + gpiochip->irq.irq_request_resources = irqchip->irq_request_resources; + gpiochip->irq.irq_release_resources = irqchip->irq_release_resources; + gpiochip->irq.irq_enable = irqchip->irq_enable; + gpiochip->irq.irq_disable = irqchip->irq_disable; + + irqchip->irq_request_resources = gpiochip_irq_reqres; + irqchip->irq_release_resources = gpiochip_irq_relres; + irqchip->irq_enable = gpiochip_irq_enable; + irqchip->irq_disable = gpiochip_irq_disable; acpi_gpiochip_request_interrupts(gpiochip); diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 5382b5183b7e..e352dd160424 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -138,6 +138,11 @@ struct gpio_irq_chip { * will allocate and map all IRQs during initialization. */ unsigned int first; + + int (*irq_request_resources)(struct irq_data *data); + void (*irq_release_resources)(struct irq_data *data); + void (*irq_enable)(struct irq_data *data); + void (*irq_disable)(struct irq_data *data); }; static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip)
Hi all, Here is yet another attempt to allow drivers to disable the irq and drive the gpio as an output. Please be gentle with me: I am neither an expert on the gpio internals, nor on the irq internals. This patch lets gpiolib override the irq_chip's irq_request/release_resources and irq_en/disable hooks. The old hooks are stored and called by gpiolib. As a result, the gpiochip_(un)lock_as_irq functions can become static (not yet implemented in this patch) and these calls should be removed from all drivers. I haven't done that since I first want to know if what I am doing here even makes sense. Reviewing the removal of those calls in drivers should be fairly easy. Regards, Hans Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> --- drivers/gpio/gpiolib.c | 78 +++++++++++++++++++++++-------------- include/linux/gpio/driver.h | 5 +++ 2 files changed, 53 insertions(+), 30 deletions(-)