[alsa-devel] [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it
In http://www.spinics.net/lists/linux-tegra/msg01731.html, Mark Brown pointed out that it was a little silly forcing every board or driver to gpio_request() a GPIO that is later converted to an IRQ, and passed to request_irq. The first patch in this series instead makes the core IRQ code perform these calls when appropriate, to avoid duplicating it everywhere.
However, this change has the potential for significant regressions; at least some drivers are already calling gpio_request for GPIOs that are also used as IRQs. This then causes the gpio_request inside the core IRQ code to fail, which causes functional regressions. I'm not sure how wide- spread this issue is, but in testing on NVIDIA Tegra, I found two instances that needed to be fixed. Perhaps a failure of gpio_request in the core IRQ code should trigger a WARN rather than returning an error, to give a grace period for conversion of other code?
Stephen Warren (3): irq: If an IRQ is a GPIO, request and configure it mmc: tegra: Don't gpio_request GPIOs used as IRQs. ASoC: jack_add_gpios: Don't gpio_request GPIOs used as IRQs.
drivers/mmc/host/sdhci-tegra.c | 8 -------- kernel/irq/manage.c | 25 +++++++++++++++++++++++-- sound/soc/soc-jack.c | 13 +------------ 3 files changed, 24 insertions(+), 22 deletions(-)
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; + } }
/* @@ -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 */
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.
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
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 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.
- 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.
On Thu, 4 Aug 2011, 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.
This is very much the wrong approach. If you think it through then the irq setup code might end up with tons of other subsystem specific setup thingies, e.g. PCI .....
The right thing to do is to add a irq_configure() function pointer to the irq chip and provide a common function for gpios in gpiolib, which is then used by the particular GPIO irq chip implementation.
Thanks,
tglx --- diff --git a/include/linux/irq.h b/include/linux/irq.h index 5951730..33ba4b8 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -265,6 +265,7 @@ static inline void irqd_clr_chained_irq_inprogress(struct irq_data *d) * struct irq_chip - hardware interrupt chip descriptor * * @name: name for /proc/interrupts + * @irq_configure: configure an interrupt (optional) * @irq_startup: start up the interrupt (defaults to ->enable if NULL) * @irq_shutdown: shut down the interrupt (defaults to ->disable if NULL) * @irq_enable: enable the interrupt (defaults to chip->unmask if NULL) @@ -289,9 +290,14 @@ static inline void irqd_clr_chained_irq_inprogress(struct irq_data *d) * @flags: chip specific flags * * @release: release function solely used by UML + * + * If @irq_configure is provided, it's called from setup_irq prior to + * enabling the interrupt. irq_configure should return 0 on success or + * an appropriate error code. */ struct irq_chip { const char *name; + int (*irq_configure)(struct irq_data *data); unsigned int (*irq_startup)(struct irq_data *data); void (*irq_shutdown)(struct irq_data *data); void (*irq_enable)(struct irq_data *data); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 9b956fa..d5e6a58 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -999,6 +999,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (!shared) { init_waitqueue_head(&desc->wait_for_threads);
+ /* Configure the interrupt */ + if (desc->chip->irq_configure) { + ret = desc->chip->irq_configure(&desc->irq_data); + if (ret) + goto out_mask; + } + /* Setup the type (level, edge polarity) if configured: */ if (new->flags & IRQF_TRIGGER_MASK) { ret = __irq_set_trigger(desc, irq,
Thomas Gleixner wrote at Friday, September 02, 2011 2:37 AM:
On Thu, 4 Aug 2011, 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.
This is very much the wrong approach. If you think it through then the irq setup code might end up with tons of other subsystem specific setup thingies, e.g. PCI .....
The right thing to do is to add a irq_configure() function pointer to the irq chip and provide a common function for gpios in gpiolib, which is then used by the particular GPIO irq chip implementation.
Sorry, could you expand on this some more.
The whole point of these patches is that it's impossible to convert from IRQ number to GPIO number.
Some drivers use just an IRQ, and don't care if it's a GPIO. They work fine currently.
Some drivers use just a GPIO; that's not relevant to these patches.
Some drivers use something that's both an IRQ and a GPIO. Historically, this has worked by passing the IRQ to the driver, and then the driver calling irq_to_gpio() to get the GPIO ID. Since irq_to_gpio() is being removed, this is no longer possible. The whole point of the removal was that it's not possible in general to convert from IRQ to GPIO, so I'm not sure exactly what you're proposing irq_configure() or gpiolib do to centralize this?
Thanks for any insight.
Stephen Warren wrote at Friday, September 02, 2011 9:25 AM:
Thomas Gleixner wrote at Friday, September 02, 2011 2:37 AM:
On Thu, 4 Aug 2011, 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.
This is very much the wrong approach. If you think it through then the irq setup code might end up with tons of other subsystem specific setup thingies, e.g. PCI .....
The right thing to do is to add a irq_configure() function pointer to the irq chip and provide a common function for gpios in gpiolib, which is then used by the particular GPIO irq chip implementation.
Sorry, could you expand on this some more.
Uggh. Sorry; just ignore this response; I got it confused with the responses to my proposed I2C changes. I've long-since dropped the patch that you responded to.
On Fri, 2 Sep 2011, Stephen Warren wrote:
Thomas Gleixner wrote at Friday, September 02, 2011 2:37 AM:
On Thu, 4 Aug 2011, 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.
This is very much the wrong approach. If you think it through then the irq setup code might end up with tons of other subsystem specific setup thingies, e.g. PCI .....
The right thing to do is to add a irq_configure() function pointer to the irq chip and provide a common function for gpios in gpiolib, which is then used by the particular GPIO irq chip implementation.
Sorry, could you expand on this some more.
The whole point of these patches is that it's impossible to convert from IRQ number to GPIO number.
Some drivers use just an IRQ, and don't care if it's a GPIO. They work fine currently.
Some drivers use just a GPIO; that's not relevant to these patches.
Some drivers use something that's both an IRQ and a GPIO. Historically, this has worked by passing the IRQ to the driver, and then the driver calling irq_to_gpio() to get the GPIO ID. Since irq_to_gpio() is being removed, this is no longer possible. The whole point of the removal was that it's not possible in general to convert from IRQ to GPIO, so I'm not sure exactly what you're proposing irq_configure() or gpiolib do to centralize this?
chip->irq_configure() calls a irqchip function if set. So now the code which implements the irq functionality for GPIO definitely knows how to map the irq number to the GPIO pin, otherwise it would not be able to handle the mask/ack/unmask function either.
The drivers which just request an irq are oblivious about that:
request_irq(irq)
if (chip->irq_configure) chip->irq_configure() configure_gpio_pin()
Whether configure_gpio_pin() is a function which is implemented per GPIO driver or a common function in gpiolib is not relevant for the irq core code.
Thanks,
tglx
request_irq now performs this internally. Remove the exra calls from the driver so that request_irq succeeds.
Signed-off-by: Stephen Warren swarren@nvidia.com --- drivers/mmc/host/sdhci-tegra.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 93da940..d8d5ddd 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -166,14 +166,7 @@ static int __devinit sdhci_tegra_probe(struct platform_device *pdev) }
if (gpio_is_valid(plat->cd_gpio)) { - rc = gpio_request(plat->cd_gpio, "sdhci_cd"); - if (rc) { - dev_err(mmc_dev(host->mmc), - "failed to allocate cd gpio\n"); - goto err_cd_req; - } tegra_gpio_enable(plat->cd_gpio); - gpio_direction_input(plat->cd_gpio);
rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING, @@ -263,7 +256,6 @@ static int __devexit sdhci_tegra_remove(struct platform_device *pdev) if (gpio_is_valid(plat->cd_gpio)) { free_irq(gpio_to_irq(plat->cd_gpio), host); tegra_gpio_disable(plat->cd_gpio); - gpio_free(plat->cd_gpio); }
if (gpio_is_valid(plat->power_gpio)) {
request_any_context_irq now performs this internally. Remove the exra calls from the driver so that request_any_context_irq succeeds.
Signed-off-by: Stephen Warren swarren@nvidia.com --- sound/soc/soc-jack.c | 13 +------------ 1 files changed, 1 insertions(+), 12 deletions(-)
diff --git a/sound/soc/soc-jack.c b/sound/soc/soc-jack.c index 7c17b98..306d521 100644 --- a/sound/soc/soc-jack.c +++ b/sound/soc/soc-jack.c @@ -310,14 +310,6 @@ int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count, goto undo; }
- ret = gpio_request(gpios[i].gpio, gpios[i].name); - if (ret) - goto undo; - - ret = gpio_direction_input(gpios[i].gpio); - if (ret) - goto err; - INIT_DELAYED_WORK(&gpios[i].work, gpio_work); gpios[i].jack = jack;
@@ -328,7 +320,7 @@ int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count, gpios[i].name, &gpios[i]); if (ret) - goto err; + goto undo;
if (gpios[i].wake) { ret = irq_set_irq_wake(gpio_to_irq(gpios[i].gpio), 1); @@ -349,8 +341,6 @@ int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count,
return 0;
-err: - gpio_free(gpios[i].gpio); undo: snd_soc_jack_free_gpios(jack, i, gpios);
@@ -378,7 +368,6 @@ void snd_soc_jack_free_gpios(struct snd_soc_jack *jack, int count, #endif free_irq(gpio_to_irq(gpios[i].gpio), &gpios[i]); cancel_delayed_work_sync(&gpios[i].work); - gpio_free(gpios[i].gpio); gpios[i].jack = NULL; } }
On Thu, Aug 04, 2011 at 05:00:17PM -0600, Stephen Warren wrote:
In http://www.spinics.net/lists/linux-tegra/msg01731.html, Mark Brown pointed out that it was a little silly forcing every board or driver to gpio_request() a GPIO that is later converted to an IRQ, and passed to request_irq. The first patch in this series instead makes the core IRQ code perform these calls when appropriate, to avoid duplicating it everywhere.
Trying to go from IRQ to GPIO is not a good idea - most of the IRQ <-> GPIO macros we have today are just plain broken. Many of them just add or subtract a constant, which means non-GPIO IRQs have an apparant GPIO number too. Couple this with the fact that all positive GPIO numbers are valid, and this is a recipe for wrong GPIOs getting used and GPIOs being requested for non-GPIO IRQs.
I think this was also discussed in the past, and the conclusion was that IRQs should be kept separate from GPIOs. Maybe views have changed since then...
However, if we do want to do this, then it would be much better to provide a new API for requesting GPIO IRQs, eg:
gpio_request_irq()
which would wrap around request_threaded_irq(), takes a GPIO number, does the GPIO->IRQ conversion internally, and whatever GPIO setup is required. Something like this:
int gpio_request_threaded_irq(int gpio, irq_handler_t handler, irq_handler_t thread_fn, unsigned long flags, const char *name, void *dev) { int ret;
if (!gpio_valid(gpio)) return -EINVAL;
ret = gpio_request_one(gpio, GPIOF_IN, name); if (ret) return ret;
ret = request_threaded_irq(gpio_to_irq(gpio), handler, thread_fn, flags, name, dev); if (ret) gpio_free(gpio);
return ret; }
This then limits the exposure of the GPIO<->IRQ conversion macros to just GPIOs, where the buggy nature of the existing conversions won't impact on non-GPIO IRQs.
On Fri, Aug 05, 2011 at 10:40:17AM +0100, Russell King - ARM Linux wrote:
On Thu, Aug 04, 2011 at 05:00:17PM -0600, Stephen Warren wrote:
In http://www.spinics.net/lists/linux-tegra/msg01731.html, Mark Brown pointed out that it was a little silly forcing every board or driver to gpio_request() a GPIO that is later converted to an IRQ, and passed to request_irq. The first patch in this series instead makes the core IRQ code perform these calls when appropriate, to avoid duplicating it everywhere.
Trying to go from IRQ to GPIO is not a good idea - most of the IRQ <-> GPIO macros we have today are just plain broken. Many of them just add or subtract a constant, which means non-GPIO IRQs have an apparant GPIO number too. Couple this with the fact that all positive GPIO numbers are valid, and this is a recipe for wrong GPIOs getting used and GPIOs being requested for non-GPIO IRQs.
Yes, and there's a pile without these defined/
I think this was also discussed in the past, and the conclusion was that IRQs should be kept separate from GPIOs. Maybe views have changed since then...
However, if we do want to do this, then it would be much better to provide a new API for requesting GPIO IRQs, eg:
gpio_request_irq()
which would wrap around request_threaded_irq(), takes a GPIO number, does the GPIO->IRQ conversion internally, and whatever GPIO setup is required. Something like this:
int gpio_request_threaded_irq(int gpio, irq_handler_t handler, irq_handler_t thread_fn, unsigned long flags, const char *name, void *dev) { int ret;
if (!gpio_valid(gpio)) return -EINVAL;
ret = gpio_request_one(gpio, GPIOF_IN, name); if (ret) return ret;
ret = request_threaded_irq(gpio_to_irq(gpio), handler, thread_fn, flags, name, dev); if (ret) gpio_free(gpio);
return ret; }
This then limits the exposure of the GPIO<->IRQ conversion macros to just GPIOs, where the buggy nature of the existing conversions won't impact on non-GPIO IRQs.
What about the case where we need to turn GPIO numbers into interrupts to pass to other drivers? In the case where we have a gpio chip that is providing interrupt services to other drivers (such as serial chip).
Having looked at a couple of IIO drivers, it seems that the need to use irq_to_gpio() seems to be to check if the device needs to be service. It would be useful to see if this is due to a problem with the threadder IRQ handler (and if so, may need fixing for the general case).
Russell King - ARM Linux wrote at Friday, August 05, 2011 3:40 AM:
On Thu, Aug 04, 2011 at 05:00:17PM -0600, Stephen Warren wrote:
In http://www.spinics.net/lists/linux-tegra/msg01731.html, Mark Brown pointed out that it was a little silly forcing every board or driver to gpio_request() a GPIO that is later converted to an IRQ, and passed to request_irq. The first patch in this series instead makes the core IRQ code perform these calls when appropriate, to avoid duplicating it everywhere.
Trying to go from IRQ to GPIO is not a good idea - most of the IRQ <-> GPIO macros we have today are just plain broken. Many of them just add or subtract a constant, which means non-GPIO IRQs have an apparant GPIO number too. Couple this with the fact that all positive GPIO numbers are valid, and this is a recipe for wrong GPIOs getting used and GPIOs being requested for non-GPIO IRQs.
I think this was also discussed in the past, and the conclusion was that IRQs should be kept separate from GPIOs. Maybe views have changed since then...
However, if we do want to do this, then it would be much better to provide a new API for requesting GPIO IRQs, eg:
gpio_request_irq()
which would wrap around request_threaded_irq(), takes a GPIO number, does the GPIO->IRQ conversion internally, and whatever GPIO setup is required. Something like this:
With that approach, drivers need to explicitly know whether they're passed a GPIO or an IRQ, and do something different, or they need to choose to only accept a GPIO or IRQ.
Take the case of the WM8903 audio codec, which is the case I was primarily motivated by.
The struct i2c_board_info for the WM8903 contains a .irq field, which is already an IRQ number. This IRQ might be a plain IRQ (i.e. a dedicated interrupt pin on the SoC's package), or a pin that's a GPIO with interrupt capabilities.
It'd be best if the WM8903 driver could handle either case, and be Completely unaware of which case was used in practice; it simply wants to request the interrupt, and have that work appropriately.
Having a separate API for this means pushing this GPIO-vs-IRQ knowledge into all drivers, which doesn't seem like a good thing.
So it seems like, as was mentioned elsewhere in this thread, the upshot of this conversation is that interrupt chip drivers should do this internally, both to avoid requiring a general irq_to_gpio function, and because calling gpio_direction_input for GPIOs-used-as-IRQs isn't appropriate for all hardware.
int gpio_request_threaded_irq(int gpio, irq_handler_t handler, irq_handler_t thread_fn, unsigned long flags, const char *name, void *dev)
...
On Fri, Aug 05, 2011 at 08:43:20AM -0700, Stephen Warren wrote:
Russell King - ARM Linux wrote at Friday, August 05, 2011 3:40 AM:
On Thu, Aug 04, 2011 at 05:00:17PM -0600, Stephen Warren wrote:
In http://www.spinics.net/lists/linux-tegra/msg01731.html, Mark Brown pointed out that it was a little silly forcing every board or driver to gpio_request() a GPIO that is later converted to an IRQ, and passed to request_irq. The first patch in this series instead makes the core IRQ code perform these calls when appropriate, to avoid duplicating it everywhere.
Trying to go from IRQ to GPIO is not a good idea - most of the IRQ <-> GPIO macros we have today are just plain broken. Many of them just add or subtract a constant, which means non-GPIO IRQs have an apparant GPIO number too. Couple this with the fact that all positive GPIO numbers are valid, and this is a recipe for wrong GPIOs getting used and GPIOs being requested for non-GPIO IRQs.
I think this was also discussed in the past, and the conclusion was that IRQs should be kept separate from GPIOs. Maybe views have changed since then...
However, if we do want to do this, then it would be much better to provide a new API for requesting GPIO IRQs, eg:
gpio_request_irq()
which would wrap around request_threaded_irq(), takes a GPIO number, does the GPIO->IRQ conversion internally, and whatever GPIO setup is required. Something like this:
With that approach, drivers need to explicitly know whether they're passed a GPIO or an IRQ, and do something different, or they need to choose to only accept a GPIO or IRQ.
You completely missed the biggest reason why your approach is broken.
+ gpio = irq_to_gpio(irq); + if (gpio_is_valid(gpio))
Let's look at the code:
#define ARCH_NR_GPIOS 256
static inline bool gpio_is_valid(int number) { return number >= 0 && number < ARCH_NR_GPIOS; }
Now, let's take AT91:
#define irq_to_gpio(irq) (irq)
This doesn't define ARCH_NR_GPIOS, so it gets the default 256. Now lets take a random selection of the AT91 interrupt numbers:
#define AT91RM9200_ID_US3 9 /* USART 3 */ #define AT91RM9200_ID_MCI 10 /* Multimedia Card Interface */ #define AT91RM9200_ID_UDP 11 /* USB Device Port */ #define AT91RM9200_ID_TWI 12 /* Two-Wire Interface */ #define AT91RM9200_ID_SPI 13 /* Serial Peripheral Interface */ #define AT91RM9200_ID_SSC0 14 /* Serial Synchronous Controller 0 */ #define AT91RM9200_ID_SSC1 15 /* Serial Synchronous Controller 1 */
None of these are GPIOs. Yet gpio_is_valid(irq_to_gpio(AT91RM9200_ID_TWI)) is true.
That's the problem - it's undefined whether gpio_is_valid(irq_to_gpio(irq)) returns true or false for any particular interrupt. There's no multiplexing through gpiolib for the IRQ-to-GPIO mapping either, so it doesn't work for off-SoC GPIOs.
So, you can't reliably go from interrupt numbers to GPIO numbers - it's just not supported. So to throw this into the IRQ layer is just going to end up breaking a hell of a lot of platforms.
Now, stack on top of that a discussion at the Linaro Connect conference this week where we discussed getting rid of IRQ numbers entirely, and our desire to kill off irq_to_gpio() and I think it makes this approach a non-starter.
So it seems like, as was mentioned elsewhere in this thread, the upshot of this conversation is that interrupt chip drivers should do this internally, both to avoid requiring a general irq_to_gpio function, and because calling gpio_direction_input for GPIOs-used-as-IRQs isn't appropriate for all hardware.
That would be more appropriate, because the IRQ chip stuff at least knows if there's a GPIO associated with it.
There's still the unanswered question whether we even want the IRQ layer to do this kind of stuff, and the previous decision on that I believe was in the negative. So I think Thomas needs to respond to that point first.
Russell King - ARM Linux wrote at Friday, August 05, 2011 1:15 PM:
On Fri, Aug 05, 2011 at 08:43:20AM -0700, Stephen Warren wrote:
Russell King - ARM Linux wrote at Friday, August 05, 2011 3:40 AM:
On Thu, Aug 04, 2011 at 05:00:17PM -0600, Stephen Warren wrote:
In http://www.spinics.net/lists/linux-tegra/msg01731.html, Mark Brown pointed out that it was a little silly forcing every board or driver to gpio_request() a GPIO that is later converted to an IRQ, and passed to request_irq. The first patch in this series instead makes the core IRQ code perform these calls when appropriate, to avoid duplicating it everywhere.
Trying to go from IRQ to GPIO is not a good idea - most of the IRQ <-> GPIO macros we have today are just plain broken. Many of them just add or subtract a constant, which means non-GPIO IRQs have an apparant GPIO number too. Couple this with the fact that all positive GPIO numbers are valid, and this is a recipe for wrong GPIOs getting used and GPIOs being requested for non-GPIO IRQs.
I think this was also discussed in the past, and the conclusion was that IRQs should be kept separate from GPIOs. Maybe views have changed since then...
However, if we do want to do this, then it would be much better to provide a new API for requesting GPIO IRQs, eg:
gpio_request_irq()
which would wrap around request_threaded_irq(), takes a GPIO number, does the GPIO->IRQ conversion internally, and whatever GPIO setup is required. Something like this:
With that approach, drivers need to explicitly know whether they're passed a GPIO or an IRQ, and do something different, or they need to choose to only accept a GPIO or IRQ.
You completely missed the biggest reason why your approach is broken.
No, I didn't.
I was discussing whether an alternative API for IRQ registration would work, and I was pointing out some problems with it.
That has nothing to do with whether my original proposal is workable.
On Fri, Aug 05, 2011 at 12:33:31PM -0700, Stephen Warren wrote:
Russell King - ARM Linux wrote at Friday, August 05, 2011 1:15 PM:
On Fri, Aug 05, 2011 at 08:43:20AM -0700, Stephen Warren wrote:
Russell King - ARM Linux wrote at Friday, August 05, 2011 3:40 AM:
On Thu, Aug 04, 2011 at 05:00:17PM -0600, Stephen Warren wrote:
In http://www.spinics.net/lists/linux-tegra/msg01731.html, Mark Brown pointed out that it was a little silly forcing every board or driver to gpio_request() a GPIO that is later converted to an IRQ, and passed to request_irq. The first patch in this series instead makes the core IRQ code perform these calls when appropriate, to avoid duplicating it everywhere.
Trying to go from IRQ to GPIO is not a good idea - most of the IRQ <-> GPIO macros we have today are just plain broken. Many of them just add or subtract a constant, which means non-GPIO IRQs have an apparant GPIO number too. Couple this with the fact that all positive GPIO numbers are valid, and this is a recipe for wrong GPIOs getting used and GPIOs being requested for non-GPIO IRQs.
I think this was also discussed in the past, and the conclusion was that IRQs should be kept separate from GPIOs. Maybe views have changed since then...
However, if we do want to do this, then it would be much better to provide a new API for requesting GPIO IRQs, eg:
gpio_request_irq()
which would wrap around request_threaded_irq(), takes a GPIO number, does the GPIO->IRQ conversion internally, and whatever GPIO setup is required. Something like this:
With that approach, drivers need to explicitly know whether they're passed a GPIO or an IRQ, and do something different, or they need to choose to only accept a GPIO or IRQ.
You completely missed the biggest reason why your approach is broken.
No, I didn't.
Yes you did.
I was discussing whether an alternative API for IRQ registration would work, and I was pointing out some problems with it.
That has nothing to do with whether my original proposal is workable.
And that proves that you missed the point. I am suggesting an alternative solution precisely because your original proposal is unworkable.
participants (5)
-
Ben Dooks -
Mark Brown -
Russell King - ARM Linux -
Stephen Warren -
Thomas Gleixner