Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Joel Fernandes]

On 09/10/2013 04:23 PM, Javier Martinez Canillas wrote:
> On 09/10/2013 09:52 PM, Stephen Warren wrote:
>> On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
>> ...
>>> The only thing that this patch tries to solve is when a driver expect to request
>>> a IRQ and it doesn't care if is a real IRQ line from an interrupt controller or
>>> a GPIO pin mapped as an IRQ.
>>
>> That can be solved in the interrupt chip driver. The fact the previous
>> attempt didn't work doesn't mean that it's impossible.
>>
> 
> Indeed. Unfortunately that patch-set got merged as a fix quite late on the -rc
> cycle so it was safer to just revert the patches instead of analyzing the
> regression and providing a fix.
> 
> If Linus is fond about taking a local fix for gpio-omap then we can try again
> as a RFC with a better test coverage than before (although the patches had
> several tested and acked-by but no one tested on OMAP1 until the patches were
> merged) getting some TI folks in the loop who have access to those ancient OMAP1
> devices. That way we can repost as a patch just once we are definitely sure that
> it won't cause a regression on any OMAP platform.
> 
>>> With board files we used to explicitly do the GPIO setup
>>> (gpio_{request,direction_input}) but with DT these board files are gone and we
>>> need a way to setup a GPIO implicitly when is mapped as an IRQ.
>>
>> Well, that's just an example of hacking around something in a board file
>> that should have been fixed in the GPIO/IRQ controller.
>>
>>> That's the only use case that this patch covers.
>> ...
>>> Also, it would be great if we can find a temporary solution so we can finally
>>> have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
>>> the mentioned change on gpiolib would take at least a couple of kernel releases.
>>
>> Really? I thought this patch was error-checking to make sure that
>> different drivers didn't request a GPIO and an IRQ that are actually the
>> same signal. This patch shouldn't affect any functionality except in
>> cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
>> driver).
>>
> 
> Yes, it doesn't do any error-checking neither prevent a driver to request a GPIO
> and an IRQ that are the same signal (as long as this is supported by the
> GPIO/IRQ controller and its driver). The only thing that Linus patch does is to
> auto request a GPIO for pins that are mapped as IRQ in DT (i.e: interrupt-parent
> = <&gpio6>).
> 
> The name of the function introduced by Linus is
> of_gpiochip_interrupt_consistency_check() but probably a better name is
> of_gpiochip_autorequest_irq() or something like that.
> 
> A similar behavior could be obtained by let's say calling gpio_request() in
> irq_create_of_mapping() if you wanted to add the logic in the IRQ chip DT core
> instead of doing it in the GPIO chip DT core as Linus does with his patch.
> 
> That's why I don't understand why Linus patch could be an issue for drivers that
> needs to request both the GPIO and the mapped IRQ.
> 
> If this is already supported then nothing will break. If the driver tries to
> request the GPIO twice because this is already made by the DT core then I think
> is a bug in the driver and has to be fixed to support DT since there won't be
> any need to do this manually anymore.

Quick question- Wouldn't this mean that there have to be 2 code paths in the
driver then.. One for DT-case where gpio_request double request is prevented,
and one for non-DT case where you would do the gpio_request followed by
request_irq. I'm not sure if there are drivers today that use DT and need to
converted to prevent the double request? If there are, and such drivers are also
used on non-DT platform, then we would have to fork 2 different code paths while
requesting an IRQ for DT/non-DT in the driver..

Also this path kind of enforces that the driver author must be aware whether
driver is being used on DT or non-DT platform.. Linus mentioned enforcing of
semantics, this is also enforcing semantics no?

Looks like the correct fix for this as discussed in this thread should be to
associate the struct device with the GPIO request, remember it and use the info
during request_irq. This is provided that the old pattern of gpio_request and
request_irq is continued to be used and working. I know this is some time away
so I'm not too opinionated about it.

Regards,

-Joel




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/