Re: [RFC PATCH] gpiolib: call gpiochip_(un)lock_as_irq for dis/enable_irq()

[Marc Zyngier <marc.zyngier@xxxxxxx>]

On Sat, 21 Jul 2018 12:16:42 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> wrote:

> On 21/07/18 12:08, Marc Zyngier wrote:
> > Hi Hans,
> > 
> > On Sat, 21 Jul 2018 09:46:19 +0100,
> > Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >>  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.
-- 
Without deviation from the norm, progress is not possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html