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

[Marc Zyngier <marc.zyngier@xxxxxxx>]

On 23/07/18 11:29, Hans Verkuil wrote:
> 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.

I fully agree here. Creating a gpio-specific API that doesn't
interoperate with the normal IRQ API is just creating a absolute mess.
On the other hand, seamlessly integrating the GPIO configuration as part
of the IRQ API makes complete sense.

> 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:

[...]

And that's absolutely fine. we do bulk changes in existing drivers all
the time, specially if this leads to a better integration of close
subsystems.

> 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.

I can see two options: either we support something fully, in a way that
is integrated with the rest of the kernel, or we don't. There is enough
horrible hacks we have to live with already.

> 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.

There is obviously a need, since someone managed to design a piece of HW
that requires something that doesn't fit in the current model. The real
question is whether we want to support it or not.

I'm definitely willing to add support for it, but not at the cost of a
third rate kludge.

> 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.

As one of the folks who would end-up maintaining the resulting mess, my
answer is "no, thank you". Linus has outlined something that makes sense
for both the GPIO and IRQ subsystems. I appreciate this would take time
and effort, but supporting silly HW comes at that cost.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
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