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

[Marc Zyngier <marc.zyngier@xxxxxxx>]

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.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.
--
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