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

[Jon Hunter <jon-hunter@ti.com>]

On 04/16/2013 01:40 PM, Stephen Warren wrote:
> On 04/15/2013 05:04 PM, Jon Hunter wrote:
>>
>> On 04/15/2013 05:16 PM, Stephen Warren wrote:
>>> On 04/15/2013 03:40 PM, Jon Hunter wrote:
> ...
>>>> 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.
>>
>> Oops yes, I overlooked that. However, the omap mmc driver
>> (drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and
>> request_threaded_irq() for the mmc card-detect interrupt. I believe
>> tegra is doing the same ...
>>
>> sdhci@78000000 {
>> 	...
>> 	cd-gpios = <&gpio 69 0>; /* gpio PI5 */
>> 	...	
>> };
> 
> Ah true. I guess at least all MMC drivers are likely hooking cd-gpios as
> an interrupt, /and/ requesting it as a GPIO so that they can read the
> current state when the GPIO goes off.
> 
> That tends to imply that no core code can possibly call gpio_request()
> in response to request_irq(), since doing so likely will conflict with
> quite a few drivers...

Yes that was my concern.

>>>> 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.
>>
>> Right this is device specific, but it avoids exposing irq_to_gpio for a
>> device. However, we could make this generic if we did expose irq_to_gpio
>> for each device.
>>
>>> 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.
>>
>> If some driver is calling gpio_request() directly, then they will most
>> likely just call gpio_to_irq() when requesting the interrupt and so the
>> xlate function would not be called in this case (mmc drivers are a good
>> example). So I don't see that as being a problem. In fact that's the
>> benefit of this approach as AFAICT it solves this problem.
> 
> Oh. That assumption seems very fragile. What about drivers that actually
> do have platform data (or DT bindings) that provide both the IRQ and
> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible.

Right. In the DT case though, if someone does provide the IRQ and GPIO
IDs then at least they would use a different xlate function. Another
option to consider would be defining the #interrupt-cells = <3> where we
would have ...

cell-#1 --> IRQ domain ID
cell-#2 --> Trigger type
cell-#3 --> GPIO ID

Then we could have a generic xlate for 3 cells that would also request
the GPIO. Again not sure if people are against a gpio being requested in
the xlate but just an idea. Or given that irq_of_parse_and_map() calls
the xlate, we could have this function call gpio_request() if the
interrupt controller is a gpio and there are 3 cells.

> Given all this, I guess simply having each GPIO+IRQ driver's set_type
> function simply do whatever is required in HW to set up that GPIO
> actually does seem like the best idea. Admittedly that isn't
> centralized, but I'm not sure now that any centralized implementation is
> possible, without significant rework of a bunch of drivers. This is what
> the Tegra GPIO driver already does, and I think one of the earlier
> patches in this thread did exactly that for OMAP IRQs too right?

Yes, however, Linus wanted us to make sure the gpio is requested which
is why we have not taken that patch. However, if we cannot find a better
alternative may be we have to do that for now.

> BTW, just so you know, I'm on vacation for 2 weeks starting Wed
> afternoon, so replies will be non-existent or spotty during that time.

Thanks!
Jon