* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren 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(-) ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: linux-arm-kernel 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(-) ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof Cc: linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra, Stephen Warren 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(-) ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <1312498820-2275-1-git-send-email-swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-04 23:00 ` Stephen Warren (?) @ 2011-08-04 23:00 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> --- 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 */ -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: linux-arm-kernel 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 */ -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof Cc: linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra, Stephen Warren 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 */ -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-04 23:00 ` Stephen Warren (?) @ 2011-08-05 0:01 ` Mark Brown -1 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 0:01 UTC (permalink / raw) To: Stephen Warren Cc: alsa-devel, linux-mmc, linux-kernel, linux-tegra, ccross, olof, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 0:01 ` Mark Brown 0 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 0:01 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 0:01 ` Mark Brown 0 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 0:01 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <20110805000148.GB13321-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-05 0:01 ` Mark Brown (?) @ 2011-08-05 3:53 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 3:53 UTC (permalink / raw) To: Mark Brown Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 3:53 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 3:53 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 3:53 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 3:53 UTC (permalink / raw) To: Mark Brown Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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 ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-05 3:53 ` Stephen Warren (?) @ 2011-08-05 5:35 ` Mark Brown -1 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 5:35 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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? ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 5:35 ` Mark Brown 0 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 5:35 UTC (permalink / raw) To: linux-arm-kernel 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? ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 5:35 ` Mark Brown 0 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 5:35 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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? ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <20110805053510.GA16956-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-05 5:35 ` Mark Brown (?) @ 2011-08-05 8:06 ` Ben Dooks -1 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 8:06 UTC (permalink / raw) To: Mark Brown Cc: Stephen Warren, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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) -- Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 8:06 ` Ben Dooks 0 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 8:06 UTC (permalink / raw) To: 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) -- Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 8:06 ` Ben Dooks 0 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 8:06 UTC (permalink / raw) To: Mark Brown Cc: Stephen Warren, alsa-devel, linux-mmc, linux-kernel, linux-tegra, ccross, olof, Thomas Gleixner, Chris Ball, Liam Girdwood, 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) -- Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-05 8:06 ` Ben Dooks (?) @ 2011-08-05 8:29 ` Mark Brown -1 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 8:29 UTC (permalink / raw) To: Ben Dooks Cc: alsa-devel, Stephen Warren, linux-mmc, linux-kernel, olof, ccross, linux-tegra, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 8:29 ` Mark Brown 0 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 8:29 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 8:29 ` Mark Brown 0 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 8:29 UTC (permalink / raw) To: Ben Dooks Cc: Stephen Warren, alsa-devel, linux-mmc, linux-kernel, linux-tegra, ccross, olof, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-05 5:35 ` Mark Brown (?) @ 2011-08-05 15:29 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 15:29 UTC (permalink / raw) To: Mark Brown Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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? -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 15:29 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 15:29 UTC (permalink / raw) To: linux-arm-kernel 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? -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 15:29 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 15:29 UTC (permalink / raw) To: Mark Brown Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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? -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-05 15:29 ` Stephen Warren (?) @ 2011-08-05 16:15 ` Mark Brown -1 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 16:15 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 16:15 ` Mark Brown 0 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 16:15 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 16:15 ` Mark Brown 0 siblings, 0 replies; 73+ messages in thread From: Mark Brown @ 2011-08-05 16:15 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-04 23:00 ` Stephen Warren @ 2011-08-05 1:54 ` Rob Herring -1 siblings, 0 replies; 73+ messages in thread From: Rob Herring @ 2011-08-05 1:54 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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 */ ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 1:54 ` Rob Herring 0 siblings, 0 replies; 73+ messages in thread From: Rob Herring @ 2011-08-05 1:54 UTC (permalink / raw) To: linux-arm-kernel 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 */ ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-05 1:54 ` Rob Herring (?) @ 2011-08-05 4:05 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 4:05 UTC (permalink / raw) To: Rob Herring Cc: alsa-devel, Mark Brown, linux-mmc, linux-kernel, linux-tegra, ccross, olof, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 4:05 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 4:05 UTC (permalink / raw) To: linux-arm-kernel 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 4:05 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 4:05 UTC (permalink / raw) To: Rob Herring Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-04 23:00 ` Stephen Warren @ 2011-08-05 7:58 ` Ben Dooks -1 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 7:58 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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 -- Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-08-05 7:58 ` Ben Dooks 0 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 7:58 UTC (permalink / raw) To: linux-arm-kernel 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 at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-08-04 23:00 ` Stephen Warren @ 2011-09-02 8:36 ` Thomas Gleixner -1 siblings, 0 replies; 73+ messages in thread From: Thomas Gleixner @ 2011-09-02 8:36 UTC (permalink / raw) To: Stephen Warren Cc: Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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, ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-09-02 8:36 ` Thomas Gleixner 0 siblings, 0 replies; 73+ messages in thread From: Thomas Gleixner @ 2011-09-02 8:36 UTC (permalink / raw) To: linux-arm-kernel 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, ^ permalink raw reply related [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-09-02 8:36 ` Thomas Gleixner (?) @ 2011-09-02 15:24 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-09-02 15:24 UTC (permalink / raw) To: Thomas Gleixner Cc: Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-09-02 15:24 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-09-02 15:24 UTC (permalink / raw) To: linux-arm-kernel 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-09-02 15:24 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-09-02 15:24 UTC (permalink / raw) To: Thomas Gleixner Cc: Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <74CDBE0F657A3D45AFBB94109FB122FF04B327A55C-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org>]
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-09-02 15:24 ` Stephen Warren (?) @ 2011-09-02 15:34 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-09-02 15:34 UTC (permalink / raw) To: Thomas Gleixner Cc: Mark Brown, Liam Girdwood, Chris Ball, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-09-02 15:34 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-09-02 15:34 UTC (permalink / raw) To: linux-arm-kernel 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-09-02 15:34 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-09-02 15:34 UTC (permalink / raw) To: Thomas Gleixner Cc: Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it 2011-09-02 15:24 ` Stephen Warren (?) @ 2011-09-02 15:50 ` Thomas Gleixner -1 siblings, 0 replies; 73+ messages in thread From: Thomas Gleixner @ 2011-09-02 15:50 UTC (permalink / raw) To: Stephen Warren Cc: Mark Brown, Liam Girdwood, Chris Ball, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-09-02 15:50 ` Thomas Gleixner 0 siblings, 0 replies; 73+ messages in thread From: Thomas Gleixner @ 2011-09-02 15:50 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it @ 2011-09-02 15:50 ` Thomas Gleixner 0 siblings, 0 replies; 73+ messages in thread From: Thomas Gleixner @ 2011-09-02 15:50 UTC (permalink / raw) To: Stephen Warren Cc: Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra 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 ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH 2/3] mmc: tegra: Don't gpio_request GPIOs used as IRQs. 2011-08-04 23:00 ` Stephen Warren (?) @ 2011-08-04 23:00 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren request_irq now performs this internally. Remove the exra calls from the driver so that request_irq succeeds. Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> --- 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)) { -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 2/3] mmc: tegra: Don't gpio_request GPIOs used as IRQs. @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: linux-arm-kernel 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)) { -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 2/3] mmc: tegra: Don't gpio_request GPIOs used as IRQs. @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof Cc: linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra, Stephen Warren 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)) { -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 3/3] ASoC: jack_add_gpios: Don't gpio_request GPIOs used as IRQs. 2011-08-04 23:00 ` Stephen Warren (?) @ 2011-08-04 23:00 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, Stephen Warren 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> --- 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; } } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 3/3] ASoC: jack_add_gpios: Don't gpio_request GPIOs used as IRQs. @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: linux-arm-kernel 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; } } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 3/3] ASoC: jack_add_gpios: Don't gpio_request GPIOs used as IRQs. @ 2011-08-04 23:00 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-04 23:00 UTC (permalink / raw) To: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof Cc: linux-kernel, linux-arm-kernel, alsa-devel, linux-mmc, linux-tegra, Stephen Warren 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; } } -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it 2011-08-04 23:00 ` Stephen Warren (?) @ 2011-08-05 7:55 ` Ben Dooks -1 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 7:55 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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. Interesting, as this is not the case across all the ARM systems, as I believe that many of the s3c24xx do not bother to request the GPIOs before using them as interrupts. > 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? Do we really need to request the GPIO? I suppose it is useful to stop someone else trying to play with it... Another interesting problem is to do with using an GPIO that has been assigned to an interrupt as a readable resource in the driver, since the I would also consider that the implementor of the IRQs set_type configure the specific GPIO to the correct mode for operating as an interrupt when it is called... this would reduce any problems of passing GPIOs to drivers (especially if device tree is being used) to avoid having to do any arch-specific mode calls. > 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(-) > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 7:55 ` Ben Dooks 0 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 7:55 UTC (permalink / raw) To: linux-arm-kernel 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. Interesting, as this is not the case across all the ARM systems, as I believe that many of the s3c24xx do not bother to request the GPIOs before using them as interrupts. > 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? Do we really need to request the GPIO? I suppose it is useful to stop someone else trying to play with it... Another interesting problem is to do with using an GPIO that has been assigned to an interrupt as a readable resource in the driver, since the I would also consider that the implementor of the IRQs set_type configure the specific GPIO to the correct mode for operating as an interrupt when it is called... this would reduce any problems of passing GPIOs to drivers (especially if device tree is being used) to avoid having to do any arch-specific mode calls. > 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(-) > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 7:55 ` Ben Dooks 0 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 7:55 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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. Interesting, as this is not the case across all the ARM systems, as I believe that many of the s3c24xx do not bother to request the GPIOs before using them as interrupts. > 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? Do we really need to request the GPIO? I suppose it is useful to stop someone else trying to play with it... Another interesting problem is to do with using an GPIO that has been assigned to an interrupt as a readable resource in the driver, since the I would also consider that the implementor of the IRQs set_type configure the specific GPIO to the correct mode for operating as an interrupt when it is called... this would reduce any problems of passing GPIOs to drivers (especially if device tree is being used) to avoid having to do any arch-specific mode calls. > 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(-) > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it 2011-08-04 23:00 ` Stephen Warren @ 2011-08-05 9:40 ` Russell King - ARM Linux -1 siblings, 0 replies; 73+ messages in thread From: Russell King - ARM Linux @ 2011-08-05 9:40 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 9:40 ` Russell King - ARM Linux 0 siblings, 0 replies; 73+ messages in thread From: Russell King - ARM Linux @ 2011-08-05 9:40 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <20110805094017.GC20575-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org>]
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it 2011-08-05 9:40 ` Russell King - ARM Linux (?) @ 2011-08-05 10:30 ` Ben Dooks -1 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 10:30 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Stephen Warren, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw, Mark Brown, linux-mmc-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-tegra-u79uwXL29TY76Z2rM5mHXA, ccross-z5hGa2qSFaRBDgjK7y7TUQ, olof-nZhT3qVonbNeoWH0uzbU5w, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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). -- Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 10:30 ` Ben Dooks 0 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 10:30 UTC (permalink / raw) To: linux-arm-kernel 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). -- Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 10:30 ` Ben Dooks 0 siblings, 0 replies; 73+ messages in thread From: Ben Dooks @ 2011-08-05 10:30 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Stephen Warren, alsa-devel, Mark Brown, linux-mmc, linux-kernel, linux-tegra, ccross, olof, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel 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). -- Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/ Large Hadron Colada: A large Pina Colada that makes the universe disappear. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it 2011-08-05 10:30 ` Ben Dooks @ 2011-08-05 20:25 ` Linus Walleij -1 siblings, 0 replies; 73+ messages in thread From: Linus Walleij @ 2011-08-05 20:25 UTC (permalink / raw) To: Ben Dooks Cc: Russell King - ARM Linux, Stephen Warren, alsa-devel, Mark Brown, linux-mmc, linux-kernel, linux-tegra, ccross, olof, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel On Fri, Aug 5, 2011 at 12:30 PM, Ben Dooks <ben-linux@fluff.org> wrote: > On Fri, Aug 05, 2011 at 10:40:17AM +0100, Russell King - ARM Linux wrote: >> 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/ And I'm piling a few patches *deleting* irq_to_gpio() from platforms that define but actually don't use it, like U300 and SA1100. Linus Walleij ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 20:25 ` Linus Walleij 0 siblings, 0 replies; 73+ messages in thread From: Linus Walleij @ 2011-08-05 20:25 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 5, 2011 at 12:30 PM, Ben Dooks <ben-linux@fluff.org> wrote: > On Fri, Aug 05, 2011 at 10:40:17AM +0100, Russell King - ARM Linux wrote: >> 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/ And I'm piling a few patches *deleting* irq_to_gpio() from platforms that define but actually don't use it, like U300 and SA1100. Linus Walleij ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it 2011-08-05 9:40 ` Russell King - ARM Linux (?) @ 2011-08-05 15:43 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 15:43 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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) ... -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 15:43 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 15:43 UTC (permalink / raw) To: linux-arm-kernel 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) ... -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 15:43 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 15:43 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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) ... -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it 2011-08-05 15:43 ` Stephen Warren (?) @ 2011-08-05 19:15 ` Russell King - ARM Linux -1 siblings, 0 replies; 73+ messages in thread From: Russell King - ARM Linux @ 2011-08-05 19:15 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 19:15 ` Russell King - ARM Linux 0 siblings, 0 replies; 73+ messages in thread From: Russell King - ARM Linux @ 2011-08-05 19:15 UTC (permalink / raw) To: linux-arm-kernel 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@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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 19:15 ` Russell King - ARM Linux 0 siblings, 0 replies; 73+ messages in thread From: Russell King - ARM Linux @ 2011-08-05 19:15 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it 2011-08-05 19:15 ` Russell King - ARM Linux (?) @ 2011-08-05 19:33 ` Stephen Warren -1 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 19:33 UTC (permalink / raw) To: Russell King - ARM Linux Cc: alsa-devel, Mark Brown, linux-mmc, linux-kernel, linux-tegra, ccross, olof, Thomas Gleixner, Chris Ball, Liam Girdwood, linux-arm-kernel 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 19:33 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 19:33 UTC (permalink / raw) To: linux-arm-kernel 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* RE: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 19:33 ` Stephen Warren 0 siblings, 0 replies; 73+ messages in thread From: Stephen Warren @ 2011-08-05 19:33 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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. -- nvpublic ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it 2011-08-05 19:33 ` Stephen Warren (?) @ 2011-08-05 21:40 ` Russell King - ARM Linux -1 siblings, 0 replies; 73+ messages in thread From: Russell King - ARM Linux @ 2011-08-05 21:40 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 21:40 ` Russell King - ARM Linux 0 siblings, 0 replies; 73+ messages in thread From: Russell King - ARM Linux @ 2011-08-05 21:40 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it @ 2011-08-05 21:40 ` Russell King - ARM Linux 0 siblings, 0 replies; 73+ messages in thread From: Russell King - ARM Linux @ 2011-08-05 21:40 UTC (permalink / raw) To: Stephen Warren Cc: Thomas Gleixner, Mark Brown, Liam Girdwood, Chris Ball, ccross, olof, alsa-devel, linux-mmc, linux-kernel, linux-tegra, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 73+ messages in thread
end of thread, other threads:[~2011-09-02 15:50 UTC | newest] Thread overview: 73+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-08-04 23:00 [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it Stephen Warren 2011-08-04 23:00 ` Stephen Warren 2011-08-04 23:00 ` Stephen Warren [not found] ` <1312498820-2275-1-git-send-email-swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 2011-08-04 23:00 ` [PATCH 1/3] irq: " Stephen Warren 2011-08-04 23:00 ` Stephen Warren 2011-08-04 23:00 ` Stephen Warren 2011-08-05 0:01 ` Mark Brown 2011-08-05 0:01 ` Mark Brown 2011-08-05 0:01 ` Mark Brown [not found] ` <20110805000148.GB13321-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> 2011-08-05 3:53 ` Stephen Warren 2011-08-05 3:53 ` Stephen Warren 2011-08-05 3:53 ` Stephen Warren 2011-08-05 5:35 ` Mark Brown 2011-08-05 5:35 ` Mark Brown 2011-08-05 5:35 ` Mark Brown [not found] ` <20110805053510.GA16956-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> 2011-08-05 8:06 ` Ben Dooks 2011-08-05 8:06 ` Ben Dooks 2011-08-05 8:06 ` Ben Dooks 2011-08-05 8:29 ` Mark Brown 2011-08-05 8:29 ` Mark Brown 2011-08-05 8:29 ` Mark Brown 2011-08-05 15:29 ` Stephen Warren 2011-08-05 15:29 ` Stephen Warren 2011-08-05 15:29 ` Stephen Warren 2011-08-05 16:15 ` Mark Brown 2011-08-05 16:15 ` Mark Brown 2011-08-05 16:15 ` Mark Brown 2011-08-05 1:54 ` Rob Herring 2011-08-05 1:54 ` Rob Herring 2011-08-05 4:05 ` Stephen Warren 2011-08-05 4:05 ` Stephen Warren 2011-08-05 4:05 ` Stephen Warren 2011-08-05 7:58 ` Ben Dooks 2011-08-05 7:58 ` Ben Dooks 2011-09-02 8:36 ` Thomas Gleixner 2011-09-02 8:36 ` Thomas Gleixner 2011-09-02 15:24 ` Stephen Warren 2011-09-02 15:24 ` Stephen Warren 2011-09-02 15:24 ` Stephen Warren [not found] ` <74CDBE0F657A3D45AFBB94109FB122FF04B327A55C-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org> 2011-09-02 15:34 ` Stephen Warren 2011-09-02 15:34 ` Stephen Warren 2011-09-02 15:34 ` Stephen Warren 2011-09-02 15:50 ` Thomas Gleixner 2011-09-02 15:50 ` Thomas Gleixner 2011-09-02 15:50 ` Thomas Gleixner 2011-08-04 23:00 ` [PATCH 2/3] mmc: tegra: Don't gpio_request GPIOs used as IRQs Stephen Warren 2011-08-04 23:00 ` Stephen Warren 2011-08-04 23:00 ` Stephen Warren 2011-08-04 23:00 ` [PATCH 3/3] ASoC: jack_add_gpios: " Stephen Warren 2011-08-04 23:00 ` Stephen Warren 2011-08-04 23:00 ` Stephen Warren 2011-08-05 7:55 ` [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it Ben Dooks 2011-08-05 7:55 ` Ben Dooks 2011-08-05 7:55 ` Ben Dooks 2011-08-05 9:40 ` Russell King - ARM Linux 2011-08-05 9:40 ` Russell King - ARM Linux [not found] ` <20110805094017.GC20575-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org> 2011-08-05 10:30 ` Ben Dooks 2011-08-05 10:30 ` Ben Dooks 2011-08-05 10:30 ` Ben Dooks 2011-08-05 20:25 ` Linus Walleij 2011-08-05 20:25 ` Linus Walleij 2011-08-05 15:43 ` Stephen Warren 2011-08-05 15:43 ` Stephen Warren 2011-08-05 15:43 ` Stephen Warren 2011-08-05 19:15 ` Russell King - ARM Linux 2011-08-05 19:15 ` Russell King - ARM Linux 2011-08-05 19:15 ` Russell King - ARM Linux 2011-08-05 19:33 ` Stephen Warren 2011-08-05 19:33 ` Stephen Warren 2011-08-05 19:33 ` Stephen Warren 2011-08-05 21:40 ` Russell King - ARM Linux 2011-08-05 21:40 ` Russell King - ARM Linux 2011-08-05 21:40 ` Russell King - ARM Linux
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.