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] ;-) 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 -- 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
