Re: Should irq_enable/disable be flagging/unflagging gpio lines used as irq?

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Tue, Jul 11, 2017 at 7:55 PM, Davide Hug <d@xxxxxxxxxx> wrote:

> I'm looking at a driver (hwmon/sht15) that does not load with message:
>
>         gpio-48 (SHT15 data): _gpiod_direction_output_raw: tried to set a GPIO tied to an IRQ as output
>
> This happens because the driver uses a GPIO line alternately (and not at the
> same time) as IRQ and output. To do this irq_enable/disable are used to
> prevent setting a GPIO with enabled IRQ as output. This seems the right
> approach to me since in the patch "gpio: add API to be strict about GPIO IRQ
> usage"[1] I read:
>
>         The API is symmetric so that lines can also be flagged from
>         .irq_enable() and unflagged from IRQ by .irq_disable().
>
> But I didn't find any GPIO driver implementing this API in irq_enable/disable.
>
> Since my platform uses omap_gpio which does not implement
> irq_enable/disable at all, I added a default irq_enable/disable
> implementation in gpio/gpiolib.c (in the patch below).
>
> Is it the right approach to call gpiochip_lock_as_irq/gpiochip_unlock_as_irq

We already call this in gpiochip_irq_reqres() and gpiochip_irq_relres().
from the .irq_request_resources()/.irq_release_resources() callbacks.

Your patch makes us call the functions twice I think.

Why isn't gpiochip_irq_relres() called before you reuse the line
for something else than IRQ?

If there is a legitimate reason why .irq_disable() and .irq_enable()
are called but not .irq_request_resources() and .irq_release_resources()
I want to know why that is so.

TGLX implemented these functions in the first place exactly to
be used for gpiochip_lock_as_irq()/gpiochip_unlock_as_irq(), see:
commit c1bacbae8192dd2a9ebadd22d793b68054f6c6e5
"genirq: Provide irq_request/release_resources chip callbacks"

Then we switch in
commit 57ef04288abd27a717287a652d324f95cb77c3c6
"gpio: switch drivers to use new callback"

At the time we were calling this from .irq_startup() and .irq_shutdown()
and I am afraid that .irq_enable() and .irq_disable() will just recreate
the same problem we had in 2014.

Yours,
Linus Walleij
--
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