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@xxxxxxxxx> >> --- >> 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) >> >