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

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

On 03/22/2013 10:33 AM, Stephen Warren wrote:
> On 03/22/2013 02:10 AM, Linus Walleij wrote:
>> On Fri, Mar 15, 2013 at 12:21 PM, Javier Martinez Canillas
>> <martinez.javier@gmail.com> wrote:
>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index 159f5c5..f5feb43 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -807,6 +807,13 @@ static void gpio_unmask_irq(struct irq_data *d)
>>>         spin_unlock_irqrestore(&bank->lock, flags);
>>>  }
>>>
>>> +static int gpio_irq_request(struct irq_data *d)
>>> +{
>>> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>>> +
>>> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
>>> +}
>>> +
>>>  static struct irq_chip gpio_irq_chip = {
>>>         .name           = "GPIO",
>>>         .irq_shutdown   = gpio_irq_shutdown,
>>> @@ -815,6 +822,7 @@ static struct irq_chip gpio_irq_chip = {
>>>         .irq_unmask     = gpio_unmask_irq,
>>>         .irq_set_type   = gpio_irq_type,
>>>         .irq_set_wake   = gpio_wake_enable,
>>> +       .irq_request    = gpio_irq_request,
>>>  };
>>
>> This is just turning everything on it's head. The normal order of things
>> is this sequence for GPIO 14 something like:
>>
>> gpio_request(14);
>> int irq = gpio_to_irq(14);
>> request_any_context_irq(irq);
> 
> That doesn't make any sense at all. Drivers don't want to care whether
> the IRQ number they receive is a GPIO-that-can-act-like-an-IRQ or a pure
> dedicated IRQ input.
> 
> To fully support the model you proprose, a driver would have to:
> 
> if (gpio_is_valid(pdata->gpio)) {
> 	gpio_request_one(pdata->gpio, GPIOF_IN, "foo irq");
> 	irq = gpio_to_irq(pdata->gpio);
> } else
> 	irq = pdata->irq;
> request_irq(...);
> 
> which means complex code in each driver, plus requiring some indication
> in platform data and/or device tree re: whether the IRQ is hosted by a
> GPIO or not.

I tend to agree with Stephen here. When we had discussed this previously
you had mentioned about adding a platform function to request the gpio
[1], but I could see this adding a bunch more platform files when we are
trying to get rid of all the board-xxx.c files for OMAP. So if you don't
like the approach suggested by Stephen and implemented by Javier, then
may be we need to means to request/reserve gpios in the dts as you had
suggested before [1].

So it seems to me that there are two options at the moment ...

1. Add a irq_chip request as suggested by Stephen.
2. Reserve/request gpios in the dts when registering a gpio chip.

Cheers
Jon

[1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/92327