| Message ID | 20180831143744.126063-4-hverkuil@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: track irq enabled/disabled state | expand |
On Fri, Aug 31, 2018 at 4:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > From: Hans Verkuil <hans.verkuil@cisco.com> > > When using the gpiolib irqchip helpers install irq_enable/disable > hooks for the irqchip to ensure that gpiolib knows when the irq > is enabled or disabled, allowing drivers to disable the irq and then > use it as an output pin, and later switch the direction to input and > re-enable the irq. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> Overall I'm a big fan of this as you already know! > + /* > + * 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_enable = irqchip->irq_enable; > + gpiochip->irq.irq_disable = irqchip->irq_disable; > + > + irqchip->irq_enable = gpiochip_irq_enable; > + irqchip->irq_disable = gpiochip_irq_disable; Actully the latter method, to use local copies of the enable/disable function pointers in gpiochip->irq is what we should use also for the request/release_resources callbacks. The chips that actually use local irq_request_resources and irq_release resources AND GPIOLIB_IRQCHIP are limited to: drivers/gpio/gpio-dwapb.c drivers/pinctrl/intel/pinctrl-intel.c drivers/pinctrl/pinctrl-st.c drivers/pinctrl/qcom/pinctrl-msm.c As far as I can tell. They probably all have some boilerplate related to reimplementing part of the core functions. But we can fix that in a separate patch after this. Yours, Linus Walleij
On 09/05/18 12:28, Linus Walleij wrote: > On Fri, Aug 31, 2018 at 4:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> When using the gpiolib irqchip helpers install irq_enable/disable >> hooks for the irqchip to ensure that gpiolib knows when the irq >> is enabled or disabled, allowing drivers to disable the irq and then >> use it as an output pin, and later switch the direction to input and >> re-enable the irq. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > Overall I'm a big fan of this as you already know! > >> + /* >> + * 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_enable = irqchip->irq_enable; >> + gpiochip->irq.irq_disable = irqchip->irq_disable; >> + >> + irqchip->irq_enable = gpiochip_irq_enable; >> + irqchip->irq_disable = gpiochip_irq_disable; > > Actully the latter method, to use local copies of the enable/disable > function pointers in gpiochip->irq is what we should use also > for the request/release_resources callbacks. I'll change this. Regards, Hans > > The chips that actually use local irq_request_resources and > irq_release resources AND GPIOLIB_IRQCHIP are limited to: > > drivers/gpio/gpio-dwapb.c > drivers/pinctrl/intel/pinctrl-intel.c > drivers/pinctrl/pinctrl-st.c > drivers/pinctrl/qcom/pinctrl-msm.c > > As far as I can tell. > > They probably all have some boilerplate related to reimplementing > part of the core functions. > > But we can fix that in a separate patch after this. > > Yours, > Linus Walleij >
On 09/05/2018 12:28 PM, Linus Walleij wrote: > On Fri, Aug 31, 2018 at 4:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> When using the gpiolib irqchip helpers install irq_enable/disable >> hooks for the irqchip to ensure that gpiolib knows when the irq >> is enabled or disabled, allowing drivers to disable the irq and then >> use it as an output pin, and later switch the direction to input and >> re-enable the irq. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > Overall I'm a big fan of this as you already know! > >> + /* >> + * 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_enable = irqchip->irq_enable; >> + gpiochip->irq.irq_disable = irqchip->irq_disable; >> + >> + irqchip->irq_enable = gpiochip_irq_enable; >> + irqchip->irq_disable = gpiochip_irq_disable; > > Actully the latter method, to use local copies of the enable/disable > function pointers in gpiochip->irq is what we should use also > for the request/release_resources callbacks. > > The chips that actually use local irq_request_resources and > irq_release resources AND GPIOLIB_IRQCHIP are limited to: > > drivers/gpio/gpio-dwapb.c Are you sure? I don't see this one selecting GPIOLIB_IRQCHIP. > drivers/pinctrl/intel/pinctrl-intel.c > drivers/pinctrl/pinctrl-st.c > drivers/pinctrl/qcom/pinctrl-msm.c Ditto for the qcom driver. The ST and Intel drivers do use it and those two driver needs updating. Regards, Hans > > As far as I can tell. > > They probably all have some boilerplate related to reimplementing > part of the core functions. > > But we can fix that in a separate patch after this. > > Yours, > Linus Walleij >
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 2ad58e63ce70..49a32f45eac6 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -86,6 +86,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, 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); +static void gpiochip_irq_enable(struct irq_data *d); +static void gpiochip_irq_disable(struct irq_data *d); static int gpiochip_irq_reqres(struct irq_data *d); static void gpiochip_irq_relres(struct irq_data *d); @@ -1814,6 +1816,30 @@ static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) return irq_create_mapping(chip->irq.domain, offset); } +static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip) +{ + struct irq_chip *irqchip = gpiochip->irq.chip; + + if (WARN_ON(irqchip->irq_enable == gpiochip_irq_enable)) + return; + + /* + * 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_enable = irqchip->irq_enable; + gpiochip->irq.irq_disable = irqchip->irq_disable; + + irqchip->irq_enable = gpiochip_irq_enable; + irqchip->irq_disable = gpiochip_irq_disable; +} + /** * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip * @gpiochip: the GPIO chip to add the IRQ chip to @@ -1872,16 +1898,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; @@ -1897,6 +1913,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, } } + gpiochip_set_irq_hooks(gpiochip); + acpi_gpiochip_request_interrupts(gpiochip); return 0; @@ -1910,11 +1928,12 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, */ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) { + struct irq_chip *irqchip = gpiochip->irq.chip; unsigned int offset; acpi_gpiochip_free_interrupts(gpiochip); - if (gpiochip->irq.chip && gpiochip->irq.parent_handler) { + if (irqchip && gpiochip->irq.parent_handler) { struct gpio_irq_chip *irq = &gpiochip->irq; unsigned int i; @@ -1938,11 +1957,16 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) irq_domain_remove(gpiochip->irq.domain); } - if (gpiochip->irq.chip) { - gpiochip->irq.chip->irq_request_resources = NULL; - gpiochip->irq.chip->irq_release_resources = NULL; - gpiochip->irq.chip = NULL; + if (irqchip && + irqchip->irq_enable == gpiochip_irq_enable) { + irqchip->irq_request_resources = NULL; + irqchip->irq_release_resources = NULL; + irqchip->irq_enable = gpiochip->irq.irq_enable; + irqchip->irq_disable = gpiochip->irq.irq_disable; } + gpiochip->irq.irq_enable = NULL; + gpiochip->irq.irq_disable = NULL; + gpiochip->irq.chip = NULL; gpiochip_irqchip_free_valid_mask(gpiochip); } @@ -2031,15 +2055,7 @@ 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_set_irq_hooks(gpiochip); acpi_gpiochip_request_interrupts(gpiochip); @@ -3329,6 +3345,28 @@ void gpiochip_enable_irq(struct gpio_chip *chip, unsigned int offset) } EXPORT_SYMBOL_GPL(gpiochip_enable_irq); +static void gpiochip_irq_enable(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + + gpiochip_enable_irq(chip, d->hwirq); + if (chip->irq.irq_enable) + chip->irq.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); + + if (chip->irq.irq_disable) + chip->irq.irq_disable(d); + else + chip->irq.chip->irq_mask(d); + gpiochip_disable_irq(chip, d->hwirq); +} + bool gpiochip_line_is_irq(struct gpio_chip *chip, unsigned int offset) { if (offset >= chip->ngpio) diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 84449e95587a..46a896773448 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -138,6 +138,9 @@ struct gpio_irq_chip { * will allocate and map all IRQs during initialization. */ unsigned int first; + + 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)