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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

On 23/07/18 00:49, Linus Walleij wrote:
> 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.

Are you really, really sure about this? As far as I can tell all of these drivers
would have to be updated, just for something that is very specific to two CEC drivers:

$ git grep -l gpiochip_lock_as_irq
Documentation/driver-api/gpio/driver.rst
drivers/gpio/gpio-bcm-kona.c
drivers/gpio/gpio-dwapb.c
drivers/gpio/gpio-em.c
drivers/gpio/gpio-tegra.c
drivers/gpio/gpio-thunderx.c
drivers/gpio/gpio-uniphier.c
drivers/gpio/gpio-vr41xx.c
drivers/gpio/gpio-xgene-sb.c
drivers/gpio/gpiolib-acpi.c
drivers/gpio/gpiolib-sysfs.c
drivers/gpio/gpiolib.c
drivers/hid/hid-cp2112.c
drivers/pinctrl/mediatek/mtk-eint.c
drivers/pinctrl/pinctrl-st.c
drivers/pinctrl/samsung/pinctrl-exynos.c
drivers/pinctrl/stm32/pinctrl-stm32.c
drivers/pinctrl/sunxi/pinctrl-sunxi.c
include/linux/gpio.h
include/linux/gpio/driver.h

$ git grep -l \\\.irq_enable.*= drivers/gpio/
drivers/gpio/gpio-ath79.c
drivers/gpio/gpio-davinci.c
drivers/gpio/gpio-dwapb.c
drivers/gpio/gpio-ingenic.c
drivers/gpio/gpio-lynxpoint.c
drivers/gpio/gpio-ml-ioh.c
drivers/gpio/gpio-pcf857x.c
drivers/gpio/gpio-sta2x11.c
drivers/gpio/gpio-thunderx.c
drivers/gpio/gpio-timberdale.c
drivers/gpio/gpio-zynq.c

$ git grep -l \\\.irq_enable.*= drivers/pinctrl/
drivers/pinctrl/bcm/pinctrl-bcm2835.c
drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
drivers/pinctrl/intel/pinctrl-intel.c
drivers/pinctrl/pinctrl-amd.c
drivers/pinctrl/pinctrl-coh901.c
drivers/pinctrl/pinctrl-rockchip.c
drivers/pinctrl/pinctrl-single.c
drivers/pinctrl/spear/pinctrl-plgpio.c
drivers/pinctrl/sunxi/pinctrl-sunxi.c

Furthermore, I can also work around it in the CEC drivers themselves (using
gpiod_direction_output_raw or something similar as discussed earlier, or even
by just calling free_irq and requesting it again after the CEC calibration
finished, i.e. the same that I do today in cec-gpio as well).

I just think this is a high-risk approach for something that is rarely needed.
In all the years that this has been around this is the first time that this
has been requested.

And to be honest, I don't think it is something I'm willing to spend that
much time on. If there was an expectation that this would be needed more
frequently in the future, then that might change, but I don't think that's
likely.

Personally I think that my proposal is the middle ground between a driver
workaround and reworking a lot of gpio drivers. I.e. it's not ideal, but
does the job. And if this turns out that this is needed more frequently,
then it is always an option to go for the better solution.

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