[PATCH] gpiolib: add gpio_request/free_irq

[PATCH] gpiolib: add gpio_request/free_irq

[H Hartley Sweeten]

Add support functions to gpiolib to request/free gpio irqs.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>

---

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..5b4b864 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1103,6 +1103,52 @@ int __gpio_to_irq(unsigned gpio)
 }
 EXPORT_SYMBOL_GPL(__gpio_to_irq);
 
+/**
+ * gpio_request_irq() - allocate a gpio interrupt line
+ * @gpio: gpio whose IRQ will be allocated
+ * @handler: function to be called when the IRQ occurs
+ * @irqflags: interrupt type flags
+ * @label: an ascii name for the claiming device
+ * @data: a cookie passed back to the handling function
+ */
+int gpio_request_irq(unsigned gpio, irq_handler_t handler,
+			unsigned long irqflags, const char *label, void *data)
+{
+	int err;
+
+	err = gpio_request(gpio, label);
+	if (err)
+		goto fail;
+
+	err = gpio_direction_input(gpio);
+	if (err)
+		goto free;
+
+	err = request_irq(gpio_to_irq(gpio), handler, irqflags, label, data);
+	if (err)
+		goto free;
+
+	return 0;
+
+free:
+	gpio_free(gpio);
+fail:
+	return err;
+}
+EXPORT_SYMBOL_GPL(gpio_request_irq);
+
+/**
+ * gpio_free_irq() - free an interrupt allocated with gpio_request_irq
+ * @irq: gpio interrupt line to free
+ * @data: device identity to free
+ */
+void gpio_free_irq(unsigned int irq, void *data)
+{
+	free_irq(irq, data);
+	gpio_free(irq_to_gpio(irq));
+}
+EXPORT_SYMBOL_GPL(gpio_free_irq);
+
 
 
 /* There's no value in making it easy to inline GPIO calls that may sleep.
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index d6c379d..c0ab7cf 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -3,6 +3,7 @@
 
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/interrupt.h>
 
 #ifdef CONFIG_GPIOLIB
 
@@ -134,6 +135,11 @@ extern int __gpio_cansleep(unsigned gpio);
 
 extern int __gpio_to_irq(unsigned gpio);
 
+/* request/free gpio interrupt */
+extern int gpio_request_irq(unsigned gpio, irq_handler_t handler,
+			unsigned long irqflags, const char *label, void *data);
+extern void gpio_free_irq(unsigned int irq, void *data);
+
 #ifdef CONFIG_GPIO_SYSFS
 
 /*
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index e10c49a..eaf7b27 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -109,6 +109,18 @@ static inline int irq_to_gpio(unsigned irq)
 	return -EINVAL;
 }
 
+static inline int gpio_request_irq(unsigned gpio, irq_handler_t handler,
+			unsigned long irqflags, const char *label, void *data)
+{
+	return -ENOSYS;
+}
+
+static inline void gpio_free_irq(unsigned int irq, void *data)
+{
+	/* GPIO irq can never have been requested */
+	WARN_ON(1);
+}
+
 #endif
 
 #endif /* __LINUX_GPIO_H */

Re: [PATCH] gpiolib: add gpio_request/free_irq

[Ben Nizette]

On Fri, 2009-06-05 at 11:51 -0700, H Hartley Sweeten wrote:
> Add support functions to gpiolib to request/free gpio irqs.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: David Brownell <dbrownell@users.sourceforge.net>

Can this really not just be open-coded?  I see other subsystems wrap up
request_irq but that tends to be in cases where there's a lot of
boilerplate to go through or it closes a race or something.  Maybe
there's just insufficient changelogging - why do we want this?

Thanks,
	--Ben.

Re: [PATCH] gpiolib: add gpio_request/free_irq

[David Brownell]

On Friday 05 June 2009, H Hartley Sweeten wrote:
> Add support functions to gpiolib to request/free gpio irqs.

I'm not keen on this.

 - At best it's a convenience layer ... for something that's
   not the least bit awkward to do otherwise.

 - Coupling it to gpiolib sort of defeats the point of saying
   that gpiolib is just an *implementation* of the interface.
   Where's the code to run for non-gpiolib platforms?

 - Since it implicitly couples gpio_request() to a flavor of
   request_irq(), it precludes sharing those IRQs.

Basically, board setup can know that the GPIO is being used
as an IRQ, and do the request()/direction_input() before it
passes gpio_to_irq() to the driver. That's worked in every
case I've happened across so far...

- Dave



> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> 
> ---
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 51a8d41..5b4b864 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1103,6 +1103,52 @@ int __gpio_to_irq(unsigned gpio)
>  }
>  EXPORT_SYMBOL_GPL(__gpio_to_irq);
>  
> +/**
> + * gpio_request_irq() - allocate a gpio interrupt line
> + * @gpio: gpio whose IRQ will be allocated
> + * @handler: function to be called when the IRQ occurs
> + * @irqflags: interrupt type flags
> + * @label: an ascii name for the claiming device
> + * @data: a cookie passed back to the handling function
> + */
> +int gpio_request_irq(unsigned gpio, irq_handler_t handler,
> +			unsigned long irqflags, const char *label, void *data)
> +{
> +	int err;
> +
> +	err = gpio_request(gpio, label);
> +	if (err)
> +		goto fail;
> +
> +	err = gpio_direction_input(gpio);
> +	if (err)
> +		goto free;
> +
> +	err = request_irq(gpio_to_irq(gpio), handler, irqflags, label, data);
> +	if (err)
> +		goto free;
> +
> +	return 0;
> +
> +free:
> +	gpio_free(gpio);
> +fail:
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(gpio_request_irq);
> +
> +/**
> + * gpio_free_irq() - free an interrupt allocated with gpio_request_irq
> + * @irq: gpio interrupt line to free
> + * @data: device identity to free
> + */
> +void gpio_free_irq(unsigned int irq, void *data)
> +{
> +	free_irq(irq, data);
> +	gpio_free(irq_to_gpio(irq));
> +}
> +EXPORT_SYMBOL_GPL(gpio_free_irq);
> +
>  
>  
>  /* There's no value in making it easy to inline GPIO calls that may sleep.
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index d6c379d..c0ab7cf 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -3,6 +3,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/errno.h>
> +#include <linux/interrupt.h>
>  
>  #ifdef CONFIG_GPIOLIB
>  
> @@ -134,6 +135,11 @@ extern int __gpio_cansleep(unsigned gpio);
>  
>  extern int __gpio_to_irq(unsigned gpio);
>  
> +/* request/free gpio interrupt */
> +extern int gpio_request_irq(unsigned gpio, irq_handler_t handler,
> +			unsigned long irqflags, const char *label, void *data);
> +extern void gpio_free_irq(unsigned int irq, void *data);
> +
>  #ifdef CONFIG_GPIO_SYSFS
>  
>  /*
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index e10c49a..eaf7b27 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -109,6 +109,18 @@ static inline int irq_to_gpio(unsigned irq)
>  	return -EINVAL;
>  }
>  
> +static inline int gpio_request_irq(unsigned gpio, irq_handler_t handler,
> +			unsigned long irqflags, const char *label, void *data)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline void gpio_free_irq(unsigned int irq, void *data)
> +{
> +	/* GPIO irq can never have been requested */
> +	WARN_ON(1);
> +}
> +
>  #endif
>  
>  #endif /* __LINUX_GPIO_H */
> 
> 

RE: [PATCH] gpiolib: add gpio_request/free_irq

[H Hartley Sweeten]

On Friday, June 05, 2009 10:20 PM, David Brownell wrote:
> On Friday 05 June 2009, H Hartley Sweeten wrote:
> Add support functions to gpiolib to request/free gpio irqs.
> 
> I'm not keen on this.
> 
>  - At best it's a convenience layer ... for something that's
>    not the least bit awkward to do otherwise.
> 

I agree it's a convenience layer, but it cleans up the error path in
many drivers that allocate a number of gpio irqs.  It also insures
that the gpios get requested before they are connected to an interrupt.
A number of drivers either don't do the gpio_request or they don't
set the gpio as an input before doing the request_irq.

>  - Coupling it to gpiolib sort of defeats the point of saying
>    that gpiolib is just an *implementation* of the interface.
>    Where's the code to run for non-gpiolib platforms?
> 

But if a driver is using the gpiolib interface calls wouldn't that
prevent that driver from working on a platform that does not support
gpiolib? File include/linux/gpio.h defines all the gpiolib calls to
either return an error code or WARN_ON(1) when called.

>  - Since it implicitly couples gpio_request() to a flavor of
>    request_irq(), it precludes sharing those IRQs.
> 

Can't the IRQ be shared by passing IRQF_SHARED as one of the flags?

> Basically, board setup can know that the GPIO is being used
> as an IRQ, and do the request()/direction_input() before it
> passes gpio_to_irq() to the driver. That's worked in every
> case I've happened across so far...

I agree it works as-is right now.  I just thought this would be a
convenient wrapper to handle a common setup step.  If it's overkill
or not appropriate to add to gpiolib please disregard the patch.

I have cleaned up the patch to be more inline with how the various
drivers would use it.  If there is any potential interest I will
repost the patch.

Thanks for your comments,
Hartley

