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

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

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.

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. 

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

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 */
};

... or ...

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

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
index 53bb8d5..caaeab2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1085,6 +1085,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
        irq_set_handler_data(bank->irq, bank);
 }
 
+
+int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
+                         const u32 *intspec, unsigned int intsize,
+                         unsigned long *out_hwirq, unsigned int *out_type)
+{
+       struct gpio_bank *bank;
+       int irq;
+
+       if (WARN_ON(intsize < 1))
+               return -EINVAL;
+
+       irq = irq_find_mapping(d, intspec[0]);
+       bank = irq_get_chip_data(irq);
+       if (!bank)
+               return -ENODEV;
+
+       gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name);
+
+       *out_hwirq = intspec[0];
+       *out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
+       return 0;
+}
+
+static const struct irq_domain_ops omap_irq_domain_ops = {
+       .xlate = omap_irq_domain_xlate,
+};
+
 static const struct of_device_id omap_gpio_match[];
 static void omap_gpio_setup_context(struct gpio_bank *p);
 
@@ -1135,7 +1162,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 
 
        bank->domain = irq_domain_add_linear(node, bank->width,
-                                            &irq_domain_simple_ops, NULL);
+                                            &omap_irq_domain_ops, NULL);
        if (!bank->domain)
                return -ENODEV;
 
Cheers
Jon