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

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

On 04/15/2013 04: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.
> 
> 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);

By the way, I know that I should check the return code here, but this
was just an example. Also I don't think using ctrlr->name here works
either as this is just "gpio".

Jon