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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Sat, Jul 21, 2018 at 10:46 AM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> Me:
> > 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.

I see what you are doing but it is kludgy and makes things
messy IMO.

It should be possible to just call irq_disable() and have the GPIO
line unlocked from the irqchip callback, clean and simple.

With this approach we are requireing special semantics and
consumers all of a sudden have to know that this IRQ line is
on a GPIO, and they did not need to know that before, and
should not need to know.

It should be just like this:

d = devm_gpiod_get(dev, GPIOD_IN);
i = gpiod_to_irq(d);
devm_request_irq(dev, i); // this locks the GPIO as IRQ
(...)
disable_irq(i); // this unlocks the GPIO as IRQ
gpiod_direction_output(d, 1);
(...)
gpiod_direction_input(d);
enable_irq(i); // this locks the GPIO as IRQ

I don't think it's a good idea to interperse this with any
GPIO-specific semantics (apart from gpiod_to_irq()) as it only
makes things unintuitive.

I feel strongly the right way is to make this work.

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