Add support functions to gpiolib to request/free gpio irqs.
Signed-off-by: H Hartley Sweeten <[email protected]>
Cc: David Brownell <[email protected]>
---
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 */
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 <[email protected]>
> Cc: David Brownell <[email protected]>
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.
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 <[email protected]>
> Cc: David Brownell <[email protected]>
>
> ---
>
> 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 */
>
>
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
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.
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