[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[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(-)

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[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(-)

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[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(-)

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[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

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[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

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[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

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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?

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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?

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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?

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Mark Brown]

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.

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Rob Herring]

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 */

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Rob Herring]

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 */

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

Re: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Thomas Gleixner]

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,

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Thomas Gleixner]

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,

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Thomas Gleixner]

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

[PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Thomas Gleixner]

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

RE: [PATCH 1/3] irq: If an IRQ is a GPIO, request and configure it

[Thomas Gleixner]

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

[PATCH 2/3] mmc: tegra: Don't gpio_request GPIOs used as IRQs.

[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

[PATCH 2/3] mmc: tegra: Don't gpio_request GPIOs used as IRQs.

[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

[PATCH 2/3] mmc: tegra: Don't gpio_request GPIOs used as IRQs.

[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

[PATCH 3/3] ASoC: jack_add_gpios: Don't gpio_request GPIOs used as IRQs.

[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

[PATCH 3/3] ASoC: jack_add_gpios: Don't gpio_request GPIOs used as IRQs.

[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

[PATCH 3/3] ASoC: jack_add_gpios: Don't gpio_request GPIOs used as IRQs.

[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

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Russell King - ARM Linux]

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.

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Russell King - ARM Linux]

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.

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Ben Dooks]

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.

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Linus Walleij]

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

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Linus Walleij]

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

RE: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Russell King - ARM Linux]

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.

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Russell King - ARM Linux]

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.

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Russell King - ARM Linux]

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.

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

RE: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Stephen Warren]

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

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Russell King - ARM Linux]

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.

[RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Russell King - ARM Linux]

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.

Re: [RFC PATCH 0/3] If an IRQ is a GPIO, request and configure it

[Russell King - ARM Linux]

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.