Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Stephen Warren <swarren@wwwdotorg.org>]

On 04/15/2013 03:40 PM, Jon Hunter wrote:
> 
> On 04/15/2013 11:58 AM, Stephen Warren wrote:
>> On 04/14/2013 02:53 PM, Linus Walleij wrote:
>>> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
>>> <martinez.javier@gmail.com> wrote:
>>>
>>>> Is the following inlined patch [1] what you were thinking that would
>>>> be the right approach?
>>>
>>> This looks sort of OK, but I'm still struggling with the question of
>>> what we could do to help other implementations facing the same issue.
>>>
>>> This is a pretty hard design pattern to properly replicate in every such
>>> driver is it not?
>>
>> Well, instead of adding .request_irq() to the irqchip, and then making
>> each driver call gpio_request() from the implementation, perhaps you
>> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
>> and if it gets back a non-error response, the IRQ core could call
>> gpio_request(). That means that the change to each GPIO+IRQ driver is
>> simply to implement a standalone data transformation function
>> .irq_to_gpio().
> 
> I am still concerned about the case where a driver may have already
> called gpio_request() and then calls request_irq(). I think that the
> solution needs to handle cases where the driver may or may not call
> gpio_request() to allocate the gpio.

Are there actually drivers that do this? Perhaps they could just be
fixed not to.

> Although it could be argued that this is problem is not DT specific,
> it does become a bigger problem to handle in the case of DT. Therefore,
> I am wondering if we should just focus on the DT case for now. 

That doesn't sound like a good idea; this issue is entirely orthogonal
to DT.

>> Now, this does re-introduce irq_to_gpio() in some way, but with the
>> following advantages:
>>
>> 1) The implementation is per-controller, not a single global function,
>> so isn't introducing any kind of centralized mapping scheme again.
>>
>> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for
>> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
>> Its potential existence doesn't imply that all IRQ chips need implement
>> this; it would be very specifically be for this one particular case.
>>
>> So, I think it's reasonable to introduce this.
> 
> How about using the gpio irq domain xlate function?

That translates DT IRQ-specifiers to Linux IRQ numbers. There's no
reason to believe that, as an absolute rule, it would work for anything
GPIO-related. The fact that in practice most GPIO+IRQ controllers happen
to use the same numbering for GPIOs and IRQs is just co-incidence.

> Typically, in DT land a device using a gpio as an interrupt source
> will have something like the following ...
> 
> eth@0 {
> 	compatible = "ks8851";
> 	...
> 	interrupt-parent = <&gpio2>;
> 	interrupts = <2 8>; /* gpio line 34, low triggered */
> };

OK, that really is an interrupt...

> ... or ...
> 
> mmc {
> 	label = "pandaboard::status2";
>  	gpios = <&gpio1 8 0>;
> 	...
> };

But that's a gpio-leds instance, not an MMC controller... I really
really hope there's no DT node using "gpios" to mean "interrupts"... And
it wouldn't make any sense for an on-SoC device anyway.

> Both these devices are using a gpio as an interrupt source, but the mmc
> driver is requesting the gpio directly. In the first case the xlate
> function for the gpio irq domain will be called where as it is not used
> in the 2nd case. Therefore, we could add a custom xlate function. For
> example ...
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c

> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
...
> +       gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name);

I guess that could work, but:

a) It still means doing the gpio_request() in each driver instead of
centrally.

b) This approach doesn't solve the issue where some client driver has
already requested the GPIO. This code would simply prevent that call
from succeeding, which would probably make the driver probe() error out,
which isn't any different to the equivalent proposed centralized
gpio_request() inside some request_irq() failing, and causing the
device's probe() to error out.