On Wed, Apr 17, 2013 at 4:00 AM, Jon Hunter <jon-hunter@xxxxxx> 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@xxxxxx> 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@xxxxxxxxxxxxxxx> >> 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@xxxxxxxxxxxxxxx> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
