[PATCH] gpio: convince line to become input in irq helper

[PATCH] gpio: convince line to become input in irq helper

[Linus Walleij]

The generic IRQ helper library just checks if the IRQ line is
set as input before activating it for interrupts. As we
recently started to check things better with .get_dir() it
turns out that it's good to try to convince the line to become
an input before attempting to lock it as IRQ.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 58d822d7e8da..b9a6a5c69fbb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1010,6 +1010,23 @@ static int gpiochip_irq_reqres(struct irq_data *d)
 	if (!try_module_get(chip->gpiodev->owner))
 		return -ENODEV;
 
+	/*
+	 * If it is possible to switch this GPIO to an input
+	 * this is a good time to do it.
+	 */
+	if (chip->direction_input) {
+		struct gpio_desc *desc;
+		int ret;
+
+		desc = gpiochip_get_desc(chip, d->hwirq);
+		if (IS_ERR(desc))
+			return PTR_ERR(desc);
+
+	        ret = chip->direction_input(chip, d->hwirq);
+		if (!ret)
+			clear_bit(FLAG_IS_OUT, &desc->flags);
+	}
+
 	if (gpiochip_lock_as_irq(chip, d->hwirq)) {
 		chip_err(chip,
 			"unable to lock HW IRQ %lu for IRQ\n",
-- 
2.4.11

Re: [PATCH] gpio: convince line to become input in irq helper

[Bjorn Andersson]

On Wed 22 Jun 07:33 PDT 2016, Linus Walleij wrote:

> The generic IRQ helper library just checks if the IRQ line is
> set as input before activating it for interrupts. As we
> recently started to check things better with .get_dir() it
> turns out that it's good to try to convince the line to become
> an input before attempting to lock it as IRQ.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

I really like this.

> ---
>  drivers/gpio/gpiolib.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 58d822d7e8da..b9a6a5c69fbb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1010,6 +1010,23 @@ static int gpiochip_irq_reqres(struct irq_data *d)
>  	if (!try_module_get(chip->gpiodev->owner))
>  		return -ENODEV;
>  
> +	/*
> +	 * If it is possible to switch this GPIO to an input
> +	 * this is a good time to do it.
> +	 */
> +	if (chip->direction_input) {
> +		struct gpio_desc *desc;
> +		int ret;
> +
> +		desc = gpiochip_get_desc(chip, d->hwirq);
> +		if (IS_ERR(desc))
> +			return PTR_ERR(desc);
> +
> +	        ret = chip->direction_input(chip, d->hwirq);
> +		if (!ret)
> +			clear_bit(FLAG_IS_OUT, &desc->flags);

With this updated semantics I think it's a mistake to not propagate this
error. Clients will not be able to rely on it and have to continue
configuring their pins as input just to be sure.

> +	}
> +
>  	if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>  		chip_err(chip,
>  			"unable to lock HW IRQ %lu for IRQ\n",

Regards,
Bjorn

Re: [PATCH] gpio: convince line to become input in irq helper

[Linus Walleij]

On Wed, Jun 22, 2016 at 7:12 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Wed 22 Jun 07:33 PDT 2016, Linus Walleij wrote:
>
>> The generic IRQ helper library just checks if the IRQ line is
>> set as input before activating it for interrupts. As we
>> recently started to check things better with .get_dir() it
>> turns out that it's good to try to convince the line to become
>> an input before attempting to lock it as IRQ.
>>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> I really like this.

Thanks :)

>> +     /*
>> +      * If it is possible to switch this GPIO to an input
>> +      * this is a good time to do it.
>> +      */
>> +     if (chip->direction_input) {
>> +             struct gpio_desc *desc;
>> +             int ret;
>> +
>> +             desc = gpiochip_get_desc(chip, d->hwirq);
>> +             if (IS_ERR(desc))
>> +                     return PTR_ERR(desc);
>> +
>> +             ret = chip->direction_input(chip, d->hwirq);
>> +             if (!ret)
>> +                     clear_bit(FLAG_IS_OUT, &desc->flags);
>
> With this updated semantics I think it's a mistake to not propagate this
> error. Clients will not be able to rely on it and have to continue
> configuring their pins as input just to be sure.

OK good point I'll propagate it. But it will only be used by those
driver utilizing the generic IRQchip helper.

It's especially for the usecase where you take an IRQ off a DT
resource without gpiod_to_irq() first.

Yours,
Linus Walleij