[PATCH 3/5] gpio/omap: Add DT support to GPIO driver
Javier Martinez Canillas
martinez.javier at gmail.com
Wed Apr 17 17:55:50 EST 2013
On Wed, Apr 17, 2013 at 4:00 AM, Jon Hunter <jon-hunter at ti.com> wrote:
>
> On 04/16/2013 07:41 PM, Javier Martinez Canillas wrote:
>> On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter <jon-hunter at ti.com> wrote:
>>>
>>> On 04/16/2013 05:11 PM, Stephen Warren wrote:
>>>> On 04/16/2013 01:27 PM, Jon Hunter wrote:
>>>>>
>>>>> On 04/16/2013 01:40 PM, Stephen Warren wrote:
>>>>>> On 04/15/2013 05:04 PM, Jon Hunter wrote:
>>>> ...
>>>>>>> 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.
>>>>
>>>> I rather dislike this approach since:
>>>>
>>>> a) It requires changes to the DT bindings, which are already defined.
>>>> Admittedly it's backwards-compatible, but still.
>>>>
>>>> b) There isn't really any need for the DT to represent this; the
>>>> GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and
>>>> vice-versa (if the HW has such a concept), so there's no need for the DT
>>>> to contain this information. This seems like pushing Linux's internal
>>>> requirements into the design of the DT binding.
>>>
>>> Yes, so the only alternative is to use irq_to_gpio to avoid this.
>>>
>>>> c) I have the feeling that hooking the of_xlate function for this is a
>>>> bit of an abuse of the function.
>>>
>>> I was wondering about that. So I was grep'ing through the various xlate
>>> implementations and found this [1]. Also you may recall that in the
>>> of_dma_simple_xlate() we call the dma_request_channel() to allocate the
>>> channel, which is very similar. However, I don't wish to get a
>>> reputation as abusing APIs so would be good to know if this really is
>>> abuse or not ;-)
>>>
>>> Cheers
>>> Jon
>>>
>>> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
>>
>> I was looking at [1] shared by Jon and come up with the following
>> patch that does something similar for OMAP GPIO. This has the
>> advantage that is local to gpio-omap instead changing gpiolib-of and
>> also doesn't require DT changes
>>
>> But I don't want to get a reputation for abusing APIs neither :-)
>>
>> Best regards,
>> Javier
>>
>> From 23368eb72b125227fcf4b22be19ea70b4ab94556 Mon Sep 17 00:00:00 2001
>> From: Javier Martinez Canillas <javier.martinez at collabora.co.uk>
>> Date: Wed, 17 Apr 2013 02:03:08 +0200
>> Subject: [PATCH 1/1] gpio/omap: add custom xlate function handler
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez at collabora.co.uk>
>> ---
>> drivers/gpio/gpio-omap.c | 29 ++++++++++++++++++++++++++++-
>> 1 files changed, 28 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 8524ce5..77216f9 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -1097,6 +1097,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank)
>> static const struct of_device_id omap_gpio_match[];
>> static void omap_gpio_init_context(struct gpio_bank *p);
>>
>> +static int omap_gpio_irq_domain_xlate(struct irq_domain *d,
>> + struct device_node *ctrlr,
>> + const u32 *intspec, unsigned int intsize,
>> + irq_hw_number_t *out_hwirq,
>> + unsigned int *out_type)
>> +{
>> + int ret;
>> + struct gpio_bank *bank = d->host_data;
>> + int gpio = bank->chip.base + intspec[0];
>> +
>> + if (WARN_ON(intsize < 2))
>> + return -EINVAL;
>> +
>> + ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name);
>> + if (ret)
>> + return ret;
>> +
>> + *out_hwirq = intspec[0];
>> + *out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
>> +
>> + return 0;
>> +}
>> +
>> +static struct irq_domain_ops omap_gpio_irq_ops = {
>> + .xlate = omap_gpio_irq_domain_xlate,
>> +};
>> +
>> static int omap_gpio_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -1144,7 +1171,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_gpio_irq_ops, bank);
>> if (!bank->domain)
>> return -ENODEV;
>>
>
> That would work for omap, in fact I posted pretty much the same thing a
> day ago [1] ;-)
>
Hi Jon,
There are so many patches flying around in this thread that I missed it :-)
Sorry about that...
> I was trying to see if we could find a common solution that everyone
> could use as it seems that ideally we should all be requesting the gpio.
>
> Cheers
> Jon
>
> [1] http://marc.info/?l=linux-arm-kernel&m=136606204823845&w=1
btw, I shared the latest patch with only build testing it, but today I
gave a try and I found a problem with this approach. The .xlate
function is being called twice for each GPIO-IRQ so the first time
gpio_request_one() succeeds but the second time it fails returning
-EBUSY.
This raises a warning on drivers/of/platform.c
(WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq)):
0.285308] ------------[ cut here ]------------
[ 0.285369] WARNING: at drivers/of/platform.c:171
of_device_alloc+0x154/0x168()
[ 0.285430] Modules linked in:
[ 0.285491] [<c001a944>] (unwind_backtrace+0x0/0xf0) from
[<c0041edc>] (warn_slowpath_common+0x4c/0x68)
[ 0.285552] [<c0041edc>] (warn_slowpath_common+0x4c/0x68) from
[<c0041f14>] (warn_slowpath_null+0x1c/0x24)
[ 0.285614] [<c0041f14>] (warn_slowpath_null+0x1c/0x24) from
[<c041ac3c>] (of_device_alloc+0x154/0x168)
[ 0.285675] [<c041ac3c>] (of_device_alloc+0x154/0x168) from
[<c041ac84>] (of_platform_device_create_pdata+0x34/0x80)
[ 0.285736] [<c041ac84>]
(of_platform_device_create_pdata+0x34/0x80) from [<c0027364>]
(gpmc_probe_generic_child+0x180/0x240)
[ 0.285827] [<c0027364>] (gpmc_probe_generic_child+0x180/0x240)
from [<c00278d8>] (gpmc_probe+0x4b4/0x614)
[ 0.285888] [<c00278d8>] (gpmc_probe+0x4b4/0x614) from [<c0325514>]
(platform_drv_probe+0x18/0x1c)
[ 0.285949] [<c0325514>] (platform_drv_probe+0x18/0x1c) from
[<c0324354>] (driver_probe_device+0x108/0x21c)
[ 0.286010] [<c0324354>] (driver_probe_device+0x108/0x21c) from
[<c0322b2c>] (bus_for_each_drv+0x5c/0x88)
[ 0.286071] [<c0322b2c>] (bus_for_each_drv+0x5c/0x88) from
[<c0324218>] (device_attach+0x78/0x90)
[ 0.286132] [<c0324218>] (device_attach+0x78/0x90) from
[<c0323868>] (bus_probe_device+0x88/0xac)
[ 0.286193] [<c0323868>] (bus_probe_device+0x88/0xac) from
[<c03220e8>] (device_add+0x4b0/0x584)
[ 0.286254] [<c03220e8>] (device_add+0x4b0/0x584) from [<c041acac>]
(of_platform_device_create_pdata+0x5c/0x80)
[ 0.286315] [<c041acac>]
(of_platform_device_create_pdata+0x5c/0x80) from [<c041ad9c>]
(of_platform_bus_create+0xcc/0x278)
[ 0.286376] [<c041ad9c>] (of_platform_bus_create+0xcc/0x278) from
[<c041adfc>] (of_platform_bus_create+0x12c/0x278)
[ 0.286468] [<c041adfc>] (of_platform_bus_create+0x12c/0x278) from
[<c041afa8>] (of_platform_populate+0x60/0x98)
[ 0.286529] [<c041afa8>] (of_platform_populate+0x60/0x98) from
[<c070c5f8>] (omap_generic_init+0x24/0x60)
[ 0.286590] [<c070c5f8>] (omap_generic_init+0x24/0x60) from
[<c06fe4c4>] (customize_machine+0x1c/0x28)
[ 0.286651] [<c06fe4c4>] (customize_machine+0x1c/0x28) from
[<c00086a4>] (do_one_initcall+0x34/0x178)
[ 0.286712] [<c00086a4>] (do_one_initcall+0x34/0x178) from
[<c06fb8f4>] (kernel_init_freeable+0xfc/0x1c8)
[ 0.286804] [<c06fb8f4>] (kernel_init_freeable+0xfc/0x1c8) from
[<c04e4668>] (kernel_init+0x8/0xe4)
[ 0.286865] [<c04e4668>] (kernel_init+0x8/0xe4) from [<c0013170>]
(ret_from_fork+0x14/0x24)
[ 0.287139] ---[ end trace d49d2507892be205 ]---
I probably won't have time to dig further on this until later this
week but I wanted to share with you in case you know why is being
calling twice and if you thought about a solution.
It works if I don't check the return gpio_request_one() (or better if
we don't return on omap_gpio_irq_domain_xlate) but of course is not
the right solution.
Thanks a lot and best regards,
Javier
More information about the devicetree-discuss
mailing list