| Message ID | 1312498820-2275-2-git-send-email-swarren@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 04, 2011 at 05:00:18PM -0600, Stephen Warren wrote: > + } else { > + gpio = irq_to_gpio(irq); > + if (gpio_is_valid(gpio)) { > + ret = gpio_request(gpio, new->name); > + if (ret < 0) > + goto out_mask; > + ret = gpio_direction_input(gpio); > + if (ret < 0) > + goto out_mask; > + } If you treat failures as an error what happens when a driver is using a GPIO as both an interrupt and a GPIO? For example a driver which monitors the level on a GPIO and uses edge triggered IRQs to be notified of state changes.
On 08/04/2011 06:00 PM, Stephen Warren wrote: > Many IRQs are associated with GPIO pins. When the pin is used as an IRQ, > it can't be used as anything else; it should be requested. Enhance the > core interrupt code to call gpio_request() and gpio_direction_input() for > any IRQ that is also a GPIO. This prevents duplication of these calls in > each driver that uses an IRQ. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > kernel/irq/manage.c | 23 +++++++++++++++++++++-- > 1 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 0a7840a..6e2db72 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -7,6 +7,7 @@ > * This file contains driver APIs to the irq subsystem. > */ > > +#include <linux/gpio.h> > #include <linux/irq.h> > #include <linux/kthread.h> > #include <linux/module.h> > @@ -875,7 +876,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > struct irqaction *old, **old_ptr; > const char *old_name = NULL; > unsigned long flags, thread_mask = 0; > - int ret, nested, shared = 0; > + int ret, nested, shared = 0, gpio = -1; > cpumask_var_t mask; > > if (!desc) > @@ -978,6 +979,16 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > old = *old_ptr; > } while (old); > shared = 1; > + } else { > + gpio = irq_to_gpio(irq); If you read the documentation for gpio, it is not recommended to use irq_to_gpio. There's only a handful of users. Part of the problem is it is platform specific and the gpio core cannot convert irq to gpio number. Here is the relevant section: Non-error values returned from irq_to_gpio() would most commonly be used with gpio_get_value(), for example to initialize or update driver state when the IRQ is edge-triggered. Note that some platforms don't support this reverse mapping, so you should avoid using it. Rob > + if (gpio_is_valid(gpio)) { > + ret = gpio_request(gpio, new->name); > + if (ret < 0) > + goto out_mask; > + ret = gpio_direction_input(gpio); > + if (ret < 0) > + goto out_mask; > + } > } > > /* > @@ -1083,6 +1094,8 @@ mismatch: > ret = -EBUSY; > > out_mask: > + if (gpio_is_valid(gpio)) > + gpio_free(gpio); > raw_spin_unlock_irqrestore(&desc->lock, flags); > free_cpumask_var(mask); > > @@ -1127,6 +1140,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) > struct irq_desc *desc = irq_to_desc(irq); > struct irqaction *action, **action_ptr; > unsigned long flags; > + int gpio; > > WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq); > > @@ -1165,8 +1179,13 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) > #endif > > /* If this was the last handler, shut down the IRQ line: */ > - if (!desc->action) > + if (!desc->action) { > irq_shutdown(desc); > + /* If the IRQ line is a GPIO, it's no longer in use */ > + gpio = irq_to_gpio(irq); > + if (gpio_is_valid(gpio)) > + gpio_free(gpio); > + } > > #ifdef CONFIG_SMP > /* make sure affinity_hint is cleaned up */
Mark Brown wrote at Thursday, August 04, 2011 6:02 PM: > On Thu, Aug 04, 2011 at 05:00:18PM -0600, Stephen Warren wrote: > > > + } else { > > + gpio = irq_to_gpio(irq); > > + if (gpio_is_valid(gpio)) { > > + ret = gpio_request(gpio, new->name); > > + if (ret < 0) > > + goto out_mask; > > + ret = gpio_direction_input(gpio); > > + if (ret < 0) > > + goto out_mask; > > + } > > If you treat failures as an error what happens when a driver is using a > GPIO as both an interrupt and a GPIO? For example a driver which > monitors the level on a GPIO and uses edge triggered IRQs to be notified > of state changes. Well, things break. This is essentially the problem I was describing in the PATCH 0 email, just with a slightly different motivation. I suppose that an alternative here would be to simply ignore any errors from gpio_request. This might have the benefit of removing the need for the other two patches I posted in the series. However, it seems a little dirty; one benefit of the IRQ code calling gpio_request and honoring errors would be to detect when some completely unrelated code had a bug and had called gpio_request on the GPIO before. Such detection would be non-existent if we don't error out on gpio_request. Perhaps some mechanism is needed to indicate that the driver has explicitly already called gpio_request for a legitimate shared purpose, and only then ignore errors? -- nvpublic
Rob Herring wrote at Thursday, August 04, 2011 7:55 PM: > On 08/04/2011 06:00 PM, Stephen Warren wrote: > > Many IRQs are associated with GPIO pins. When the pin is used as an IRQ, > > it can't be used as anything else; it should be requested. Enhance the > > core interrupt code to call gpio_request() and gpio_direction_input() for > > any IRQ that is also a GPIO. This prevents duplication of these calls in > > each driver that uses an IRQ. ... > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c ... > > @@ -978,6 +979,16 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > > old = *old_ptr; > > } while (old); > > shared = 1; > > + } else { > > + gpio = irq_to_gpio(irq); > > If you read the documentation for gpio, it is not recommended to use > irq_to_gpio. There's only a handful of users. Part of the problem is it > is platform specific and the gpio core cannot convert irq to gpio > number It seems like that's a soluble problem though? I was thinking about adding a to_gpio function to struct irq_chip, as the inverse of struct gpio_chip's to_irq. Then, presumably any platform would be able to convert back from IRQ to GPIO, provided the platform called a new __irq_to_gpio from the platform-specific gpio_to_irq, just like most gpio_to_irq implementations defer to __gpio_to_irq. I ended up not doing that in this patchset, since Tegra's gpio_to_irq function already works for the IRQs/GPIOs that were relevant for my testing, and I wanted to post a simple patch first to driver discussion. > Here is the relevant section: > > Non-error values returned from irq_to_gpio() would most commonly be used > with gpio_get_value(), for example to initialize or update driver state > when the IRQ is edge-triggered. Note that some platforms don't support > this reverse mapping, so you should avoid using it.
On Thu, Aug 04, 2011 at 08:53:34PM -0700, Stephen Warren wrote: > Well, things break. This is essentially the problem I was describing in > the PATCH 0 email, just with a slightly different motivation. There's a bunch of existing code using that idiom. > I suppose that an alternative here would be to simply ignore any errors > from gpio_request. This might have the benefit of removing the need for > the other two patches I posted in the series. However, it seems a little > dirty; one benefit of the IRQ code calling gpio_request and honoring > errors would be to detect when some completely unrelated code had a bug > and had called gpio_request on the GPIO before. Such detection would be > non-existent if we don't error out on gpio_request. Perhaps some mechanism > is needed to indicate that the driver has explicitly already called > gpio_request for a legitimate shared purpose, and only then ignore > errors? But it's not a bug to use a GPIO as an IRQ source, otherwise we wouldn't have gpio_to_irq() in the first place. Feels like we need a backchannel between gpiolib and the IRQ code to do this. Or perhaps the drivers that implement this should be taking care of setting up the GPIO mode?
On Thu, Aug 04, 2011 at 05:00:18PM -0600, Stephen Warren wrote: > Many IRQs are associated with GPIO pins. When the pin is used as an IRQ, > it can't be used as anything else; it should be requested. Enhance the > core interrupt code to call gpio_request() and gpio_direction_input() for > any IRQ that is also a GPIO. This prevents duplication of these calls in > each driver that uses an IRQ. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > kernel/irq/manage.c | 23 +++++++++++++++++++++-- > 1 files changed, 21 insertions(+), 2 deletions(-) > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 0a7840a..6e2db72 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -7,6 +7,7 @@ > * This file contains driver APIs to the irq subsystem. > */ > > +#include <linux/gpio.h> > #include <linux/irq.h> > #include <linux/kthread.h> > #include <linux/module.h> > @@ -875,7 +876,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > struct irqaction *old, **old_ptr; > const char *old_name = NULL; > unsigned long flags, thread_mask = 0; > - int ret, nested, shared = 0; > + int ret, nested, shared = 0, gpio = -1; > cpumask_var_t mask; > > if (!desc) > @@ -978,6 +979,16 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > old = *old_ptr; > } while (old); > shared = 1; > + } else { > + gpio = irq_to_gpio(irq); > + if (gpio_is_valid(gpio)) { > + ret = gpio_request(gpio, new->name); > + if (ret < 0) > + goto out_mask; > + ret = gpio_direction_input(gpio); > + if (ret < 0) > + goto out_mask; > + } First,y gpio_direction_input() is an action specific to the implementation and would break on the s3c24xx series as the irq mode is also part of the gpio i/o/sfn configuration Secondly, you are going to have to go through all the soc code and fixup the missing or bad implmenetations of irq_to_gpio() as a number of SoCs are not implemented or worse are so badly implemented you will get valid results for non-gpio interrupts. > } > > /* > @@ -1083,6 +1094,8 @@ mismatch: > ret = -EBUSY; > > out_mask: > + if (gpio_is_valid(gpio)) > + gpio_free(gpio); > raw_spin_unlock_irqrestore(&desc->lock, flags); > free_cpumask_var(mask); > > @@ -1127,6 +1140,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) > struct irq_desc *desc = irq_to_desc(irq); > struct irqaction *action, **action_ptr; > unsigned long flags; > + int gpio; > > WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq); > > @@ -1165,8 +1179,13 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) > #endif > > /* If this was the last handler, shut down the IRQ line: */ > - if (!desc->action) > + if (!desc->action) { > irq_shutdown(desc); > + /* If the IRQ line is a GPIO, it's no longer in use */ > + gpio = irq_to_gpio(irq); > + if (gpio_is_valid(gpio)) > + gpio_free(gpio); > + } > > #ifdef CONFIG_SMP > /* make sure affinity_hint is cleaned up */ > -- > 1.7.0.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Aug 05, 2011 at 06:35:11AM +0100, Mark Brown wrote: > On Thu, Aug 04, 2011 at 08:53:34PM -0700, Stephen Warren wrote: > > > Well, things break. This is essentially the problem I was describing in > > the PATCH 0 email, just with a slightly different motivation. > > There's a bunch of existing code using that idiom. > > > I suppose that an alternative here would be to simply ignore any errors > > from gpio_request. This might have the benefit of removing the need for > > the other two patches I posted in the series. However, it seems a little > > dirty; one benefit of the IRQ code calling gpio_request and honoring > > errors would be to detect when some completely unrelated code had a bug > > and had called gpio_request on the GPIO before. Such detection would be > > non-existent if we don't error out on gpio_request. Perhaps some mechanism > > is needed to indicate that the driver has explicitly already called > > gpio_request for a legitimate shared purpose, and only then ignore > > errors? > > But it's not a bug to use a GPIO as an IRQ source, otherwise we wouldn't > have gpio_to_irq() in the first place. Feels like we need a backchannel > between gpiolib and the IRQ code to do this. Or perhaps the drivers > that implement this should be taking care of setting up the GPIO mode? Converting GPIOs to IRQs is really useful. The reverse mapping is not as easy, as when go NR->CHIP->to_irq() method, but the reverse is not being tracked at the moment, which makes life difficult. I would say that setting the interrupt should deal with all the necessary configuration to get that interrupt working. In the case of pretty much all of the SoCs I have been involved with have always set the GPIO's function to allow the interrupt to pass through. Whilst there's a case for having this being done either in the core IRQ code (may break for everyone else) we could quite easily do this in the GPIO driver. As a note, we are actuallly trying to remove the irq_to_gpio() calls, as they are not widely used across the kernel, very sparsely and badly implemented across ARM and are very rarely used (the IIO system is the only system currently doing this, for some fairly nasty reasons)
On Fri, Aug 05, 2011 at 09:06:20AM +0100, Ben Dooks wrote: > I would say that setting the interrupt should deal with all the necessary > configuration to get that interrupt working. In the case of pretty much all > of the SoCs I have been involved with have always set the GPIO's function > to allow the interrupt to pass through. That's what Stephen is trying to do, essentially. It's looking like it's more sensible to fix it in the Tegra interrupt drivers, though.
Mark Brown wrote at Thursday, August 04, 2011 11:35 PM: > On Thu, Aug 04, 2011 at 08:53:34PM -0700, Stephen Warren wrote: > > > Well, things break. This is essentially the problem I was describing in > > the PATCH 0 email, just with a slightly different motivation. > > There's a bunch of existing code using that idiom. Certainly. > > I suppose that an alternative here would be to simply ignore any errors > > from gpio_request. This might have the benefit of removing the need for > > the other two patches I posted in the series. However, it seems a little > > dirty; one benefit of the IRQ code calling gpio_request and honoring > > errors would be to detect when some completely unrelated code had a bug > > and had called gpio_request on the GPIO before. Such detection would be > > non-existent if we don't error out on gpio_request. Perhaps some mechanism > > is needed to indicate that the driver has explicitly already called > > gpio_request for a legitimate shared purpose, and only then ignore > > errors? > > But it's not a bug to use a GPIO as an IRQ source, otherwise we wouldn't > have gpio_to_irq() in the first place. True, but I think there are two cases: 1) Some code legitimately uses a pin as both a GPIO and IRQ, and is fully cognizant of the fact, just like in your example -> no bug. 2) Two pieces of unrelated code somehow accidentally get a GPIO and IRQ number that map to the same resource, e.g. due to incorrect board files or Device Tree content. This is probably a bug, but ends up looking exactly the same as far as the IRQ code's gpio_request call failing in the patch I posted. > Feels like we need a backchannel > between gpiolib and the IRQ code to do this. Or perhaps the drivers > that implement this should be taking care of setting up the GPIO mode?
On Fri, Aug 05, 2011 at 08:29:38AM -0700, Stephen Warren wrote: > Mark Brown wrote at Thursday, August 04, 2011 11:35 PM: > > But it's not a bug to use a GPIO as an IRQ source, otherwise we wouldn't > > have gpio_to_irq() in the first place. > 2) Two pieces of unrelated code somehow accidentally get a GPIO and IRQ > number that map to the same resource, e.g. due to incorrect board files or > Device Tree content. This is probably a bug, but ends up looking exactly > the same as far as the IRQ code's gpio_request call failing in the patch I > posted. Right, but this doesn't mean we can break the legitimate users to catch the buggy ones.
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 0a7840a..6e2db72 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -7,6 +7,7 @@ * This file contains driver APIs to the irq subsystem. */ +#include <linux/gpio.h> #include <linux/irq.h> #include <linux/kthread.h> #include <linux/module.h> @@ -875,7 +876,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) struct irqaction *old, **old_ptr; const char *old_name = NULL; unsigned long flags, thread_mask = 0; - int ret, nested, shared = 0; + int ret, nested, shared = 0, gpio = -1; cpumask_var_t mask; if (!desc) @@ -978,6 +979,16 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) old = *old_ptr; } while (old); shared = 1; + } else { + gpio = irq_to_gpio(irq); + if (gpio_is_valid(gpio)) { + ret = gpio_request(gpio, new->name); + if (ret < 0) + goto out_mask; + ret = gpio_direction_input(gpio); + if (ret < 0) + goto out_mask; + } } /* @@ -1083,6 +1094,8 @@ mismatch: ret = -EBUSY; out_mask: + if (gpio_is_valid(gpio)) + gpio_free(gpio); raw_spin_unlock_irqrestore(&desc->lock, flags); free_cpumask_var(mask); @@ -1127,6 +1140,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) struct irq_desc *desc = irq_to_desc(irq); struct irqaction *action, **action_ptr; unsigned long flags; + int gpio; WARN(in_interrupt(), "Trying to free IRQ %d from IRQ context!\n", irq); @@ -1165,8 +1179,13 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) #endif /* If this was the last handler, shut down the IRQ line: */ - if (!desc->action) + if (!desc->action) { irq_shutdown(desc); + /* If the IRQ line is a GPIO, it's no longer in use */ + gpio = irq_to_gpio(irq); + if (gpio_is_valid(gpio)) + gpio_free(gpio); + } #ifdef CONFIG_SMP /* make sure affinity_hint is cleaned up */
Many IRQs are associated with GPIO pins. When the pin is used as an IRQ, it can't be used as anything else; it should be requested. Enhance the core interrupt code to call gpio_request() and gpio_direction_input() for any IRQ that is also a GPIO. This prevents duplication of these calls in each driver that uses an IRQ. Signed-off-by: Stephen Warren <swarren@nvidia.com> --- kernel/irq/manage.c | 23 +++++++++++++++++++++-- 1 files changed, 21 insertions(+), 2 deletions(-)