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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

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.

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html