| Message ID | 8c586fd0-7deb-82bf-4c3b-3e2d0a3d1738@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] gpiolib: call gpiochip_(un)lock_as_irq for dis/enable_irq() | expand |
Ping? Regards, Hans On 06/07/18 13:25, Hans Verkuil wrote: > If a gpio pin has an interrupt associated with it, but you need to set the > direction of the pin to output, then what you want to do is to disable the > interrupt and change direction to output. And after changing the direction > back to input, then you call enable_irq again. > > This does not work today since FLAG_USED_AS_IRQ is set when the interrupt > is first requested and gpiod_direction_output() checks for that. So the > only option is to free the interrupt and re-request it, which is painful. > > This RFC patch sets hooks that allow the driver to know when the irq is > disabled and enabled and it sets FLAG_USED_AS_IRQ accordingly. > > It works, but I have one open question: > > When gpiochip_irq_enable() is called, I currently just WARN if > gpiochip_lock_as_irq() fails and continue after that. It really is > a driver bug, but I wonder if it is also possible to automatically > change the direction to input before enabling the interrupt. I have no > idea how to do this, though, since gpiod_direction_input() needs a > struct gpio_desc whereas in irq_enable() I have a struct gpio_chip. > > This patch would be really useful for two CEC drivers: > > In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and > re-request the irq (cec_gpio_dis/enable_irq()). This driver implements > low-level support for the HDMI CEC bus. When waiting for something to > happen the gpio pin is set to input with an interrupt to avoid polling. > When writing to the bus it is set to output and the interrupt obviously > has to be removed (or, as I would like to see, just disabled). Requesting > an irq can fail, and handling that is really problematic. Just disabling > and enabling is much easier and cleaner. > > The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the > tda998x_cec_calibration() function: this is also a CEC driver but here > the tda998x interrupt pin is overloaded: when calibrating the CEC line > the interrupt pin of the chip is used as a gpio calibration output signal. > > This driver disables the interrupt and switches the direction to output > before doing the calibration. This fails for the BeagleBone board unless > I apply this patch. This driver was originally developed for the dove-cubox > board which uses this gpio driver: drivers/gpio/gpio-mvebu.c. For some > reason this worked fine (i.e. it is apparently possible to disable the irq > and change the output without running into the FLAG_USED_AS_IRQ check), but > I don't understand why. I don't have this hardware, so I can't test it. > > I'm a newbie when it comes to gpio, so I have no idea how much sense this > patch makes. > > Regards, > > Hans > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index e11a3bb03820..dcdb457b83b0 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1822,6 +1822,20 @@ static void gpiochip_irq_relres(struct irq_data *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)); > +} > + > +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); > +} > + > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > { > if (!gpiochip_irqchip_irq_valid(chip, offset)) > @@ -1897,6 +1911,10 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, > !irqchip->irq_release_resources) { > irqchip->irq_request_resources = gpiochip_irq_reqres; > irqchip->irq_release_resources = gpiochip_irq_relres; > + if (!irqchip->irq_enable && !irqchip->irq_disable) { > + irqchip->irq_enable = gpiochip_irq_enable; > + irqchip->irq_disable = gpiochip_irq_disable; > + } > } > > if (gpiochip->irq.parent_handler) { > @@ -2056,6 +2074,10 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, > !irqchip->irq_release_resources) { > irqchip->irq_request_resources = gpiochip_irq_reqres; > irqchip->irq_release_resources = gpiochip_irq_relres; > + if (!irqchip->irq_enable && !irqchip->irq_disable) { > + irqchip->irq_enable = gpiochip_irq_enable; > + irqchip->irq_disable = gpiochip_irq_disable; > + } > } > > acpi_gpiochip_request_interrupts(gpiochip); > -- 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 Fri, Jul 6, 2018 at 1:25 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > If a gpio pin has an interrupt associated with it, but you need to set the > direction of the pin to output, then what you want to do is to disable the > interrupt and change direction to output. And after changing the direction > back to input, then you call enable_irq again. OK so that kind of works already, but you want to make it more seamless and convenient (which is nice). > This does not work today since FLAG_USED_AS_IRQ is set when the interrupt > is first requested and gpiod_direction_output() checks for that. So the > only option is to free the interrupt and re-request it, which is painful. It's not the only option (hehe). If you study: drivers/w1/masters/w1-gpio.c you see that this uses gpiod_set_raw_value() which will ignore all flags and protection and just drive the line. In the W1 case this is done to "recharge" the line. So when you know exactly what you're doing it's fine to just drive the line. > When gpiochip_irq_enable() is called, I currently just WARN if > gpiochip_lock_as_irq() fails and continue after that. It really is > a driver bug, but I wonder if it is also possible to automatically > change the direction to input before enabling the interrupt. I think I initially had gpiod_lock_as_irq() set direction to input unless it was already but there was some problem with that. >I have no > idea how to do this, though, since gpiod_direction_input() needs a > struct gpio_desc whereas in irq_enable() I have a struct gpio_chip. That is not working anyway, you need to handle this on a gpio_desc level. > In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and > re-request the irq (cec_gpio_dis/enable_irq()). This driver implements > low-level support for the HDMI CEC bus. When waiting for something to > happen the gpio pin is set to input with an interrupt to avoid polling. > When writing to the bus it is set to output and the interrupt obviously > has to be removed (or, as I would like to see, just disabled). Requesting > an irq can fail, and handling that is really problematic. Just disabling > and enabling is much easier and cleaner. Yeah. But it works, kind of atleast... > The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the > tda998x_cec_calibration() function: this is also a CEC driver but here > the tda998x interrupt pin is overloaded: when calibrating the CEC line > the interrupt pin of the chip is used as a gpio calibration output signal. That sounds like you should use the raw accessor in this case. > +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)); > +} > + > +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); > +} This will not work other than with drivers that use GPIOLIB_IRQCHIP right? Since there are a lot of drivers that don't used that, this is not going to work. The key is that not all GPIO controllers enumerate their IRQs from zero, and some have "holes" in the IRQ or GPIO rangem so d->hwirq is not generally the same as the GPIO offset, it just happens to be like that in many cases, but it is a hardware specific. > + if (!irqchip->irq_enable && !irqchip->irq_disable) { > + irqchip->irq_enable = gpiochip_irq_enable; > + irqchip->irq_disable = gpiochip_irq_disable; > + } So these hooks would have to be in the irqchip driver and opt-in. But I don't know about that. The lock_as_irq() callback is in the irqchip .request_resources since it is the only safe slowpath call from irqchip. I initially had these hooks in startup/shutdown IRQ but it didn't work because almost all irqchip callbacks are implicitly callable from fastpath IIRC. See commits: commit c1bacbae8192dd2a9ebadd22d793b68054f6c6e5 "genirq: Provide irq_request/release_resources chip callbacks" commit 57ef04288abd27a717287a652d324f95cb77c3c6 "gpio: switch drivers to use new callback" 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
On 18/07/18 09:59, Linus Walleij wrote: > On Fri, Jul 6, 2018 at 1:25 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> If a gpio pin has an interrupt associated with it, but you need to set the >> direction of the pin to output, then what you want to do is to disable the >> interrupt and change direction to output. And after changing the direction >> back to input, then you call enable_irq again. > > OK so that kind of works already, but you want to make it more > seamless and convenient (which is nice). > >> This does not work today since FLAG_USED_AS_IRQ is set when the interrupt >> is first requested and gpiod_direction_output() checks for that. So the >> only option is to free the interrupt and re-request it, which is painful. > > It's not the only option (hehe). If you study: > drivers/w1/masters/w1-gpio.c > you see that this uses gpiod_set_raw_value() which will ignore > all flags and protection and just drive the line. > > In the W1 case this is done to "recharge" the line. > > So when you know exactly what you're doing it's fine to just > drive the line. But the problem is not with setting the gpio, it's with changing the direction. I just noticed that there is a gpiod_direction_output_raw() as well that skips the IRQ test, but gpiod_direction_output() does more things than gpiod_direction_output_raw(), in particular it is calling gpio_set_drive_single_ended(). It looks like I can work around this by: 1) calling gpiod_direction_output() to ensure gpio_set_drive_single_ended() is called. 2) call gpiod_direction_input() so it is safe to add the interrupt. 3) call request_irq 4) If I now want to switch from input to output I can disable the irq and call gpiod_direction_output_raw() and use gpiod_is_active_low() to determine whether active is high or low (since I still need that). I don't see where gpiod_set_raw_value() comes in. In fact, it seems that gpiod_set_value() just changes the direction automatically and that there is no need to explicitly call gpiod_direction_output() at all, except once during initialization to ensure that gpio_set_drive_single_ended() is called. If true, then in point 4 above I can just call gpiod_set_value and drop the gpiod_direction_output()/_input() calls altogether. Or am I missing something? Note: the CEC pin used in cec-gpio.c is open drain. > >> When gpiochip_irq_enable() is called, I currently just WARN if >> gpiochip_lock_as_irq() fails and continue after that. It really is >> a driver bug, but I wonder if it is also possible to automatically >> change the direction to input before enabling the interrupt. > > I think I initially had gpiod_lock_as_irq() set direction to > input unless it was already but there was some problem > with that. > >> I have no >> idea how to do this, though, since gpiod_direction_input() needs a >> struct gpio_desc whereas in irq_enable() I have a struct gpio_chip. > > That is not working anyway, you need to handle this on a gpio_desc > level. I suspected as much, but it is good to have confirmation of this. > >> In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and >> re-request the irq (cec_gpio_dis/enable_irq()). This driver implements >> low-level support for the HDMI CEC bus. When waiting for something to >> happen the gpio pin is set to input with an interrupt to avoid polling. >> When writing to the bus it is set to output and the interrupt obviously >> has to be removed (or, as I would like to see, just disabled). Requesting >> an irq can fail, and handling that is really problematic. Just disabling >> and enabling is much easier and cleaner. > > Yeah. But it works, kind of atleast... > >> The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the >> tda998x_cec_calibration() function: this is also a CEC driver but here >> the tda998x interrupt pin is overloaded: when calibrating the CEC line >> the interrupt pin of the chip is used as a gpio calibration output signal. > > That sounds like you should use the raw accessor in this case. > >> +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)); >> +} >> + >> +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); >> +} > > This will not work other than with drivers that use GPIOLIB_IRQCHIP > right? Since there are a lot of drivers that don't used that, > this is not going to work. Correct. But it is this specific case that causes the problem. If either of these CEC drivers are used in non-GPIOLIB_IRQCHIP situations, then those should be fixed there as well, i.e. by adding irq_enable/irq_disable hooks. Looking at 'git grep gpiochip_lock_as_irq' the only relevant gpio driver for cec-gpio might be the tegra driver. > The key is that not all GPIO controllers enumerate their IRQs from > zero, and some have "holes" in the IRQ or GPIO rangem so > d->hwirq is not generally the same as the GPIO offset, it just happens > to be like that in many cases, but it is a hardware specific. > >> + if (!irqchip->irq_enable && !irqchip->irq_disable) { >> + irqchip->irq_enable = gpiochip_irq_enable; >> + irqchip->irq_disable = gpiochip_irq_disable; >> + } > > So these hooks would have to be in the irqchip driver and opt-in. > But I don't know about that. > > The lock_as_irq() callback is in the irqchip .request_resources since > it is the only safe slowpath call from irqchip. > > I initially had these hooks in startup/shutdown IRQ but it didn't work > because almost all irqchip callbacks are implicitly callable from > fastpath IIRC. > > See commits: > commit c1bacbae8192dd2a9ebadd22d793b68054f6c6e5 > "genirq: Provide irq_request/release_resources chip callbacks" > commit 57ef04288abd27a717287a652d324f95cb77c3c6 > "gpio: switch drivers to use new callback" If the proposed work-around above works, then I'm happy to use that. Alternatively we could make a gpiod_direction_output_ignore_irq() call that just skips the FLAG_USED_AS_IRQ check. Regards, Hans > > 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
On Wed, Jul 18, 2018 at 10:56 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> This does not work today since FLAG_USED_AS_IRQ is set when the interrupt > >> is first requested and gpiod_direction_output() checks for that. So the > >> only option is to free the interrupt and re-request it, which is painful. > > > > It's not the only option (hehe). If you study: > > drivers/w1/masters/w1-gpio.c > > you see that this uses gpiod_set_raw_value() which will ignore > > all flags and protection and just drive the line. > > > > In the W1 case this is done to "recharge" the line. > > > > So when you know exactly what you're doing it's fine to just > > drive the line. > > But the problem is not with setting the gpio, it's with changing the > direction. So I was thinking the only reason to change the direction from input to output would be to drive a value on the output, so then you could request the line as input and use the raw accessor to anyways actively drive the input when it needs driving. > I just noticed that there is a gpiod_direction_output_raw() as well > that skips the IRQ test, but gpiod_direction_output() does more things > than gpiod_direction_output_raw(), in particular it is calling > gpio_set_drive_single_ended(). So what happens is that you want some of the semantic checks in gpiolib and not all of them. What we want to do is achieve a 1-1 mapping between the semantics and your usecase. I'm trying to figure out what is the general usecase that is lurking behind this, because I hardly think it is restricted to CEC. > It looks like I can work around this by: > > 1) calling gpiod_direction_output() to ensure gpio_set_drive_single_ended() > is called. > 2) call gpiod_direction_input() so it is safe to add the interrupt. > 3) call request_irq > 4) If I now want to switch from input to output I can disable the irq > and call gpiod_direction_output_raw() and use gpiod_is_active_low() to > determine whether active is high or low (since I still need that). Yeah that starts to bypass all the semantics in the core. If this works I guess go for it, I'm just worried about that it starts to look a bit duct-tape-y. I think the core of your problem is really that the IRQchip abstraction is not providing an easy way to unhinge the IRQ on the line (including unlocking the line as IRQ in gpiolib) so you can dynamically switch the IRQ on and off. Are we trying to work around an irqchip shortcoming in gpiolib? > I don't see where gpiod_set_raw_value() comes in. I just inverted your usecase and made input the default mode and output the (raw) exception instead of the other way around. Have you tried this? Or is is counter-intuitive? > In fact, it seems that > gpiod_set_value() just changes the direction automatically and that there > is no need to explicitly call gpiod_direction_output() at all, gpiod_set_value() does not change the direction in the general sense. I guess you refer to the open drain code that will use direction change to input as a way to drive the line high (through the pull-up resistor on the line). This does not happen if the chip has an explicit .set_config() callback that can set a special bit (or so) to make the hardware natively support open drain. Switching the line to input is just open drain emulation by using the high impedance state (input mode) as open drain. > except once > during initialization to ensure that gpio_set_drive_single_ended() is called. > If true, then in point 4 above I can just call gpiod_set_value and drop the > gpiod_direction_output()/_input() calls altogether. This seems fragile. If you are relying on the "switch to input mode" as described above, you are both exploiting gpiolib semantics and the fact that your particular GPIO controller does not have native open drain support. > > This will not work other than with drivers that use GPIOLIB_IRQCHIP > > right? Since there are a lot of drivers that don't used that, > > this is not going to work. > > Correct. But it is this specific case that causes the problem. > If either of these CEC drivers are used in non-GPIOLIB_IRQCHIP situations, > then those should be fixed there as well, i.e. by adding irq_enable/irq_disable > hooks. > > Looking at 'git grep gpiochip_lock_as_irq' the only relevant gpio driver > for cec-gpio might be the tegra driver. Yeah ... but I think that if we add a semantic like that we need to implement it in all chips. Users will start to expect that they can always do this. Or you will soon see "why is cec-gpio not working for me now!?" and "use another GPIO controller" is not going to be a good answer to that. So it seems like a support disaster. The CEC GPIO driver is supposed to be generic I think. > If the proposed work-around above works, then I'm happy to use that. > Alternatively we could make a gpiod_direction_output_ignore_irq() > call that just skips the FLAG_USED_AS_IRQ check. It all seems pretty duct-tapey. Isn't the problem that attaching/removing the IRQ from the GPIO line is heavy/hard to do? So you are essentially trying to work around a problem with irqchip by duct-taping around with new semantics in gpiolib? Maybe it is better if we discuss this with the irqchip maintainers in that case? I've been told to use the .irq_request_resources() and .irq_release_resources() in struct irq_chip to call .gpiochip_lock_as_irq() and .gpiochip_unlock_as_irq(). But when you look at these functions: gpiochip_unlock_as_irq() and gpiochip_unlock_as_irq(), there is nothing that requires slowpath in them. They can be called with a spinlock held and IRQs disabled. I think the real problem is that GPIOLIB_IRQCHIP, that many irqchips use, use module_get()/put() which are slowpath: static int gpiochip_irq_reqres(struct irq_data *d) { struct gpio_chip *chip = irq_data_get_irq_chip_data(d); 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); module_put(chip->gpiodev->owner); return -EINVAL; } return 0; } 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); module_put(chip->gpiodev->owner); } This leads to the situtation that you can only attach/remove an IRQ from a GPIO line by a whole cycle of [devm_]request_irq() [devm_]free_irq(). You would probably think it's fine if you didn't have to do this heavy lifting and could just request them on probe and call enable_irq() and disable_irq() when needed, and have these call down into the irq_chip .irq_enable() and .irq_disable() and lock the GPIO line as input *there* instead, and then change this everywhere (yes patch all gpio_chips with an irqchip driver in that case). As drivers have likely sometimes already assigned function pointers to .irq_enable() and .irq_disable() these have to be copied and stored in struct gpio_irq_chip() by gpiochip_set_cascaded_irqchip() and called in sequence. But it can be done. What about changing the GPIOLIB_IRQCHIP code to just do the module_get()/put() part on .irq_request_resources() and move the locking to the .irq_enable()/.irq_disable() callbacks so that we can enable and disable irqs on the fly in a better way? (BIG WIN!) What about going and reinvestigating this instead? I'm sorry that I can't present any simple solution, but hey, I presented a really complicated solution instead, isn't it great! :D 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 20/07/18 10:06, Linus Walleij wrote: <snip> > You would probably think it's fine if you didn't have to do this > heavy lifting and could just request them on probe and > call enable_irq() and disable_irq() when needed, and have these > call down into the irq_chip .irq_enable() and .irq_disable() and > lock the GPIO line as input *there* instead, and then change > this everywhere (yes patch all gpio_chips with an irqchip > driver in that case). > > As drivers have likely sometimes already assigned function > pointers to .irq_enable() and .irq_disable() these have to > be copied and stored in struct gpio_irq_chip() by > gpiochip_set_cascaded_irqchip() and called in sequence. > But it can be done. > > What about changing the GPIOLIB_IRQCHIP code to just > do the module_get()/put() part on .irq_request_resources() > and move the locking to the .irq_enable()/.irq_disable() > callbacks so that we can enable and disable irqs on the fly > in a better way? (BIG WIN!) > > What about going and reinvestigating this instead? > > I'm sorry that I can't present any simple solution, but hey, > I presented a really complicated solution instead, isn't it > great! :D I did do some investigation into this, but it made me very unhappy: it's a lot of low-level changes in a lot of drivers for a lot of different boards, some of which are probably hard to test. Scary. But I've come up with a much simpler alternative: add two new gpiolib functions: gpiod_enable_irq and gpiod_disable_irq. An RFC patch is below (documentation is missing, but it works fine). Basically if you have a gpio which has an interrupt, then you can use these functions to disable and enable it. It enables/disables the interrupt and calls gpiochip_(un)lock_as_irq() as well. It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure that the direction is set correctly. I have not done so, but if you think it is useful, then it's trivial to do. Right now it will return an error if the direction is still set to output. If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not sure if that's a bug or a feature. It's easy enough to change. Let me know what you think. IMHO this avoids a huge amount of churn in code you really do no want to touch and it does exactly what you want it to do. And it looks pretty clean as well, both in gpiolib and in the drivers. Regards, Hans Signed-off-by: Hans Verkuil <hansverk@cisco.com> --- drivers/gpio/gpiolib.c | 83 ++++++++++++++++++++++++++--------- include/linux/gpio/consumer.h | 3 ++ 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e11a3bb03820..75ad6177fa95 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc) } EXPORT_SYMBOL_GPL(gpiod_to_irq); -/** - * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ - * @chip: the chip the GPIO to lock belongs to - * @offset: the offset of the GPIO to lock as IRQ - * - * This is used directly by GPIO drivers that want to lock down - * a certain GPIO line to be used for IRQs. - */ -int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) +static int gpiodesc_lock_as_irq(struct gpio_desc *desc) { - struct gpio_desc *desc; - - desc = gpiochip_get_desc(chip, offset); - if (IS_ERR(desc)) - return PTR_ERR(desc); + struct gpio_chip *chip = gpiod_to_chip(desc); /* * If it's fast: flush the direction setting if something changed * behind our back */ if (!chip->can_sleep && chip->get_direction) { - int dir = chip->get_direction(chip, offset); + int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc)); if (dir) clear_bit(FLAG_IS_OUT, &desc->flags); @@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) return 0; } + +/** + * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ + * @chip: the chip the GPIO to lock belongs to + * @offset: the offset of the GPIO to lock as IRQ + * + * This is used directly by GPIO drivers that want to lock down + * a certain GPIO line to be used for IRQs. + */ +int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) +{ + struct gpio_desc *desc; + + desc = gpiochip_get_desc(chip, offset); + if (IS_ERR(desc)) + return PTR_ERR(desc); + return gpiodesc_lock_as_irq(desc); +} EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq); +int gpiod_enable_irq(struct gpio_desc *desc) +{ + int irq; + int ret; + + VALIDATE_DESC(desc); + + irq = gpiod_to_irq(desc); + if (irq < 0) + return 0; + + ret = gpiodesc_lock_as_irq(desc); + if (!ret) + enable_irq(irq); + return ret; +} +EXPORT_SYMBOL_GPL(gpiod_enable_irq); + +static void gpiodesc_unlock_as_irq(struct gpio_desc *desc) +{ + clear_bit(FLAG_USED_AS_IRQ, &desc->flags); + + /* If we only had this marking, erase it */ + if (desc->label && !strcmp(desc->label, "interrupt")) + desc_set_label(desc, NULL); +} + /** * gpiochip_unlock_as_irq() - unlock a GPIO used as IRQ * @chip: the chip the GPIO to lock belongs to @@ -3294,14 +3327,24 @@ void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset) if (IS_ERR(desc)) return; - clear_bit(FLAG_USED_AS_IRQ, &desc->flags); - - /* If we only had this marking, erase it */ - if (desc->label && !strcmp(desc->label, "interrupt")) - desc_set_label(desc, NULL); + gpiodesc_unlock_as_irq(desc); } EXPORT_SYMBOL_GPL(gpiochip_unlock_as_irq); +void gpiod_disable_irq(struct gpio_desc *desc) +{ + int irq; + + VALIDATE_DESC_VOID(desc); + irq = gpiod_to_irq(desc); + if (irq < 0) + return; + + disable_irq(irq); + gpiodesc_unlock_as_irq(desc); +} +EXPORT_SYMBOL_GPL(gpiod_disable_irq); + bool gpiochip_line_is_irq(struct gpio_chip *chip, unsigned int offset) { if (offset >= chip->ngpio) diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 243112c7fa7d..c7a9d154df22 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -104,6 +104,9 @@ int gpiod_direction_input(struct gpio_desc *desc); int gpiod_direction_output(struct gpio_desc *desc, int value); int gpiod_direction_output_raw(struct gpio_desc *desc, int value); +int gpiod_enable_irq(struct gpio_desc *desc); +void gpiod_disable_irq(struct gpio_desc *desc); + /* Value get/set from non-sleeping context */ int gpiod_get_value(const struct gpio_desc *desc); int gpiod_get_array_value(unsigned int array_size,
Hi Hans, On Sat, 21 Jul 2018 09:46:19 +0100, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > Hi Linus, > > On 20/07/18 10:06, Linus Walleij wrote: > > <snip> > > > You would probably think it's fine if you didn't have to do this > > heavy lifting and could just request them on probe and > > call enable_irq() and disable_irq() when needed, and have these > > call down into the irq_chip .irq_enable() and .irq_disable() and > > lock the GPIO line as input *there* instead, and then change > > this everywhere (yes patch all gpio_chips with an irqchip > > driver in that case). > > > > As drivers have likely sometimes already assigned function > > pointers to .irq_enable() and .irq_disable() these have to > > be copied and stored in struct gpio_irq_chip() by > > gpiochip_set_cascaded_irqchip() and called in sequence. > > But it can be done. > > > > What about changing the GPIOLIB_IRQCHIP code to just > > do the module_get()/put() part on .irq_request_resources() > > and move the locking to the .irq_enable()/.irq_disable() > > callbacks so that we can enable and disable irqs on the fly > > in a better way? (BIG WIN!) > > > > What about going and reinvestigating this instead? > > > > I'm sorry that I can't present any simple solution, but hey, > > I presented a really complicated solution instead, isn't it > > great! :D > > I did do some investigation into this, but it made me very unhappy: > it's a lot of low-level changes in a lot of drivers for a lot of > different boards, some of which are probably hard to test. Scary. > > But I've come up with a much simpler alternative: add two new > gpiolib functions: gpiod_enable_irq and gpiod_disable_irq. > > An RFC patch is below (documentation is missing, but it works fine). > > Basically if you have a gpio which has an interrupt, then you can use > these functions to disable and enable it. It enables/disables the interrupt > and calls gpiochip_(un)lock_as_irq() as well. > > It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure > that the direction is set correctly. I have not done so, but if you think it is > useful, then it's trivial to do. Right now it will return an error if the > direction is still set to output. > > If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not > sure if that's a bug or a feature. It's easy enough to change. > > Let me know what you think. IMHO this avoids a huge amount of churn in code you > really do no want to touch and it does exactly what you want it to do. And it > looks pretty clean as well, both in gpiolib and in the drivers. > > Regards, > > Hans > > Signed-off-by: Hans Verkuil <hansverk@cisco.com> > --- > drivers/gpio/gpiolib.c | 83 ++++++++++++++++++++++++++--------- > include/linux/gpio/consumer.h | 3 ++ > 2 files changed, 66 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index e11a3bb03820..75ad6177fa95 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc) > } > EXPORT_SYMBOL_GPL(gpiod_to_irq); > > -/** > - * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ > - * @chip: the chip the GPIO to lock belongs to > - * @offset: the offset of the GPIO to lock as IRQ > - * > - * This is used directly by GPIO drivers that want to lock down > - * a certain GPIO line to be used for IRQs. > - */ > -int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) > +static int gpiodesc_lock_as_irq(struct gpio_desc *desc) > { > - struct gpio_desc *desc; > - > - desc = gpiochip_get_desc(chip, offset); > - if (IS_ERR(desc)) > - return PTR_ERR(desc); > + struct gpio_chip *chip = gpiod_to_chip(desc); > > /* > * If it's fast: flush the direction setting if something changed > * behind our back > */ > if (!chip->can_sleep && chip->get_direction) { > - int dir = chip->get_direction(chip, offset); > + int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc)); > > if (dir) > clear_bit(FLAG_IS_OUT, &desc->flags); > @@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) > > return 0; > } > + > +/** > + * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ > + * @chip: the chip the GPIO to lock belongs to > + * @offset: the offset of the GPIO to lock as IRQ > + * > + * This is used directly by GPIO drivers that want to lock down > + * a certain GPIO line to be used for IRQs. > + */ > +int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) > +{ > + struct gpio_desc *desc; > + > + desc = gpiochip_get_desc(chip, offset); > + if (IS_ERR(desc)) > + return PTR_ERR(desc); > + return gpiodesc_lock_as_irq(desc); > +} > EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq); > > +int gpiod_enable_irq(struct gpio_desc *desc) > +{ > + int irq; > + int ret; > + > + VALIDATE_DESC(desc); > + > + irq = gpiod_to_irq(desc); > + if (irq < 0) > + return 0; > + > + ret = gpiodesc_lock_as_irq(desc); > + if (!ret) > + enable_irq(irq); > + return ret; > +} > +EXPORT_SYMBOL_GPL(gpiod_enable_irq); {enable,disable}_irq() are supposed to allow nesting. Do you intent to preserve the same semantics? This is specially important if you allow sharing of the interrupt line between devices. Thanks, M.
On 21/07/18 12:08, Marc Zyngier wrote: > Hi Hans, > > On Sat, 21 Jul 2018 09:46:19 +0100, > Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> Hi Linus, >> >> On 20/07/18 10:06, Linus Walleij wrote: >> >> <snip> >> >>> You would probably think it's fine if you didn't have to do this >>> heavy lifting and could just request them on probe and >>> call enable_irq() and disable_irq() when needed, and have these >>> call down into the irq_chip .irq_enable() and .irq_disable() and >>> lock the GPIO line as input *there* instead, and then change >>> this everywhere (yes patch all gpio_chips with an irqchip >>> driver in that case). >>> >>> As drivers have likely sometimes already assigned function >>> pointers to .irq_enable() and .irq_disable() these have to >>> be copied and stored in struct gpio_irq_chip() by >>> gpiochip_set_cascaded_irqchip() and called in sequence. >>> But it can be done. >>> >>> What about changing the GPIOLIB_IRQCHIP code to just >>> do the module_get()/put() part on .irq_request_resources() >>> and move the locking to the .irq_enable()/.irq_disable() >>> callbacks so that we can enable and disable irqs on the fly >>> in a better way? (BIG WIN!) >>> >>> What about going and reinvestigating this instead? >>> >>> I'm sorry that I can't present any simple solution, but hey, >>> I presented a really complicated solution instead, isn't it >>> great! :D >> >> I did do some investigation into this, but it made me very unhappy: >> it's a lot of low-level changes in a lot of drivers for a lot of >> different boards, some of which are probably hard to test. Scary. >> >> But I've come up with a much simpler alternative: add two new >> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq. >> >> An RFC patch is below (documentation is missing, but it works fine). >> >> Basically if you have a gpio which has an interrupt, then you can use >> these functions to disable and enable it. It enables/disables the interrupt >> and calls gpiochip_(un)lock_as_irq() as well. >> >> It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure >> that the direction is set correctly. I have not done so, but if you think it is >> useful, then it's trivial to do. Right now it will return an error if the >> direction is still set to output. >> >> If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not >> sure if that's a bug or a feature. It's easy enough to change. >> >> Let me know what you think. IMHO this avoids a huge amount of churn in code you >> really do no want to touch and it does exactly what you want it to do. And it >> looks pretty clean as well, both in gpiolib and in the drivers. >> >> Regards, >> >> Hans >> >> Signed-off-by: Hans Verkuil <hansverk@cisco.com> >> --- >> drivers/gpio/gpiolib.c | 83 ++++++++++++++++++++++++++--------- >> include/linux/gpio/consumer.h | 3 ++ >> 2 files changed, 66 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index e11a3bb03820..75ad6177fa95 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc) >> } >> EXPORT_SYMBOL_GPL(gpiod_to_irq); >> >> -/** >> - * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ >> - * @chip: the chip the GPIO to lock belongs to >> - * @offset: the offset of the GPIO to lock as IRQ >> - * >> - * This is used directly by GPIO drivers that want to lock down >> - * a certain GPIO line to be used for IRQs. >> - */ >> -int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) >> +static int gpiodesc_lock_as_irq(struct gpio_desc *desc) >> { >> - struct gpio_desc *desc; >> - >> - desc = gpiochip_get_desc(chip, offset); >> - if (IS_ERR(desc)) >> - return PTR_ERR(desc); >> + struct gpio_chip *chip = gpiod_to_chip(desc); >> >> /* >> * If it's fast: flush the direction setting if something changed >> * behind our back >> */ >> if (!chip->can_sleep && chip->get_direction) { >> - int dir = chip->get_direction(chip, offset); >> + int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc)); >> >> if (dir) >> clear_bit(FLAG_IS_OUT, &desc->flags); >> @@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) >> >> return 0; >> } >> + >> +/** >> + * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ >> + * @chip: the chip the GPIO to lock belongs to >> + * @offset: the offset of the GPIO to lock as IRQ >> + * >> + * This is used directly by GPIO drivers that want to lock down >> + * a certain GPIO line to be used for IRQs. >> + */ >> +int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) >> +{ >> + struct gpio_desc *desc; >> + >> + desc = gpiochip_get_desc(chip, offset); >> + if (IS_ERR(desc)) >> + return PTR_ERR(desc); >> + return gpiodesc_lock_as_irq(desc); >> +} >> EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq); >> >> +int gpiod_enable_irq(struct gpio_desc *desc) >> +{ >> + int irq; >> + int ret; >> + >> + VALIDATE_DESC(desc); >> + >> + irq = gpiod_to_irq(desc); >> + if (irq < 0) >> + return 0; >> + >> + ret = gpiodesc_lock_as_irq(desc); >> + if (!ret) >> + enable_irq(irq); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(gpiod_enable_irq); > > {enable,disable}_irq() are supposed to allow nesting. Do you intent to > preserve the same semantics? This is specially important if you allow > sharing of the interrupt line between devices. This code doesn't preserve it. E.g. calling gpiod_disable_irq twice, followed by gpiod_enable_irq once will mark the GPIO line as being used for an interrupt, even though the irq is not actually enabled yet. I guess gpiodesc_(un)lock_as_irq can be adapter to support this by adding some counter, but for the use-case I have it makes no sense to support nesting. It's very rare that you need to do this, I'm just unlucky that I have two drivers that need this. In both cases because an input gpio pin with an interrupt has to be temporarily switched to an output pin to drive the bus. I could add a WARN_ON if an attempt to nest these calls is detected. Regards, Hans -- 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 Sat, 21 Jul 2018 12:16:42 +0200 Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 21/07/18 12:08, Marc Zyngier wrote: > > Hi Hans, > > > > On Sat, 21 Jul 2018 09:46:19 +0100, > > Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> > >> Hi Linus, > >> > >> On 20/07/18 10:06, Linus Walleij wrote: > >> > >> <snip> > >> > >>> You would probably think it's fine if you didn't have to do this > >>> heavy lifting and could just request them on probe and > >>> call enable_irq() and disable_irq() when needed, and have these > >>> call down into the irq_chip .irq_enable() and .irq_disable() and > >>> lock the GPIO line as input *there* instead, and then change > >>> this everywhere (yes patch all gpio_chips with an irqchip > >>> driver in that case). > >>> > >>> As drivers have likely sometimes already assigned function > >>> pointers to .irq_enable() and .irq_disable() these have to > >>> be copied and stored in struct gpio_irq_chip() by > >>> gpiochip_set_cascaded_irqchip() and called in sequence. > >>> But it can be done. > >>> > >>> What about changing the GPIOLIB_IRQCHIP code to just > >>> do the module_get()/put() part on .irq_request_resources() > >>> and move the locking to the .irq_enable()/.irq_disable() > >>> callbacks so that we can enable and disable irqs on the fly > >>> in a better way? (BIG WIN!) > >>> > >>> What about going and reinvestigating this instead? > >>> > >>> I'm sorry that I can't present any simple solution, but hey, > >>> I presented a really complicated solution instead, isn't it > >>> great! :D > >> > >> I did do some investigation into this, but it made me very unhappy: > >> it's a lot of low-level changes in a lot of drivers for a lot of > >> different boards, some of which are probably hard to test. Scary. > >> > >> But I've come up with a much simpler alternative: add two new > >> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq. > >> > >> An RFC patch is below (documentation is missing, but it works fine). > >> > >> Basically if you have a gpio which has an interrupt, then you can use > >> these functions to disable and enable it. It enables/disables the interrupt > >> and calls gpiochip_(un)lock_as_irq() as well. > >> > >> It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure > >> that the direction is set correctly. I have not done so, but if you think it is > >> useful, then it's trivial to do. Right now it will return an error if the > >> direction is still set to output. > >> > >> If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not > >> sure if that's a bug or a feature. It's easy enough to change. > >> > >> Let me know what you think. IMHO this avoids a huge amount of churn in code you > >> really do no want to touch and it does exactly what you want it to do. And it > >> looks pretty clean as well, both in gpiolib and in the drivers. > >> > >> Regards, > >> > >> Hans > >> > >> Signed-off-by: Hans Verkuil <hansverk@cisco.com> > >> --- > >> drivers/gpio/gpiolib.c | 83 ++++++++++++++++++++++++++--------- > >> include/linux/gpio/consumer.h | 3 ++ > >> 2 files changed, 66 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > >> index e11a3bb03820..75ad6177fa95 100644 > >> --- a/drivers/gpio/gpiolib.c > >> +++ b/drivers/gpio/gpiolib.c > >> @@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc) > >> } > >> EXPORT_SYMBOL_GPL(gpiod_to_irq); > >> > >> -/** > >> - * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ > >> - * @chip: the chip the GPIO to lock belongs to > >> - * @offset: the offset of the GPIO to lock as IRQ > >> - * > >> - * This is used directly by GPIO drivers that want to lock down > >> - * a certain GPIO line to be used for IRQs. > >> - */ > >> -int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) > >> +static int gpiodesc_lock_as_irq(struct gpio_desc *desc) > >> { > >> - struct gpio_desc *desc; > >> - > >> - desc = gpiochip_get_desc(chip, offset); > >> - if (IS_ERR(desc)) > >> - return PTR_ERR(desc); > >> + struct gpio_chip *chip = gpiod_to_chip(desc); > >> > >> /* > >> * If it's fast: flush the direction setting if something changed > >> * behind our back > >> */ > >> if (!chip->can_sleep && chip->get_direction) { > >> - int dir = chip->get_direction(chip, offset); > >> + int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc)); > >> > >> if (dir) > >> clear_bit(FLAG_IS_OUT, &desc->flags); > >> @@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) > >> > >> return 0; > >> } > >> + > >> +/** > >> + * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ > >> + * @chip: the chip the GPIO to lock belongs to > >> + * @offset: the offset of the GPIO to lock as IRQ > >> + * > >> + * This is used directly by GPIO drivers that want to lock down > >> + * a certain GPIO line to be used for IRQs. > >> + */ > >> +int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset) > >> +{ > >> + struct gpio_desc *desc; > >> + > >> + desc = gpiochip_get_desc(chip, offset); > >> + if (IS_ERR(desc)) > >> + return PTR_ERR(desc); > >> + return gpiodesc_lock_as_irq(desc); > >> +} > >> EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq); > >> > >> +int gpiod_enable_irq(struct gpio_desc *desc) > >> +{ > >> + int irq; > >> + int ret; > >> + > >> + VALIDATE_DESC(desc); > >> + > >> + irq = gpiod_to_irq(desc); > >> + if (irq < 0) > >> + return 0; > >> + > >> + ret = gpiodesc_lock_as_irq(desc); > >> + if (!ret) > >> + enable_irq(irq); > >> + return ret; > >> +} > >> +EXPORT_SYMBOL_GPL(gpiod_enable_irq); > > > > {enable,disable}_irq() are supposed to allow nesting. Do you intent to > > preserve the same semantics? This is specially important if you allow > > sharing of the interrupt line between devices. > > This code doesn't preserve it. E.g. calling gpiod_disable_irq twice, followed > by gpiod_enable_irq once will mark the GPIO line as being used for an interrupt, > even though the irq is not actually enabled yet. > > I guess gpiodesc_(un)lock_as_irq can be adapter to support this by adding some > counter, but for the use-case I have it makes no sense to support nesting. I appreciate that your use case is somewhat limited, but if we're introducing a new API, it should be consistent with the rest of the IRQ subsystem. Also, duplicating the counter is likely to lead to all kind of horrible situations if one driver uses your new API, and the other uses the standard {enable,disable}_irq calls (as it ought to be able to -- after all this is "just an interrupt"). > It's very rare that you need to do this, I'm just unlucky that I have > two drivers that need this. In both cases because an input gpio pin > with an interrupt has to be temporarily switched to an output pin to > drive the bus. I'm not sure I quite understand the nature of the problem. What happens to the devices when you switch the GPIO pin to being an output? M.
On Sat, Jul 21, 2018 at 10:46 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > Me: > > You would probably think it's fine if you didn't have to do this > > heavy lifting and could just request them on probe and > > call enable_irq() and disable_irq() when needed, and have these > > call down into the irq_chip .irq_enable() and .irq_disable() and > > lock the GPIO line as input *there* instead, and then change > > this everywhere (yes patch all gpio_chips with an irqchip > > driver in that case). > > > > As drivers have likely sometimes already assigned function > > pointers to .irq_enable() and .irq_disable() these have to > > be copied and stored in struct gpio_irq_chip() by > > gpiochip_set_cascaded_irqchip() and called in sequence. > > But it can be done. > > > > What about changing the GPIOLIB_IRQCHIP code to just > > do the module_get()/put() part on .irq_request_resources() > > and move the locking to the .irq_enable()/.irq_disable() > > callbacks so that we can enable and disable irqs on the fly > > in a better way? (BIG WIN!) > > > > What about going and reinvestigating this instead? > > > > I'm sorry that I can't present any simple solution, but hey, > > I presented a really complicated solution instead, isn't it > > great! :D > > I did do some investigation into this, but it made me very unhappy: > it's a lot of low-level changes in a lot of drivers for a lot of > different boards, some of which are probably hard to test. Scary. > > But I've come up with a much simpler alternative: add two new > gpiolib functions: gpiod_enable_irq and gpiod_disable_irq. I see what you are doing but it is kludgy and makes things messy IMO. It should be possible to just call irq_disable() and have the GPIO line unlocked from the irqchip callback, clean and simple. With this approach we are requireing special semantics and consumers all of a sudden have to know that this IRQ line is on a GPIO, and they did not need to know that before, and should not need to know. It should be just like this: d = devm_gpiod_get(dev, GPIOD_IN); i = gpiod_to_irq(d); devm_request_irq(dev, i); // this locks the GPIO as IRQ (...) disable_irq(i); // this unlocks the GPIO as IRQ gpiod_direction_output(d, 1); (...) gpiod_direction_input(d); enable_irq(i); // this locks the GPIO as IRQ I don't think it's a good idea to interperse this with any GPIO-specific semantics (apart from gpiod_to_irq()) as it only makes things unintuitive. I feel strongly the right way is to make this work. 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
On 23/07/18 00:49, Linus Walleij wrote: > On Sat, Jul 21, 2018 at 10:46 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> Me: >>> You would probably think it's fine if you didn't have to do this >>> heavy lifting and could just request them on probe and >>> call enable_irq() and disable_irq() when needed, and have these >>> call down into the irq_chip .irq_enable() and .irq_disable() and >>> lock the GPIO line as input *there* instead, and then change >>> this everywhere (yes patch all gpio_chips with an irqchip >>> driver in that case). >>> >>> As drivers have likely sometimes already assigned function >>> pointers to .irq_enable() and .irq_disable() these have to >>> be copied and stored in struct gpio_irq_chip() by >>> gpiochip_set_cascaded_irqchip() and called in sequence. >>> But it can be done. >>> >>> What about changing the GPIOLIB_IRQCHIP code to just >>> do the module_get()/put() part on .irq_request_resources() >>> and move the locking to the .irq_enable()/.irq_disable() >>> callbacks so that we can enable and disable irqs on the fly >>> in a better way? (BIG WIN!) >>> >>> What about going and reinvestigating this instead? >>> >>> I'm sorry that I can't present any simple solution, but hey, >>> I presented a really complicated solution instead, isn't it >>> great! :D >> >> I did do some investigation into this, but it made me very unhappy: >> it's a lot of low-level changes in a lot of drivers for a lot of >> different boards, some of which are probably hard to test. Scary. >> >> But I've come up with a much simpler alternative: add two new >> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq. > > I see what you are doing but it is kludgy and makes things > messy IMO. > > It should be possible to just call irq_disable() and have the GPIO > line unlocked from the irqchip callback, clean and simple. > > With this approach we are requireing special semantics and > consumers all of a sudden have to know that this IRQ line is > on a GPIO, and they did not need to know that before, and > should not need to know. > > It should be just like this: > > d = devm_gpiod_get(dev, GPIOD_IN); > i = gpiod_to_irq(d); > devm_request_irq(dev, i); // this locks the GPIO as IRQ > (...) > disable_irq(i); // this unlocks the GPIO as IRQ > gpiod_direction_output(d, 1); > (...) > gpiod_direction_input(d); > enable_irq(i); // this locks the GPIO as IRQ > > I don't think it's a good idea to interperse this with any > GPIO-specific semantics (apart from gpiod_to_irq()) as it only > makes things unintuitive. > > I feel strongly the right way is to make this work. Are you really, really sure about this? As far as I can tell all of these drivers would have to be updated, just for something that is very specific to two CEC drivers: $ git grep -l gpiochip_lock_as_irq Documentation/driver-api/gpio/driver.rst drivers/gpio/gpio-bcm-kona.c drivers/gpio/gpio-dwapb.c drivers/gpio/gpio-em.c drivers/gpio/gpio-tegra.c drivers/gpio/gpio-thunderx.c drivers/gpio/gpio-uniphier.c drivers/gpio/gpio-vr41xx.c drivers/gpio/gpio-xgene-sb.c drivers/gpio/gpiolib-acpi.c drivers/gpio/gpiolib-sysfs.c drivers/gpio/gpiolib.c drivers/hid/hid-cp2112.c drivers/pinctrl/mediatek/mtk-eint.c drivers/pinctrl/pinctrl-st.c drivers/pinctrl/samsung/pinctrl-exynos.c drivers/pinctrl/stm32/pinctrl-stm32.c drivers/pinctrl/sunxi/pinctrl-sunxi.c include/linux/gpio.h include/linux/gpio/driver.h $ git grep -l \\\.irq_enable.*= drivers/gpio/ drivers/gpio/gpio-ath79.c drivers/gpio/gpio-davinci.c drivers/gpio/gpio-dwapb.c drivers/gpio/gpio-ingenic.c drivers/gpio/gpio-lynxpoint.c drivers/gpio/gpio-ml-ioh.c drivers/gpio/gpio-pcf857x.c drivers/gpio/gpio-sta2x11.c drivers/gpio/gpio-thunderx.c drivers/gpio/gpio-timberdale.c drivers/gpio/gpio-zynq.c $ git grep -l \\\.irq_enable.*= drivers/pinctrl/ drivers/pinctrl/bcm/pinctrl-bcm2835.c drivers/pinctrl/bcm/pinctrl-nsp-gpio.c drivers/pinctrl/intel/pinctrl-intel.c drivers/pinctrl/pinctrl-amd.c drivers/pinctrl/pinctrl-coh901.c drivers/pinctrl/pinctrl-rockchip.c drivers/pinctrl/pinctrl-single.c drivers/pinctrl/spear/pinctrl-plgpio.c drivers/pinctrl/sunxi/pinctrl-sunxi.c Furthermore, I can also work around it in the CEC drivers themselves (using gpiod_direction_output_raw or something similar as discussed earlier, or even by just calling free_irq and requesting it again after the CEC calibration finished, i.e. the same that I do today in cec-gpio as well). I just think this is a high-risk approach for something that is rarely needed. In all the years that this has been around this is the first time that this has been requested. And to be honest, I don't think it is something I'm willing to spend that much time on. If there was an expectation that this would be needed more frequently in the future, then that might change, but I don't think that's likely. Personally I think that my proposal is the middle ground between a driver workaround and reworking a lot of gpio drivers. I.e. it's not ideal, but does the job. And if this turns out that this is needed more frequently, then it is always an option to go for the better solution. Regards, Hans -- 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 23/07/18 11:29, Hans Verkuil wrote: > On 23/07/18 00:49, Linus Walleij wrote: >> On Sat, Jul 21, 2018 at 10:46 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: >>> Me: >>>> You would probably think it's fine if you didn't have to do this >>>> heavy lifting and could just request them on probe and >>>> call enable_irq() and disable_irq() when needed, and have these >>>> call down into the irq_chip .irq_enable() and .irq_disable() and >>>> lock the GPIO line as input *there* instead, and then change >>>> this everywhere (yes patch all gpio_chips with an irqchip >>>> driver in that case). >>>> >>>> As drivers have likely sometimes already assigned function >>>> pointers to .irq_enable() and .irq_disable() these have to >>>> be copied and stored in struct gpio_irq_chip() by >>>> gpiochip_set_cascaded_irqchip() and called in sequence. >>>> But it can be done. >>>> >>>> What about changing the GPIOLIB_IRQCHIP code to just >>>> do the module_get()/put() part on .irq_request_resources() >>>> and move the locking to the .irq_enable()/.irq_disable() >>>> callbacks so that we can enable and disable irqs on the fly >>>> in a better way? (BIG WIN!) >>>> >>>> What about going and reinvestigating this instead? >>>> >>>> I'm sorry that I can't present any simple solution, but hey, >>>> I presented a really complicated solution instead, isn't it >>>> great! :D >>> >>> I did do some investigation into this, but it made me very unhappy: >>> it's a lot of low-level changes in a lot of drivers for a lot of >>> different boards, some of which are probably hard to test. Scary. >>> >>> But I've come up with a much simpler alternative: add two new >>> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq. >> >> I see what you are doing but it is kludgy and makes things >> messy IMO. >> >> It should be possible to just call irq_disable() and have the GPIO >> line unlocked from the irqchip callback, clean and simple. >> >> With this approach we are requireing special semantics and >> consumers all of a sudden have to know that this IRQ line is >> on a GPIO, and they did not need to know that before, and >> should not need to know. >> >> It should be just like this: >> >> d = devm_gpiod_get(dev, GPIOD_IN); >> i = gpiod_to_irq(d); >> devm_request_irq(dev, i); // this locks the GPIO as IRQ >> (...) >> disable_irq(i); // this unlocks the GPIO as IRQ >> gpiod_direction_output(d, 1); >> (...) >> gpiod_direction_input(d); >> enable_irq(i); // this locks the GPIO as IRQ >> >> I don't think it's a good idea to interperse this with any >> GPIO-specific semantics (apart from gpiod_to_irq()) as it only >> makes things unintuitive. >> >> I feel strongly the right way is to make this work. I fully agree here. Creating a gpio-specific API that doesn't interoperate with the normal IRQ API is just creating a absolute mess. On the other hand, seamlessly integrating the GPIO configuration as part of the IRQ API makes complete sense. > Are you really, really sure about this? As far as I can tell all of these drivers > would have to be updated, just for something that is very specific to two CEC drivers: [...] And that's absolutely fine. we do bulk changes in existing drivers all the time, specially if this leads to a better integration of close subsystems. > Furthermore, I can also work around it in the CEC drivers themselves (using > gpiod_direction_output_raw or something similar as discussed earlier, or even > by just calling free_irq and requesting it again after the CEC calibration > finished, i.e. the same that I do today in cec-gpio as well). > > I just think this is a high-risk approach for something that is rarely needed. > In all the years that this has been around this is the first time that this > has been requested. I can see two options: either we support something fully, in a way that is integrated with the rest of the kernel, or we don't. There is enough horrible hacks we have to live with already. > And to be honest, I don't think it is something I'm willing to spend that > much time on. If there was an expectation that this would be needed more > frequently in the future, then that might change, but I don't think that's > likely. There is obviously a need, since someone managed to design a piece of HW that requires something that doesn't fit in the current model. The real question is whether we want to support it or not. I'm definitely willing to add support for it, but not at the cost of a third rate kludge. > Personally I think that my proposal is the middle ground between a driver > workaround and reworking a lot of gpio drivers. I.e. it's not ideal, but > does the job. And if this turns out that this is needed more frequently, > then it is always an option to go for the better solution. As one of the folks who would end-up maintaining the resulting mess, my answer is "no, thank you". Linus has outlined something that makes sense for both the GPIO and IRQ subsystems. I appreciate this would take time and effort, but supporting silly HW comes at that cost. Thanks, M.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e11a3bb03820..dcdb457b83b0 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1822,6 +1822,20 @@ static void gpiochip_irq_relres(struct irq_data *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)); +} + +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); +} + static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) { if (!gpiochip_irqchip_irq_valid(chip, offset)) @@ -1897,6 +1911,10 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, !irqchip->irq_release_resources) { irqchip->irq_request_resources = gpiochip_irq_reqres; irqchip->irq_release_resources = gpiochip_irq_relres; + if (!irqchip->irq_enable && !irqchip->irq_disable) { + irqchip->irq_enable = gpiochip_irq_enable; + irqchip->irq_disable = gpiochip_irq_disable; + } } if (gpiochip->irq.parent_handler) { @@ -2056,6 +2074,10 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, !irqchip->irq_release_resources) { irqchip->irq_request_resources = gpiochip_irq_reqres; irqchip->irq_release_resources = gpiochip_irq_relres; + if (!irqchip->irq_enable && !irqchip->irq_disable) { + irqchip->irq_enable = gpiochip_irq_enable; + irqchip->irq_disable = gpiochip_irq_disable; + } } acpi_gpiochip_request_interrupts(gpiochip);
If a gpio pin has an interrupt associated with it, but you need to set the direction of the pin to output, then what you want to do is to disable the interrupt and change direction to output. And after changing the direction back to input, then you call enable_irq again. This does not work today since FLAG_USED_AS_IRQ is set when the interrupt is first requested and gpiod_direction_output() checks for that. So the only option is to free the interrupt and re-request it, which is painful. This RFC patch sets hooks that allow the driver to know when the irq is disabled and enabled and it sets FLAG_USED_AS_IRQ accordingly. It works, but I have one open question: When gpiochip_irq_enable() is called, I currently just WARN if gpiochip_lock_as_irq() fails and continue after that. It really is a driver bug, but I wonder if it is also possible to automatically change the direction to input before enabling the interrupt. I have no idea how to do this, though, since gpiod_direction_input() needs a struct gpio_desc whereas in irq_enable() I have a struct gpio_chip. This patch would be really useful for two CEC drivers: In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and re-request the irq (cec_gpio_dis/enable_irq()). This driver implements low-level support for the HDMI CEC bus. When waiting for something to happen the gpio pin is set to input with an interrupt to avoid polling. When writing to the bus it is set to output and the interrupt obviously has to be removed (or, as I would like to see, just disabled). Requesting an irq can fail, and handling that is really problematic. Just disabling and enabling is much easier and cleaner. The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the tda998x_cec_calibration() function: this is also a CEC driver but here the tda998x interrupt pin is overloaded: when calibrating the CEC line the interrupt pin of the chip is used as a gpio calibration output signal. This driver disables the interrupt and switches the direction to output before doing the calibration. This fails for the BeagleBone board unless I apply this patch. This driver was originally developed for the dove-cubox board which uses this gpio driver: drivers/gpio/gpio-mvebu.c. For some reason this worked fine (i.e. it is apparently possible to disable the irq and change the output without running into the FLAG_USED_AS_IRQ check), but I don't understand why. I don't have this hardware, so I can't test it. I'm a newbie when it comes to gpio, so I have no idea how much sense this patch makes. Regards, Hans Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> --- -- 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