Re: [PATCH] gpiolib: add gpio_request/free_irq

[David Brownell]

On Tuesday 09 June 2009, H Hartley Sweeten wrote:
> On Friday, June 05, 2009 10:20 PM, David Brownell wrote:
> > On Friday 05 June 2009, H Hartley Sweeten wrote:
> > Add support functions to gpiolib to request/free gpio irqs.
> > 
> > I'm not keen on this.
> > 
> >  - At best it's a convenience layer ... for something that's
> >    not the least bit awkward to do otherwise.
> 
> 		... deletia ...
> 
> >  - Coupling it to gpiolib sort of defeats the point of saying
> >    that gpiolib is just an *implementation* of the interface.
> >    Where's the code to run for non-gpiolib platforms?
> > 
> 
> But if a driver is using the gpiolib interface calls wouldn't that
> prevent that driver from working on a platform that does not support
> gpiolib? File include/linux/gpio.h defines all the gpiolib calls to
> either return an error code or WARN_ON(1) when called.

There are three kinds of config to be concerned with:

 - Platform doesn't support the GPIO calls at all.  In those
   cases, drivers using <linux/gpio.h> get NOP stubs; you have
   this case covered.

 - Platforms supporting the calls but not using gpiolib.
   That's the case I pointed out -- you don't handle it.

 - Platforms supporting the calls with gpiolib.  This is the
   other case you handle.


> >  - Since it implicitly couples gpio_request() to a flavor of
> >    request_irq(), it precludes sharing those IRQs.
> > 
> 
> Can't the IRQ be shared by passing IRQF_SHARED as one of the flags?

Only one of them will be able to gpio_request(), so it
doesn't matter at all what you say to request_irq().


> > Basically, board setup can know that the GPIO is being used
> > as an IRQ, and do the request()/direction_input() before it
> > passes gpio_to_irq() to the driver. That's worked in every
> > case I've happened across so far...
> 
> I agree it works as-is right now.  I just thought this would be a
> convenient wrapper to handle a common setup step.  If it's overkill
> or not appropriate to add to gpiolib please disregard the patch.

RE: [PATCH] gpiolib: add gpio_request/free_irq

[H Hartley Sweeten]

On Tuesday, June 09, 2009 5:56 PM, David Brownell wrote:
> On Tuesday 09 June 2009, H Hartley Sweeten wrote:
> > On Friday, June 05, 2009 10:20 PM, David Brownell wrote:
> > > On Friday 05 June 2009, H Hartley Sweeten wrote:
> > > Add support functions to gpiolib to request/free gpio irqs.
> > > 
> > > I'm not keen on this.
> > > 
> > >  - At best it's a convenience layer ... for something that's
> > >    not the least bit awkward to do otherwise.
> > 
> > 		... deletia ...
> > 
> > >  - Coupling it to gpiolib sort of defeats the point of saying
> > >    that gpiolib is just an *implementation* of the interface.
> > >    Where's the code to run for non-gpiolib platforms?
> > > 
> 
> There are three kinds of config to be concerned with:
> 
>  - Platform doesn't support the GPIO calls at all.  In those
>    cases, drivers using <linux/gpio.h> get NOP stubs; you have
>    this case covered.
> 
>  - Platforms supporting the calls but not using gpiolib.
>    That's the case I pointed out -- you don't handle it.
> 

Ah.. Missed that point. I assume this is when GENERIC_GPIO=y
but GPIOLIB (ARCH_REQUIRE_GPIOLIB?) is not defined. Correct?

I'll continue looking it over to see if there is clean way to handle
that case.

>  - Platforms supporting the calls with gpiolib.  This is the
>    other case you handle.


> > >  - Since it implicitly couples gpio_request() to a flavor of
> > >    request_irq(), it precludes sharing those IRQs.
> > > 
> > 
> > Can't the IRQ be shared by passing IRQF_SHARED as one of the flags?
> 
> Only one of them will be able to gpio_request(), so it
> doesn't matter at all what you say to request_irq().

But the gpio_request() is for a given gpio, which can only be requested
once.  The request_irq() call is using the irq value returned by
gpio_to_irq() which could return the same irq number for multiple irq's,
wouldn't these then get shared as long as the IRQF_SHARED flag is set?

> > > Basically, board setup can know that the GPIO is being used
> > > as an IRQ, and do the request()/direction_input() before it
> > > passes gpio_to_irq() to the driver. That's worked in every
> > > case I've happened across so far...
> > 
> > I agree it works as-is right now.  I just thought this would be a
> > convenient wrapper to handle a common setup step.  If it's overkill
> > or not appropriate to add to gpiolib please disregard the patch.

Thanks for your comments,
Hartley