| Message ID | CAAwP0s1CFRO-rf==p5sYs1n0gigMif7wuT8Yv0N_iLUDMrtcOg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); I understand that the approach take make things easier for device tree but it screws up the semantics of the entire kernel (the lockdep etc warnings are just a symptom), we are locked to the above semantic sequence: you know the GPIO, *then* you get the IRQ, not the other way around. irq_to_gpio has been deemed a legacy thing that we want to get rid of. The reason being that it is ambiguous on most every GPIO expander out there. It is not supported by GPIOLIB. This is the reason to why the implementation in the GPIOLIB in <linux/gpio.h> looks like this: static inline int irq_to_gpio(unsigned int irq) { return -EINVAL; } The only chance to have the function is in a non-gpiolib platform that override it. The irq_to_gpio() in gpio-omap.c overrides this and should cause compile warnings, does it not? There is a reason every other driver in drivers/gpio has renamed this function to things like pxa_irq_to_gpio, msm_irq_to_gpio: we just don't support it generically. I know I'm not very helpful, I can just say this is not going to work. :-/ It seems you need a way to get the GPIO corresponding to a certain IRQ from the device tree, not from the kernel as is done here, then follow the mentioned sequence. Yours, Linus Walleij
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.
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
On Fri, Mar 22, 2013 at 11:52 PM, Jon Hunter <jon-hunter@ti.com> wrote: > On 03/22/2013 10:33 AM, Stephen Warren wrote: >> On 03/22/2013 02:10 AM, Linus Walleij wrote: >>> 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 suspect the above is just referring to the DT or similar usecases (such as ACPI)? We already have a number of platforms passing IRQ numbers for something that is actually a GPIO pin but we don't really know and debugfs doesn't say because they never issue gpio_request() of the pin. And that's something I don't like. Because that is loosing control over the GPIO numberspace not quite knowing which pin is used and which one isn't and making these things prone to bugs. What worries me is changing kernel semantics to fit device tree, I think it is better to try to confine this to the drivers/gpio/gpiolib-of.c file. I'd like OF GPIO code to hog the pin using gpio_request(), then figure out the IRQ number using gpio_to_irq() and pass that out when picking an IRQ right out of a GPIO controllers DT node. And I'd like it to be compulsory exercise to list that one GPIO line as an input hog when used for just some IRQ line. This is what I would want to achieve by the GPIO hog concept. > 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. This seems like you need to bring in Grant for a second opinion I'm getting confused by this now... Paging Alexandre who's doing the GPIO descriptor refactoring as well. > [1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/92327 Is there anything wrong with the GPIO hog concept as presented in the mentioned reply? + input-hog-gpios = <4>, <5>; Would result in these GPIOs being requested, so we can keep track of them in e.g. drivers/gpio/gpiolib-of.c, then supply IRQs from these hogged inputs on demand from drivers. But with the requirement that they be hogged, so we can keep track of them, and they show up with some sensible name in <debugfs>/gpio. Yours, Linus Walleij
On 03/27/2013 07:52 AM, Linus Walleij wrote: > On Fri, Mar 22, 2013 at 11:52 PM, Jon Hunter <jon-hunter@ti.com> wrote: >> On 03/22/2013 10:33 AM, Stephen Warren wrote: >>> On 03/22/2013 02:10 AM, Linus Walleij wrote: >>>> 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 suspect the above is just referring to the DT or similar usecases > (such as ACPI)? ... > What worries me is changing kernel semantics to fit device tree, > I think it is better to try to confine this to the drivers/gpio/gpiolib-of.c > file. No, this has absolutely noting to do with device tree; the example I gave above is purely based on platform data. It's simply that if a device emits an IRQ, there's no reason to assume that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin and not something that gpiolib (or any other GPIO API) knows about. One possibility: (Device IRQ output wired into GPIO input on SoC which then generates an interrupt) +----------------------------+ | SoC | +--------+ | IRQ 5 +------+ GPIO 10 | DEV_IRQ | Device | | GIC <---- | GPIO | <-------|---------|-IRQ | | +------+ | +--------+ +----------------------------+ Another possibility: (Device IRQ output wired directly into dedicated IRQ input pin, and that pin has no GPIO capabilities) +----------------------------+ | SoC | +--------+ | IRQ 5 | DEV_IRQ | Device | | GIC <----------------------|---------|-IRQ | | | +--------+ +----------------------------+ As such, the driver /must/ receive an "int irq" in platform data. If it received an "int gpio", then there would be no way for the driver to support boards that routed that interrupt signal to a dedicated IRQ pin on the SoC, rather than to a GPIO pin that just happened to have interrupt generation capabilities. And then, given that irq_to_gpio() isn't semantically possible, there's no way for the driver to be able to gpio_request() anything, since it doesn't, can't, and shouldn't know whether there is in fact any GPIO ID that it should request, let alone what that GPIO ID is. And finally, even if we ignore dedicated IRQ input pins, and assume that every IRQ input is really a GPIO, why should the driver be forced to call both request_irq() and gpio_request(); that's something that should really be dealt with inside the IRQ subsystem or relevant IRQ driver. Otherwise, every other driver that uses IRQs has to be burdened with the code to do that.
On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > It's simply that if a device emits an IRQ, there's no reason to assume > that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin > and not something that gpiolib (or any other GPIO API) knows about. OK (following the first example, that's what I have in the Nomadik or Snowball already). > Another possibility: > > (Device IRQ output wired directly into dedicated IRQ input pin, and that > pin has no GPIO capabilities) > > +----------------------------+ > | SoC | +--------+ > | IRQ 5 | DEV_IRQ | Device | > | GIC <----------------------|---------|-IRQ | > | | +--------+ > +----------------------------+ > > As such, the driver /must/ receive an "int irq" in platform data. Nah. Lets pass a struct resource * of type IORESOURCE_IRQ. But I understand the necessity to pass the IRQ number somehow. > If it > received an "int gpio", then there would be no way for the driver to > support boards that routed that interrupt signal to a dedicated IRQ pin > on the SoC, rather than to a GPIO pin that just happened to have > interrupt generation capabilities. This is the case for some SMSC911x clients like the snowball routing it to a GPIO, whereas I think the RealView and Integrator/CP has a dedicated IRQ line for it on the FPGA so it's a good example. In such cases the right thing to do is for the platform code populating the platform data with the int irq field, or device tree core, or whatever piece of code that knows about the actual GPIO shall start from the GPIO, request it and then convert it to an IRQ. If it seems like identical boilerplate in every machine we can simply centralize it to gpiolib. Something like int gpio_add_single_irq_resource(struct platform_device *pdev, int gpio, unsigned flags) { struct resource *r; int irq; int err; r = devm_kmalloc(&pdev->dev, sizeof(*r), GFP_KERNEL); if (!r) return -ENOMEM; memset(r, 0, sizeof(*r)); err = request_gpio(gpio); if (err) return err; irq = gpio_to_irq(gpio); if (irq < 0) return irq; r->start = irq; r->end = irq; r->flags = IORESOURCE_IRQ | flags; pdev->num_resources = 1; pdev->resource = res; }; int gpio_populate_irq_resource(struct platform_device *pdev, int gpio, unsigned flags, struct resource *r) { int irq; int err; err = request_gpio(gpio); if (err) return err; irq = gpio_to_irq(gpio); if (irq < 0) return irq; r->start = irq; r->end = irq; r->flags = IORESOURCE_IRQ | flags; }; (Add permutations for either filling in a struct resource *r from the board code, or krealloc() to extend an existing dynamic allocation or whatever, surely these helpers can be written.) So this is for a pure platform data case (based on arch/arm/mach-ux500/board-mop500.c, which currently relies on hard-coded GPIO and IRQ numbers): static struct resource ethernet_res[] = { { .name = "smsc911x-memory", .start = (0x5000 << 16), .end = (0x5000 << 16) + 0xffff, .flags = IORESOURCE_MEM, }, { }, /* To be filled in with GPIO IRQ */ }; static struct platform_device ethernet = { .name = "smsc911x", .dev = { .platform_data = &...., }, }; gpio_populate_irq_resource(ðernet, 17, IORESOURCE_IRQ_HIGHEDGE, ðernet_res[1]); platform_device_register(ðernet); That populates the device with the single GPIO IRQ from the GPIO number in a smooth way. Of course it has to be done after the GPIO driver is up and running so may require some other refactoring, but it should work. I can implement this and convert the Snowball if it helps. > And then, given that irq_to_gpio() isn't semantically possible, there's > no way for the driver to be able to gpio_request() anything, since it > doesn't, can't, and shouldn't know whether there is in fact any GPIO ID > that it should request, let alone what that GPIO ID is. That's cool. The driver does not have to know the IRQ is backed by a GPIO, but somewhere in the system something should know this. Such as board code or DT core (I'm thinking drivers/of/platform.c code calling gpiolib-of.c to do this mapping for DT, for example). And by introducing the requirement that such GPIO lines be GPIO input hogs in the device tree else warn we can get a nice closure in the DT case: the GPIO subsystem knows the actual GPIO line as a hog that is request and sets as input at probe, the referencing device node only gets the IRQ but at runtime the device node instatiation for the IRQ consumer will ask the gpiolib to check its hog list to make sure one of these will match that IRQ, then request it. > And finally, even if we ignore dedicated IRQ input pins, and assume that > every IRQ input is really a GPIO, why should the driver be forced to > call both request_irq() and gpio_request(); that's something that should > really be dealt with inside the IRQ subsystem or relevant IRQ driver. > Otherwise, every other driver that uses IRQs has to be burdened with the > code to do that. I agree it need not be in the driver if all it wants is an IRQ. I was thinking more like the above. Yours, Linus Walleij
On 03/27/2013 02:55 PM, Linus Walleij wrote: > On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >> It's simply that if a device emits an IRQ, there's no reason to assume >> that the IRQ is in fact a GPIO. It might be a dedicated IRQ input pin >> and not something that gpiolib (or any other GPIO API) knows about. > > OK (following the first example, that's what I have in the Nomadik > or Snowball already). > >> Another possibility: >> >> (Device IRQ output wired directly into dedicated IRQ input pin, and that >> pin has no GPIO capabilities) >> >> +----------------------------+ >> | SoC | +--------+ >> | IRQ 5 | DEV_IRQ | Device | >> | GIC <----------------------|---------|-IRQ | >> | | +--------+ >> +----------------------------+ >> >> As such, the driver /must/ receive an "int irq" in platform data. > > Nah. Lets pass a struct resource * of type IORESOURCE_IRQ. > But I understand the necessity to pass the IRQ number somehow. Oh sure. I tend to consider the resources part of platform data, although I agree they really aren't. >> If it >> received an "int gpio", then there would be no way for the driver to >> support boards that routed that interrupt signal to a dedicated IRQ pin >> on the SoC, rather than to a GPIO pin that just happened to have >> interrupt generation capabilities. > > This is the case for some SMSC911x clients like the snowball > routing it to a GPIO, whereas I think the RealView and Integrator/CP > has a dedicated IRQ line for it on the FPGA so it's a good example. > > In such cases the right thing to do is for the platform > code populating the platform data with the int irq field, or device tree > core, or whatever piece of code that knows about > the actual GPIO shall start from the GPIO, request it and > then convert it to an IRQ. So board code could easily do that; plenty of opportunity to put whatever custom code is needed right there. For DT core code to do that, we'd need some alternative way of specifying interrupts. That would change the DT bindings for interrupts. I don't think we can do that... > If it seems like identical boilerplate in every machine we can > simply centralize it to gpiolib. Something like > > int gpio_add_single_irq_resource(struct platform_device *pdev, int > gpio, unsigned flags) ... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it] > int gpio_populate_irq_resource(struct platform_device *pdev, int gpio, > unsigned flags, struct resource *r) ... [code to create IORESOURCE_IRQ from the IRQ] ... > gpio_populate_irq_resource(ðernet, 17, > IORESOURCE_IRQ_HIGHEDGE, ðernet_res[1]); > platform_device_register(ðernet); > > That populates the device with the single GPIO IRQ from > the GPIO number in a smooth way. > > Of course it has to be done after the GPIO driver is up > and running so may require some other refactoring, > but it should work. That latter issue also somewhat scuppers this approach; then you have to somehow run a bunch of your board file code inter-mixed with various driver probe() calls. That will quickly get nasy. And it doesn't address how the DT core will know when to call gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without changing the DT binding for interrupts. >> And then, given that irq_to_gpio() isn't semantically possible, there's >> no way for the driver to be able to gpio_request() anything, since it >> doesn't, can't, and shouldn't know whether there is in fact any GPIO ID >> that it should request, let alone what that GPIO ID is. > > That's cool. The driver does not have to know the IRQ is backed by a > GPIO, but somewhere in the system something should know this. Something already does; the irqchip driver for the IRQ in question would always know whether it's programming a plain IRQ controller, or an IRQ controller that's part of a GPIO controller. In the GPIO case, the irqchip driver also has enought HW-specific information to perform whatever GPIO<->IRQ number conversion is needed. > Such as board code or DT core (I'm thinking drivers/of/platform.c code > calling gpiolib-of.c to do this mapping for DT, for example). > > And by introducing the requirement that such GPIO lines be > GPIO input hogs in the device tree else warn we can get a nice > closure in the DT case: the GPIO subsystem knows the actual > GPIO line as a hog that is request and sets as input at probe, > the referencing device node only gets the IRQ but at runtime > the device node instatiation for the IRQ consumer will ask > the gpiolib to check its hog list to make sure one of these will > match that IRQ, then request it. "Hogs" sounds like pinctrl. pinctrl doesn't support the concept of hogs on GPIOs, since it only knows about pins not GPIOs really; even with the integration with gpiolib, pinctrl gets informed when GPIOs are requested, not when setting up pinctrl states ahead of time, and also this notification is only so we can mark the pin related to the GPIO in-use, not really to actually hog it. If pinctrl started hogging GPIOs, I think me might end up with both pinctrl and gpiolib calling each-other, and hence have nasty circular dependencies. Or, were you thinking of creating some kind of hogging system for gpiolib itself?
On Fri, Mar 29, 2013 at 6:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 03/27/2013 02:55 PM, Linus Walleij wrote: >> On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> >> This is the case for some SMSC911x clients like the snowball >> routing it to a GPIO, whereas I think the RealView and Integrator/CP >> has a dedicated IRQ line for it on the FPGA so it's a good example. >> >> In such cases the right thing to do is for the platform >> code populating the platform data with the int irq field, or device tree >> core, or whatever piece of code that knows about >> the actual GPIO shall start from the GPIO, request it and >> then convert it to an IRQ. > > So board code could easily do that; plenty of opportunity to put > whatever custom code is needed right there. > > For DT core code to do that, we'd need some alternative way of > specifying interrupts. That would change the DT bindings for interrupts. > I don't think we can do that... Sorry, I'm probably not knowledgeable about DT to understand this. The information for what GPIO corresponds to what IRQ is present in the device tree, is it not? If the information is there, whether to convert from IRQ to GPIO or from GPIO to IRQ is a technicality and any order should be feasible in some way? I just can't get the necessity to do it one way preferred over the other through my head, sorry... >> If it seems like identical boilerplate in every machine we can >> simply centralize it to gpiolib. Something like >> >> int gpio_add_single_irq_resource(struct platform_device *pdev, int >> gpio, unsigned flags) > ... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it] > >> int gpio_populate_irq_resource(struct platform_device *pdev, int gpio, >> unsigned flags, struct resource *r) > ... [code to create IORESOURCE_IRQ from the IRQ] > > ... >> gpio_populate_irq_resource(ðernet, 17, >> IORESOURCE_IRQ_HIGHEDGE, ðernet_res[1]); >> platform_device_register(ðernet); >> >> That populates the device with the single GPIO IRQ from >> the GPIO number in a smooth way. >> >> Of course it has to be done after the GPIO driver is up >> and running so may require some other refactoring, >> but it should work. > > That latter issue also somewhat scuppers this approach; then you have to > somehow run a bunch of your board file code inter-mixed with various > driver probe() calls. That will quickly get nasy. No, just use a module_init() for the above code in the board file and it will defer just like any other initcall. > And it doesn't address how the DT core will know when to call > gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without > changing the DT binding for interrupts. Is it not possible to do this in drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know* which GPIOs will be used as plain IRQs? That is the point with the gpio hogs I talk about ... GPIO hogs would hog input pins here and immediately call gpio_to_irq() on them, and from that point on they can only be used for input, and only for generating IRQs. >>> And then, given that irq_to_gpio() isn't semantically possible, there's >>> no way for the driver to be able to gpio_request() anything, since it >>> doesn't, can't, and shouldn't know whether there is in fact any GPIO ID >>> that it should request, let alone what that GPIO ID is. >> >> That's cool. The driver does not have to know the IRQ is backed by a >> GPIO, but somewhere in the system something should know this. > > Something already does; the irqchip driver for the IRQ in question would > always know whether it's programming a plain IRQ controller, or an IRQ > controller that's part of a GPIO controller. In the GPIO case, the > irqchip driver also has enought HW-specific information to perform > whatever GPIO<->IRQ number conversion is needed. So I'm opposed to doing these things both ways because I think it's getting messy. We have already has irq_to_gpio() cause problems in the past and this reminds me heavily about that. > Or, were you thinking of creating some kind of hogging system for > gpiolib itself? Yes. Yours, Linus Walleij
On 04/10/2013 12:12 PM, Linus Walleij wrote: > On Fri, Mar 29, 2013 at 6:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 03/27/2013 02:55 PM, Linus Walleij wrote: >>> On Wed, Mar 27, 2013 at 5:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> This is the case for some SMSC911x clients like the snowball >>> routing it to a GPIO, whereas I think the RealView and Integrator/CP >>> has a dedicated IRQ line for it on the FPGA so it's a good example. >>> >>> In such cases the right thing to do is for the platform >>> code populating the platform data with the int irq field, or device tree >>> core, or whatever piece of code that knows about >>> the actual GPIO shall start from the GPIO, request it and >>> then convert it to an IRQ. >> >> So board code could easily do that; plenty of opportunity to put >> whatever custom code is needed right there. >> >> For DT core code to do that, we'd need some alternative way of >> specifying interrupts. That would change the DT bindings for interrupts. >> I don't think we can do that... > > Sorry, I'm probably not knowledgeable about DT to understand this. > The information for what GPIO corresponds to what IRQ is > present in the device tree, is it not? No, I don't think so. DT treats the IRQ and GPIO numbering spaces as entirely unrelated things, just like the kernel APIs do. The fact that some DT nodes (device/drivers) happen to be both GPIO and IRQ providers (implement gpio_chip and irq_chip) and happen to use the same numbering scheme for both GPIO and IRQ IDs is purely an implementation detail within individual drivers (or rather, of specific DT binding definitions really), and not a general truism. > If the information is there, whether to convert from IRQ to GPIO > or from GPIO to IRQ is a technicality and any order should be > feasible in some way? There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put another way, a single GPIO would likely only ever trigger a single IRQ, but a single IRQ might easily be triggered by any number of GPIOs. This is exactly why the function irq_to_gpio() isn't something one should use any more. I think there was an effort to outright remove the API, although it doesn't look like that's entirely complete yet. I guess you know this given your mentions of problem with gpio_to_irq() later on, so I'm not quite sure what your paragraph above was supposed to mean. > I just can't get the necessity to do it one way preferred over the > other through my head, sorry... > >>> If it seems like identical boilerplate in every machine we can >>> simply centralize it to gpiolib. Something like >>> >>> int gpio_add_single_irq_resource(struct platform_device *pdev, int >>> gpio, unsigned flags) >> ... [code to convert GPIO to IRQ and create IORESOURCE_IRQ from it] >> >>> int gpio_populate_irq_resource(struct platform_device *pdev, int gpio, >>> unsigned flags, struct resource *r) >> ... [code to create IORESOURCE_IRQ from the IRQ] >> >> ... >>> gpio_populate_irq_resource(ðernet, 17, >>> IORESOURCE_IRQ_HIGHEDGE, ðernet_res[1]); >>> platform_device_register(ðernet); >>> >>> That populates the device with the single GPIO IRQ from >>> the GPIO number in a smooth way. >>> >>> Of course it has to be done after the GPIO driver is up >>> and running so may require some other refactoring, >>> but it should work. >> >> That latter issue also somewhat scuppers this approach; then you have to >> somehow run a bunch of your board file code inter-mixed with various >> driver probe() calls. That will quickly get nasy. > > No, just use a module_init() for the above code in the board > file and it will defer just like any other initcall. Using initcalls to order things is rather fragile. Perhaps it could work out OK with board files, but it's certainly something that's frowned upon now for DT-based systems, since deferred probe solves it much more generally, and without any maintenance issues (e.g. continual re-shuffling of the initcall order, not having enough initcall levels if you end up with more dependencies being added, etc.). >> And it doesn't address how the DT core will know when to call >> gpio_add_single_irq_resource() vs. gpio_populate_irq_resource() without >> changing the DT binding for interrupts. > > Is it not possible to do this in > drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know* > which GPIOs will be used as plain IRQs? That is the point with > the gpio hogs I talk about ... I suppose that if you modified the device tree bindings (schema definitions), you could have every GPIO provider DT node indicate which GPIOs were used as IRQs instead. However, a) This would be an incompatible change to the DT bindings, and they're supposed to be a stable ABI; old DTBs should work unmodified with new kernels. b) This would place GPIO usage information in multiple places. Right now, GPIO consumers list which GPIO(s) they use. The same goes for IRQs. If a GPIO provider started listing which GPIOs were really IRQs, then that'd mean configuring each GPIO-as-an-IRQ in two places; once in the client node to indicate which IRQ it should hook/register, and once in the provider node to indicate that the GPIO is really an IRQ. Two places is more hassle for people to implement, and more opportunity for mistakes.
On Wed, Apr 10, 2013 at 10:29 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/10/2013 12:12 PM, Linus Walleij wrote: >> If the information is there, whether to convert from IRQ to GPIO >> or from GPIO to IRQ is a technicality and any order should be >> feasible in some way? > > There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put > another way, a single GPIO would likely only ever trigger a single IRQ, > but a single IRQ might easily be triggered by any number of GPIOs. This > is exactly why the function irq_to_gpio() isn't something one should use > any more. I think there was an effort to outright remove the API, > although it doesn't look like that's entirely complete yet. I guess you > know this given your mentions of problem with gpio_to_irq() later on, so > I'm not quite sure what your paragraph above was supposed to mean. So the only reason I'm rambing on about this is that it breaks the connection between GPIOs and their IRQs and at one point I percieved it as there was going to be some IRQ -> GPIO lookup, and it would sneak back in. But now I realize that may have been supposed to be something local to the driver, in it's irqchip portions and then it's actually no problem for the kernel at large. Let's restate: I'm also after something fragile here. IIUC, it will be possible for a GPIO to be set up as input and used as an IRQ without the GPIO subsystem knowing it. And it will be possible for the GPIO subsystem to go in and request the same pin and set it as output and e.g. bit-bang it. I wonder what happens then. If the drive code is written in such a way that this will be denied, I'll soften up, but I'd like to see the code first. Then the same mistake has to be avoided in all drivers. And that makes me want to probe for solutions tying them up in the core so no mistakes can be made. >> Is it not possible to do this in >> drivers/gpio/gpiolib-of.c: of_gpiochip_add(), if the DT *know* >> which GPIOs will be used as plain IRQs? That is the point with >> the gpio hogs I talk about ... > > I suppose that if you modified the device tree bindings (schema > definitions), you could have every GPIO provider DT node indicate which > GPIOs were used as IRQs instead. However, > > a) This would be an incompatible change to the DT bindings, and they're > supposed to be a stable ABI; old DTBs should work unmodified with new > kernels. > > b) This would place GPIO usage information in multiple places. Right > now, GPIO consumers list which GPIO(s) they use. The same goes for IRQs. > If a GPIO provider started listing which GPIOs were really IRQs, then > that'd mean configuring each GPIO-as-an-IRQ in two places; once in the > client node to indicate which IRQ it should hook/register, and once in > the provider node to indicate that the GPIO is really an IRQ. Two places > is more hassle for people to implement, and more opportunity for mistakes. Yeah but the current approach AFAICT isn't exactly safe either. So I need some more convincing... or code rather, I guess. If I can't point out the problems I see in a piece of code I guess I'm just plain wrong :-) Yours, Linus Walleij
On Wednesday 10 April 2013 14:29:17 Stephen Warren wrote: > > > If the information is there, whether to convert from IRQ to GPIO > > or from GPIO to IRQ is a technicality and any order should be > > feasible in some way? > > There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put > another way, a single GPIO would likely only ever trigger a single IRQ, > but a single IRQ might easily be triggered by any number of GPIOs. This > is exactly why the function irq_to_gpio() isn't something one should use > any more. More importantly, most irqs don't have any GPIO associated with them at all. The interface made some sense for simple platforms that had a linear range of GPIO IRQs, but that also isn't true these days, with arbitrary IRQ domains. > I think there was an effort to outright remove the API, > although it doesn't look like that's entirely complete yet. It's essentially complete. There are a few remnants in areas of the kernel that we don't like to look at: * The blackfin architecture provides the interface and uses it in the pata_rb532_cf driver. * MIPS provides it on a few older platforms, and it's used on the alchemy pcmcia driver. * avr32, m68k, sh and unicore32 provide the interface but don't use it. * On ARM, the interface is provided on iop, gemini, ixp4xx, ks8695 and w90x900. None of these use it, and they rarely see any patches at all these days. * ARM/davinci provides a stub but always return -ENOSYS. * One driver for PXA (tosa_battery) still uses this interface, but PXA stopped providing it long ago. Arnd
On 04/10/2013 03:28 PM, Linus Walleij wrote: > On Wed, Apr 10, 2013 at 10:29 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/10/2013 12:12 PM, Linus Walleij wrote: > >>> If the information is there, whether to convert from IRQ to GPIO >>> or from GPIO to IRQ is a technicality and any order should be >>> feasible in some way? >> >> There isn't always a unique 1:1 mapping between GPIOs and IRQs. Put >> another way, a single GPIO would likely only ever trigger a single IRQ, >> but a single IRQ might easily be triggered by any number of GPIOs. This >> is exactly why the function irq_to_gpio() isn't something one should use >> any more. I think there was an effort to outright remove the API, >> although it doesn't look like that's entirely complete yet. I guess you >> know this given your mentions of problem with gpio_to_irq() later on, so >> I'm not quite sure what your paragraph above was supposed to mean. > > So the only reason I'm rambing on about this is that it breaks the I'm not sure I understand this paragraph; what is "it" in the line above. If "it" is this patch, then should "breaks" be re-establishes? > connection between GPIOs and their IRQs and at one point > I percieved it as there was going to be some IRQ -> GPIO lookup, > and it would sneak back in. But now I realize that may have been > supposed to be something local to the driver, in it's irqchip portions > and then it's actually no problem for the kernel at large. Yes, I believe the intention was for their to be a correlation between GPIOs and IRQs only with the individual gpio_chip/irq_chip drivers for those GPIOs/IRQs, and not exposed anywhere outside it. > Let's restate: I'm also after something fragile here. I assume you mean the opposite of that? > IIUC, it will be possible for a GPIO to be set up as input and used > as an IRQ without the GPIO subsystem knowing it. And it will be > possible for the GPIO subsystem to go in and request the same pin > and set it as output and e.g. bit-bang it. I wonder what happens then. Yes, I think that's possible now. If I recall the patch I'm replying to correctly, the idea was to add an "IRQ request" operation that would (internally to the GPIO/IRQ driver itself) map IRQ->GPIO, and call gpio_request(). That would then prevent exactly the situation you mention above.
On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/10/2013 03:28 PM, Linus Walleij wrote: >> So the only reason I'm rambing on about this is that it breaks the > > I'm not sure I understand this paragraph; what is "it" in the line above. > > If "it" is this patch, then should "breaks" be re-establishes? No I'm replying to Javier Martinez Canillas mail in this thread: http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2 which is stating a question to grand, and contains the below hunk: > +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"); > +} irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio(). It's the same thing you mention below: > If I recall the patch I'm replying to correctly, the idea was to add an > "IRQ request" operation that would (internally to the GPIO/IRQ driver > itself) map IRQ->GPIO, and call gpio_request(). That would then prevent > exactly the situation you mention above. So the above does not look like it's internal to the driver does it? I think this is irq_to_gpio sneaking back in, and requiring that every driver of this sort follow the same design pattern. And then maybe you see my persistance ... are we talking about different things? So I'd be happy with something local to the driver, foo_irq_to_gpio() and that we also back out a bit and see what the subsystem can do to avoid this kind of code having to be duplicated everywhere, that's all. Yours, Linus Walleij
On 04/11/2013 04:16 PM, Linus Walleij wrote: > On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/10/2013 03:28 PM, Linus Walleij wrote: > >>> So the only reason I'm rambing on about this is that it breaks the >> >> I'm not sure I understand this paragraph; what is "it" in the line above. >> >> If "it" is this patch, then should "breaks" be re-establishes? > > No I'm replying to Javier Martinez Canillas mail in this thread: > http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2 > which is stating a question to grand, and contains the below > hunk: > >> +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"); >> +} > > irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio(). OK, right. sorry for being so confused/confusing. Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not irq_to_gpio(). Probably gpio_irq_request() wants renaming to something more like omap_gpio__irq_request() too, so the names don't look like they'd clash with global functions. (__ added for clarity but probably only one _ at a time)
On Fri, Apr 12, 2013 at 12:16 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/10/2013 03:28 PM, Linus Walleij wrote: > >>> So the only reason I'm rambing on about this is that it breaks the >> >> I'm not sure I understand this paragraph; what is "it" in the line above. >> >> If "it" is this patch, then should "breaks" be re-establishes? > > No I'm replying to Javier Martinez Canillas mail in this thread: > http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2 > which is stating a question to grand, and contains the below > hunk: > >> +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"); >> +} > > irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio(). > > It's the same thing you mention below: > >> If I recall the patch I'm replying to correctly, the idea was to add an >> "IRQ request" operation that would (internally to the GPIO/IRQ driver >> itself) map IRQ->GPIO, and call gpio_request(). That would then prevent >> exactly the situation you mention above. > > So the above does not look like it's internal to the driver does it? > > I think this is irq_to_gpio sneaking back in, and requiring that every > driver of this sort follow the same design pattern. And then maybe > you see my persistance ... are we talking about different things? > > So I'd be happy with something local to the driver, foo_irq_to_gpio() > and that we also back out a bit and see what the subsystem can do > to avoid this kind of code having to be duplicated everywhere, that's > all. > Hi Linus, Thanks a lot for your explanation. I didn't know that irq_to_gpio() was being deprecated and we shouldn't use anymore. Now from this thread discussion I understand the reasons for this decision. I'll read the whole thread again and try to implement something that is local to the gpio-omap driver instead using irq_to_gpio(). Besides using a controller specific mapping instead of irq_to_gpio(), do you think that is a good idea to add a new "irq_request" function pointer to struct irq_chip? The idea is that GPIO controller drivers can call gpio_request() and enabling the GPIO bank module before a call to request_irq() is made. Or do you think that is better to implement the DT gpio hogs that you were suggesting? I really want to find a solution to this since currently we can't use any device that uses an GPIO line as an IRQ on OMAP based boards. > Yours, > Linus Walleij Best regards, Javier
On 04/11/2013 04:16 PM, Linus Walleij wrote: > On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/10/2013 03:28 PM, Linus Walleij wrote: > >>> So the only reason I'm rambing on about this is that it breaks the >> >> I'm not sure I understand this paragraph; what is "it" in the line above. >> >> If "it" is this patch, then should "breaks" be re-establishes? > > No I'm replying to Javier Martinez Canillas mail in this thread: > http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2 > which is stating a question to grand, and contains the below > hunk: > >> +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"); >> +} > > irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio(). > > It's the same thing you mention below: > >> If I recall the patch I'm replying to correctly, the idea was to add an >> "IRQ request" operation that would (internally to the GPIO/IRQ driver >> itself) map IRQ->GPIO, and call gpio_request(). That would then prevent >> exactly the situation you mention above. OK, right. sorry for being so confused/confusing. Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not irq_to_gpio(). Probably gpio_irq_request() wants renaming to something more like omap_gpio__irq_request() too, so the names don't look like they'd clash with global functions. (__ added for clarity but probably only one _ at a time)
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, }; /*---------------------------------------------------------------------*/ diff --git a/include/linux/irq.h b/include/linux/irq.h index bc4e066..2aeaa24 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -303,6 +303,7 @@ struct irq_chip { void (*irq_shutdown)(struct irq_data *data); void (*irq_enable)(struct irq_data *data); void (*irq_disable)(struct irq_data *data); + int (*irq_request)(struct irq_data *data); void (*irq_ack)(struct irq_data *data); void (*irq_mask)(struct irq_data *data); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index fa17855..07c20f7 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1093,6 +1093,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (!shared) { init_waitqueue_head(&desc->wait_for_threads); + if (desc->irq_data.chip->irq_request) { + ret = desc->irq_data.chip->irq_request(&desc->irq_data); + + if (ret) + goto out_mask; + } + /* Setup the type (level, edge polarity) if configured: */ if (new->flags & IRQF_TRIGGER_MASK) {