[PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Alek Du]

>From 7a76916ccea4a376a260ea67fbc79ac4d958757f Mon Sep 17 00:00:00 2001
From: Alek Du <alek.du@intel.com>
Date: Fri, 8 May 2009 09:46:49 +0800
Subject: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

Add some more functions to GPIOLIB, they are:
* gpio_detect is to set GPIO interrupt triggering method (edge, level, high,
  low, etc.)
* gpio_debounce is to set GPIO trigger HW debounce value if GPIO hw supports.
* gpio_alt_func is to set pin as alternative function or GPIO.

We will submit GPIO drivers that leverage this new interface later.

Signed-off-by: Alek Du <alek.du@intel.com>
---
 drivers/gpio/gpiolib.c     |   33 +++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h |   25 ++++++++++++++++++++++++-
 include/linux/gpio.h       |   15 +++++++++++++++
 3 files changed, 72 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..087efce 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1066,6 +1066,39 @@ void __gpio_set_value(unsigned gpio, int value)
 }
 EXPORT_SYMBOL_GPL(__gpio_set_value);
 
+void gpio_detect(unsigned gpio, enum gpio_trigger_t value)
+{
+	struct gpio_chip	*chip;
+
+	chip = gpio_to_chip(gpio);
+	WARN_ON(extra_checks && chip->can_sleep);
+	if (chip->detect)
+		chip->detect(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(gpio_detect);
+
+void gpio_debounce(unsigned gpio, int value)
+{
+	struct gpio_chip	*chip;
+
+	chip = gpio_to_chip(gpio);
+	WARN_ON(extra_checks && chip->can_sleep);
+	if (chip->debounce)
+		chip->debounce(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(gpio_debounce);
+
+void gpio_alt_func(unsigned gpio, int value)
+{
+	struct gpio_chip	*chip;
+
+	chip = gpio_to_chip(gpio);
+	WARN_ON(extra_checks && chip->can_sleep);
+	if (chip->alt_func)
+		chip->alt_func(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(gpio_alt_func);
+
 /**
  * __gpio_cansleep() - report whether gpio value access will sleep
  * @gpio: gpio in question
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index d6c379d..de9aaf9 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -30,6 +30,15 @@ static inline int gpio_is_valid(int number)
 struct seq_file;
 struct module;
 
+enum gpio_trigger_t {
+	DETECT_LEVEL_LOW = -2,
+	DETECT_EDGE_FALLING = -1,
+	DETECT_DISABLE = 0,
+	DETECT_EDGE_RISING = 1,
+	DETECT_LEVEL_HIGH = 2,
+	DETECT_EDGE_BOTH = 3,
+};
+
 /**
  * struct gpio_chip - abstract a GPIO controller
  * @label: for diagnostics
@@ -44,6 +53,9 @@ struct module;
  *	returns either the value actually sensed, or zero
  * @direction_output: configures signal "offset" as output, or returns error
  * @set: assigns output value for signal "offset"
+ * @detect: configures signal interrupt detection method, see gpio_trigger_t
+ * @debounce: configures signal interrupt detection debounce
+ * @alt_func: configure signal as GPIO or alternative function
  * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
  *	implementation may not sleep
  * @dbg_show: optional routine to show contents in debugfs; default code
@@ -89,6 +101,15 @@ struct gpio_chip {
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
 
+	void			(*detect)(struct gpio_chip *chip,
+						unsigned offset,
+						enum gpio_trigger_t value);
+	void			(*debounce)(struct gpio_chip *chip,
+						unsigned offset,
+						int value);
+	void			(*alt_func)(struct gpio_chip *chip,
+						unsigned offset, int value);
+
 	int			(*to_irq)(struct gpio_chip *chip,
 						unsigned offset);
 
@@ -118,7 +139,9 @@ extern void gpio_free(unsigned gpio);
 
 extern int gpio_direction_input(unsigned gpio);
 extern int gpio_direction_output(unsigned gpio, int value);
-
+extern void gpio_detect(unsigned gpio, enum gpio_trigger_t value);
+extern void gpio_debounce(unsigned gpio, int value);
+extern void gpio_alt_func(unsigned gpio, int value);
 extern int gpio_get_value_cansleep(unsigned gpio);
 extern void gpio_set_value_cansleep(unsigned gpio, int value);
 
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index e10c49a..76dd5f9 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -82,6 +82,21 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
 	WARN_ON(1);
 }
 
+static inline void gpio_detect(unsigned gpio, int value)
+{
+	WARN_ON(1);
+}
+
+static inline void gpio_debounce(unsigned gpio, int value)
+{
+	WARN_ON(1);
+}
+
+static inline void gpio_alt_func(unsigned gpio, int value)
+{
+	WARN_ON(1);
+}
+
 static inline int gpio_export(unsigned gpio, bool direction_may_change)
 {
 	/* GPIO can never have been requested or set as {in,out}put */
-- 
1.6.0.4

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Dooks]

On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> >From 7a76916ccea4a376a260ea67fbc79ac4d958757f Mon Sep 17 00:00:00 2001
> From: Alek Du <alek.du@intel.com>
> Date: Fri, 8 May 2009 09:46:49 +0800
> Subject: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB
> 
> Add some more functions to GPIOLIB, they are:
> * gpio_detect is to set GPIO interrupt triggering method (edge, level, high,
>   low, etc.)
> * gpio_debounce is to set GPIO trigger HW debounce value if GPIO hw supports.
> * gpio_alt_func is to set pin as alternative function or GPIO.

gpio_alt_func is feature creep, I don't really belive this is the
best place to put it as it will be difficult to actually make this
generic for all gpio platforms.

> +enum gpio_trigger_t {
> +	DETECT_LEVEL_LOW = -2,
> +	DETECT_EDGE_FALLING = -1,
> +	DETECT_DISABLE = 0,
> +	DETECT_EDGE_RISING = 1,
> +	DETECT_LEVEL_HIGH = 2,
> +	DETECT_EDGE_BOTH = 3,
> +};

wny not reuse the IRQ trigger types?

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Mark Brown]

On Mon, Jun 15, 2009 at 10:50:23AM +0100, Ben Dooks wrote:
> On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:

> > * gpio_alt_func is to set pin as alternative function or GPIO.

> gpio_alt_func is feature creep, I don't really belive this is the
> best place to put it as it will be difficult to actually make this
> generic for all gpio platforms.

Since the proposed API just passes a value through to the driver for the
GPIO chip it looks generic enough - each chip can define whatever set of
constants it likes.  I'd expect a large proportion of driver specific
APIs would end up just the same.

Given the number of manufacturers that don't use a separate term like
the PXA MFP for the alternative functions of their GPIOs it makes sense
to have a gpiolib API for this.  Without one you end up having each
driver needing to add its own API, and since the pins are just referred
to as GPIOs in the documentation the API will have that in the name and
look like it ought to be connected with gpiolib.

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Alek Du]

On Mon, 15 Jun 2009 18:02:53 +0800
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> On Mon, Jun 15, 2009 at 10:50:23AM +0100, Ben Dooks wrote:
> > On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> 
> > > * gpio_alt_func is to set pin as alternative function or GPIO.
> 
> > gpio_alt_func is feature creep, I don't really belive this is the
> > best place to put it as it will be difficult to actually make this
> > generic for all gpio platforms.
> 
> Since the proposed API just passes a value through to the driver for the
> GPIO chip it looks generic enough - each chip can define whatever set of
> constants it likes.  I'd expect a large proportion of driver specific
> APIs would end up just the same.
> 
> Given the number of manufacturers that don't use a separate term like
> the PXA MFP for the alternative functions of their GPIOs it makes sense
> to have a gpiolib API for this.  Without one you end up having each
> driver needing to add its own API, and since the pins are just referred
> to as GPIOs in the documentation the API will have that in the name and
> look like it ought to be connected with gpiolib.

Mark,
Thanks for the comments. I do believe that API would benefit some GPIO device
drivers. I'm preparing a GPIO driver for one Intel IOH that has GPIO block
needs that API.

Thanks,
Alek

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Alek Du]

On Mon, 15 Jun 2009 17:50:23 +0800
Ben Dooks <ben-linux@fluff.org> wrote:

> > +enum gpio_trigger_t {
> > +	DETECT_LEVEL_LOW = -2,
> > +	DETECT_EDGE_FALLING = -1,
> > +	DETECT_DISABLE = 0,
> > +	DETECT_EDGE_RISING = 1,
> > +	DETECT_LEVEL_HIGH = 2,
> > +	DETECT_EDGE_BOTH = 3,
> > +};
> 
> wny not reuse the IRQ trigger types?
> 

Don't know it this is a good reason: the IRQ trigger name IRQ_TYPE_*
seems not good as DETECT_*. Since not all GPIO device would have IRQ line
connected to interrupt controller -- some of them just set a trigger bit
when detection happens.

Thanks,
Alek

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Dooks]

On Mon, Jun 15, 2009 at 11:02:53AM +0100, Mark Brown wrote:
> On Mon, Jun 15, 2009 at 10:50:23AM +0100, Ben Dooks wrote:
> > On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> 
> > > * gpio_alt_func is to set pin as alternative function or GPIO.
> 
> > gpio_alt_func is feature creep, I don't really belive this is the
> > best place to put it as it will be difficult to actually make this
> > generic for all gpio platforms.
> 
> Since the proposed API just passes a value through to the driver for the
> GPIO chip it looks generic enough - each chip can define whatever set of
> constants it likes.  I'd expect a large proportion of driver specific
> APIs would end up just the same.
> 
> Given the number of manufacturers that don't use a separate term like
> the PXA MFP for the alternative functions of their GPIOs it makes sense
> to have a gpiolib API for this.  Without one you end up having each
> driver needing to add its own API, and since the pins are just referred
> to as GPIOs in the documentation the API will have that in the name and
> look like it ought to be connected with gpiolib.

Yes, however I can see some horrible problems ahead as soon as people
try and then try and standardise the values passed through this. The
GPIO API was meant to be a lightweight way of allowing drivers at
GPIOs, now everyone seems to want to push whatever they feel like in.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Dooks]

On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> >From 7a76916ccea4a376a260ea67fbc79ac4d958757f Mon Sep 17 00:00:00 2001
> From: Alek Du <alek.du@intel.com>
> Date: Fri, 8 May 2009 09:46:49 +0800
> Subject: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB
> 
> Add some more functions to GPIOLIB, they are:
> * gpio_detect is to set GPIO interrupt triggering method (edge, level, high,
>   low, etc.)

This is the wrong way of doing it, there is as a definit set_type
method in the irq_chip structure which should be used to alter the
way the IRQ is triggered.

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Florian Fainelli]

Le Monday 15 June 2009 13:19:16 Alek Du, vous avez écrit :
> On Mon, 15 Jun 2009 18:02:53 +0800
>
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> > On Mon, Jun 15, 2009 at 10:50:23AM +0100, Ben Dooks wrote:
> > > On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> > > > * gpio_alt_func is to set pin as alternative function or GPIO.
> > >
> > > gpio_alt_func is feature creep, I don't really belive this is the
> > > best place to put it as it will be difficult to actually make this
> > > generic for all gpio platforms.
> >
> > Since the proposed API just passes a value through to the driver for the
> > GPIO chip it looks generic enough - each chip can define whatever set of
> > constants it likes.  I'd expect a large proportion of driver specific
> > APIs would end up just the same.
> >
> > Given the number of manufacturers that don't use a separate term like
> > the PXA MFP for the alternative functions of their GPIOs it makes sense
> > to have a gpiolib API for this.  Without one you end up having each
> > driver needing to add its own API, and since the pins are just referred
> > to as GPIOs in the documentation the API will have that in the name and
> > look like it ought to be connected with gpiolib.
>
> Mark,
> Thanks for the comments. I do believe that API would benefit some GPIO
> device drivers. I'm preparing a GPIO driver for one Intel IOH that has GPIO
> block needs that API.

This is indeed useful, RB532 in the MIPS tree also has an alternate function 
setting feature.

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Florian Fainelli]

Le Monday 15 June 2009 14:51:46 Ben Dooks, vous avez écrit :
> On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> > >From 7a76916ccea4a376a260ea67fbc79ac4d958757f Mon Sep 17 00:00:00 2001
> >
> > From: Alek Du <alek.du@intel.com>
> > Date: Fri, 8 May 2009 09:46:49 +0800
> > Subject: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and
> > gpio_alt_func features to GPIOLIB
> >
> > Add some more functions to GPIOLIB, they are:
> > * gpio_detect is to set GPIO interrupt triggering method (edge, level,
> > high, low, etc.)

The name does not seem to reflect what it does, what about gpio_set_type or 
gpio_set_int_type for instance ?

>
> This is the wrong way of doing it, there is as a definit set_type
> method in the irq_chip structure which should be used to alter the
> way the IRQ is triggered.

I would expect your architecture IRQ handler to have a set_type callback for 
the GPIO lines capables of generating an interrupt. See how we have beeing 
doing it for rb532 for instance: 
http://www.linux-mips.org/git?p=linux-queue.git;a=blob;f=arch/mips/rb532/irq.c;h=f07882029a90d3d155c17b462812c2936229458c;hb=HEAD#l173

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Mark Brown]

On Mon, Jun 15, 2009 at 01:50:25PM +0100, Ben Dooks wrote:
> On Mon, Jun 15, 2009 at 11:02:53AM +0100, Mark Brown wrote:

> > Since the proposed API just passes a value through to the driver for the
> > GPIO chip it looks generic enough - each chip can define whatever set of

> Yes, however I can see some horrible problems ahead as soon as people
> try and then try and standardise the values passed through this. The

I fully expect that if anyone tries to do that all the GPIO driver
authors will turn round and tell them not to be so silly.

> GPIO API was meant to be a lightweight way of allowing drivers at
> GPIOs, now everyone seems to want to push whatever they feel like in.

This doesn't feel heavyweight to me.  I think it's better to have this
sort of widely implemented stuff there in the framework rather than
having a large proportion of GPIO drivers having to implement their own
(very similar) APIs on the side - as soon as drivers start having to do
that they start feeling unpleasant.

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Dooks]

On Mon, Jun 15, 2009 at 03:04:50PM +0200, Florian Fainelli wrote:
> Le Monday 15 June 2009 14:51:46 Ben Dooks, vous avez écrit :
> > On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> > > >From 7a76916ccea4a376a260ea67fbc79ac4d958757f Mon Sep 17 00:00:00 2001
> > >
> > > From: Alek Du <alek.du@intel.com>
> > > Date: Fri, 8 May 2009 09:46:49 +0800
> > > Subject: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and
> > > gpio_alt_func features to GPIOLIB
> > >
> > > Add some more functions to GPIOLIB, they are:
> > > * gpio_detect is to set GPIO interrupt triggering method (edge, level,
> > > high, low, etc.)
> 
> The name does not seem to reflect what it does, what about gpio_set_type or 
> gpio_set_int_type for instance ?

You seem to have made the same case as myself to why this call is irrelevant
to the gpiolib layer.
 
> >
> > This is the wrong way of doing it, there is as a definit set_type
> > method in the irq_chip structure which should be used to alter the
> > way the IRQ is triggered.
> 
> I would expect your architecture IRQ handler to have a set_type callback for 
> the GPIO lines capables of generating an interrupt. See how we have beeing 
> doing it for rb532 for instance: 
> http://www.linux-mips.org/git?p=linux-queue.git;a=blob;f=arch/mips/rb532/irq.c;h=f07882029a90d3d155c17b462812c2936229458c;hb=HEAD#l173

Yes, or if the GPIO driver is exporting interrupts, the relevant handler
for that chip should have the .set_type method defined.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Alek Du]

On Mon, 15 Jun 2009 21:04:50 +0800
Florian Fainelli <florian@openwrt.org> wrote:

> Le Monday 15 June 2009 14:51:46 Ben Dooks, vous avez écrit :
> > On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> > > >From 7a76916ccea4a376a260ea67fbc79ac4d958757f Mon Sep 17 00:00:00 2001
> > >
> > > From: Alek Du <alek.du@intel.com>
> > > Date: Fri, 8 May 2009 09:46:49 +0800
> > > Subject: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and
> > > gpio_alt_func features to GPIOLIB
> > >
> > > Add some more functions to GPIOLIB, they are:
> > > * gpio_detect is to set GPIO interrupt triggering method (edge, level,
> > > high, low, etc.)
> 
> The name does not seem to reflect what it does, what about gpio_set_type or 
> gpio_set_int_type for instance ?
> 
> >
> > This is the wrong way of doing it, there is as a definit set_type
> > method in the irq_chip structure which should be used to alter the
> > way the IRQ is triggered.
> 
> I would expect your architecture IRQ handler to have a set_type callback for 
> the GPIO lines capables of generating an interrupt. See how we have beeing 
> doing it for rb532 for instance: 
> http://www.linux-mips.org/git?p=linux-queue.git;a=blob;f=arch/mips/rb532/irq.c;h=f07882029a90d3d155c17b462812c2936229458c;hb=HEAD#l173


For you case, in set_type handler it calls a "rb532_gpio_set_ilevel", my purpose of the gpio_detect is to provide the general way to replace 
the private function like "rb532_gpio_set_ilevel". Suppose you have several type GPIO devices in the system, in you way, you have to export
different private function to set each GPIO device triggering mode. Also for some system, we have i2c or SPI GPIO expander, we need to hook
them on the fly. The gpio_detect API provide a general way to set triggering mode, you could call it under set_type callback.

Thanks,
Alek

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Alek Du]

On Mon, 15 Jun 2009 21:09:06 +0800
Ben Dooks <ben-linux@fluff.org> wrote:

> > 
> > I would expect your architecture IRQ handler to have a set_type callback for 
> > the GPIO lines capables of generating an interrupt. See how we have beeing 
> > doing it for rb532 for instance: 
> > http://www.linux-mips.org/git?p=linux-queue.git;a=blob;f=arch/mips/rb532/irq.c;h=f07882029a90d3d155c17b462812c2936229458c;hb=HEAD#l173
> 
> Yes, or if the GPIO driver is exporting interrupts, the relevant handler
> for that chip should have the .set_type method defined.
> 

In the .set_type method, you finally will call the GPIO driver's function to set interrupt trigger mode, right?
Current GPIOLIB do not provide such an interface. Current driver always exports a separate function to do that --
that's not good.
My patch provide a general API to do that.


Thanks,
Alek

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Dooks]

On Tue, Jun 16, 2009 at 09:28:48AM +0800, Alek Du wrote:
> On Mon, 15 Jun 2009 21:09:06 +0800
> Ben Dooks <ben-linux@fluff.org> wrote:
> 
> > > 
> > > I would expect your architecture IRQ handler to have a set_type callback for 
> > > the GPIO lines capables of generating an interrupt. See how we have beeing 
> > > doing it for rb532 for instance: 
> > > http://www.linux-mips.org/git?p=linux-queue.git;a=blob;f=arch/mips/rb532/irq.c;h=f07882029a90d3d155c17b462812c2936229458c;hb=HEAD#l173
> > 
> > Yes, or if the GPIO driver is exporting interrupts, the relevant handler
> > for that chip should have the .set_type method defined.
> > 
> 
> In the .set_type method, you finally will call the GPIO driver's function to set interrupt trigger mode, right?

No, that's totaly the wrong way around. GPIOLIB provides an GPIO to IRQ
function that the driver providing the GPIOLIB chip needs to provide. To do
IRQs, the same driver will have to provide a irq chip and that is the place
where this functionality should reside.

> Current GPIOLIB do not provide such an interface. Current driver always exports a separate function to do that --
> that's not good.
> My patch provide a general API to do that.

When there's already an extant API to do that. There are drivers already
doing it this way.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Dooks]

On Tue, Jun 16, 2009 at 09:21:21AM +0800, Alek Du wrote:
> On Mon, 15 Jun 2009 21:04:50 +0800
> Florian Fainelli <florian@openwrt.org> wrote:
> 
> > Le Monday 15 June 2009 14:51:46 Ben Dooks, vous avez écrit :
> > > On Mon, Jun 15, 2009 at 05:15:02PM +0800, Alek Du wrote:
> > > > >From 7a76916ccea4a376a260ea67fbc79ac4d958757f Mon Sep 17 00:00:00 2001
> > > >
> > > > From: Alek Du <alek.du@intel.com>
> > > > Date: Fri, 8 May 2009 09:46:49 +0800
> > > > Subject: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and
> > > > gpio_alt_func features to GPIOLIB
> > > >
> > > > Add some more functions to GPIOLIB, they are:
> > > > * gpio_detect is to set GPIO interrupt triggering method (edge, level,
> > > > high, low, etc.)
> > 
> > The name does not seem to reflect what it does, what about gpio_set_type or 
> > gpio_set_int_type for instance ?
> > 
> > >
> > > This is the wrong way of doing it, there is as a definit set_type
> > > method in the irq_chip structure which should be used to alter the
> > > way the IRQ is triggered.
> > 
> > I would expect your architecture IRQ handler to have a set_type callback for 
> > the GPIO lines capables of generating an interrupt. See how we have beeing 
> > doing it for rb532 for instance: 
> > http://www.linux-mips.org/git?p=linux-queue.git;a=blob;f=arch/mips/rb532/irq.c;h=f07882029a90d3d155c17b462812c2936229458c;hb=HEAD#l173
> 
> 
> For you case, in set_type handler it calls a "rb532_gpio_set_ilevel", my purpose of the gpio_detect is to provide the general way to replace 
> the private function like "rb532_gpio_set_ilevel". Suppose you have several type GPIO devices in the system, in you way, you have to export
> different private function to set each GPIO device triggering mode. Also for some system, we have i2c or SPI GPIO expander, we need to hook
> them on the fly. The gpio_detect API provide a general way to set triggering mode, you could call it under set_type callback.

Somehow you are still missing my point, this is the way it can be done
at the moment, without any extension to the API!

int attach_my_gpio_irq(int gpio, void *irqpw)
{
	int ret;
	int irq;

	irq = gpio_to_irq(gpio);
	if (irq < 0) {
		printk(KERN_ERR "%s: no gpio irq available\n", __func__);
		return irq;
	}

	ret = request_irq(irq, my_irq_handler, IRQF_TRIGGER_FALLING,
			  "mygpio", irqpw);
	if (ret < 0) 
		printk(KERN_ERR "%s: cannot request irq\n", __func__);

	return ret;
}

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Alek Du]

On Tue, 16 Jun 2009 16:45:24 +0800
Ben Dooks <ben-linux@fluff.org> wrote:

under set_type callback.
> 
> Somehow you are still missing my point, this is the way it can be done
> at the moment, without any extension to the API!
> 
> int attach_my_gpio_irq(int gpio, void *irqpw)
> {
> 	int ret;
> 	int irq;
> 
> 	irq = gpio_to_irq(gpio);
> 	if (irq < 0) {
> 		printk(KERN_ERR "%s: no gpio irq available\n", __func__);
> 		return irq;
> 	}
> 
> 	ret = request_irq(irq, my_irq_handler, IRQF_TRIGGER_FALLING,
> 			  "mygpio", irqpw);
> 	if (ret < 0) 
> 		printk(KERN_ERR "%s: cannot request irq\n", __func__);
> 
> 	return ret;
> }
> 
I mean, in the set_type of the irq_chip driver, you need to call something like rb532_gpio_set_ilevel.
The GPIO driver needs to export rb532_gpio_set_ilevel, and this is specific to each GPIO driver. Ideally,
I should always call gpio_detect(gpio, ...) here.

static int rb532_set_type(unsigned int irq_nr, unsigned type)
{
	int gpio = irq_nr - GPIO_MAPPED_IRQ_BASE;
	int group = irq_to_group(irq_nr);

	if (group != GPIO_MAPPED_IRQ_GROUP || irq_nr > (GROUP4_IRQ_BASE + 13))
		return (type == IRQ_TYPE_LEVEL_HIGH) ? 0 : -EINVAL;

	switch (type) {
	case IRQ_TYPE_LEVEL_HIGH:
		rb532_gpio_set_ilevel(1, gpio);
		break;
	case IRQ_TYPE_LEVEL_LOW:
		rb532_gpio_set_ilevel(0, gpio);
		break;
	default:
		return -EINVAL;
	}
	return 0;
}

Re: [PATCH] gpiolib: Add gpio_detect, gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Dooks]

On Tue, Jun 16, 2009 at 04:51:35PM +0800, Alek Du wrote:
> On Tue, 16 Jun 2009 16:45:24 +0800
> Ben Dooks <ben-linux@fluff.org> wrote:
> 
> under set_type callback.
> > 
> > Somehow you are still missing my point, this is the way it can be done
> > at the moment, without any extension to the API!
> > 
> > int attach_my_gpio_irq(int gpio, void *irqpw)
> > {
> > 	int ret;
> > 	int irq;
> > 
> > 	irq = gpio_to_irq(gpio);
> > 	if (irq < 0) {
> > 		printk(KERN_ERR "%s: no gpio irq available\n", __func__);
> > 		return irq;
> > 	}
> > 
> > 	ret = request_irq(irq, my_irq_handler, IRQF_TRIGGER_FALLING,
> > 			  "mygpio", irqpw);
> > 	if (ret < 0) 
> > 		printk(KERN_ERR "%s: cannot request irq\n", __func__);
> > 
> > 	return ret;
> > }
> > 
> I mean, in the set_type of the irq_chip driver, you need to call something like rb532_gpio_set_ilevel.

The correct place is for the code to live in the set_type of the IRQ chip,
I see no reason for this extra complexity of adding stuff to the GPIOLIB
where there is already an interface for it.

You seem to be trying to export some internal to the driver function
implementing an extant interface for no gain to the system as a whole.

> The GPIO driver needs to export rb532_gpio_set_ilevel, and this is specific to each GPIO driver. Ideally,
> I should always call gpio_detect(gpio, ...) here.
> 
> static int rb532_set_type(unsigned int irq_nr, unsigned type)
> {
> 	int gpio = irq_nr - GPIO_MAPPED_IRQ_BASE;
> 	int group = irq_to_group(irq_nr);
> 
> 	if (group != GPIO_MAPPED_IRQ_GROUP || irq_nr > (GROUP4_IRQ_BASE + 13))
> 		return (type == IRQ_TYPE_LEVEL_HIGH) ? 0 : -EINVAL;
> 
> 	switch (type) {
> 	case IRQ_TYPE_LEVEL_HIGH:
> 		rb532_gpio_set_ilevel(1, gpio);
> 		break;
> 	case IRQ_TYPE_LEVEL_LOW:
> 		rb532_gpio_set_ilevel(0, gpio);
> 		break;
> 	default:
> 		return -EINVAL;
> 	}
> 	return 0;
> }

My point is that there is an already extant way of configuring
IRQ polarities and types. This interface is exported from something
you must already implement if you want IRQs from GPIOs.

so:

1) This interface exists in the irq_chip struct
2) The GPIO driver needs to implement irq_chip to provide interrupts
3) Therefore the functionality is already covered.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

[PATCH v2] gpiolib: Add gpio_debounce and gpio_alt_func features to GPIOLIB

[Alek Du]

Changes from v1:

Removed gpio_detect since we should do that with irq_chip.set_type function.


>From 6b3c9398acf338c263170fcb74c0b2b345ad5369 Mon Sep 17 00:00:00 2001
From: Alek Du <alek.du@intel.com>
Date: Wed, 17 Jun 2009 14:50:51 +0800
Subject: [PATCH] GPIO: Add gpio_debounce and gpio_alt_func features to GPIOLIB

Add gpio_debounce and gpio_alt_func features to GPIOLIB:
* gpio_debounce is to adjust signal HW debounce value (need HW support)
* gpio_alt_func is to set GPIO as alternative function (need HW support)

Signed-off-by: Alek Du <alek.du@intel.com>
---
 drivers/gpio/gpiolib.c     |   22 ++++++++++++++++++++++
 include/asm-generic/gpio.h |   11 ++++++++++-
 include/linux/gpio.h       |   10 ++++++++++
 3 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..6365038 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1066,6 +1066,28 @@ void __gpio_set_value(unsigned gpio, int value)
 }
 EXPORT_SYMBOL_GPL(__gpio_set_value);
 
+void gpio_debounce(unsigned gpio, int value)
+{
+	struct gpio_chip	*chip;
+
+	chip = gpio_to_chip(gpio);
+	WARN_ON(extra_checks && chip->can_sleep);
+	if (chip->debounce)
+		chip->debounce(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(gpio_debounce);
+
+void gpio_alt_func(unsigned gpio, int value)
+{
+	struct gpio_chip	*chip;
+
+	chip = gpio_to_chip(gpio);
+	WARN_ON(extra_checks && chip->can_sleep);
+	if (chip->alt_func)
+		chip->alt_func(chip, gpio - chip->base, value);
+}
+EXPORT_SYMBOL_GPL(gpio_alt_func);
+
 /**
  * __gpio_cansleep() - report whether gpio value access will sleep
  * @gpio: gpio in question
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index d6c379d..bc98c96 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -44,6 +44,8 @@ struct module;
  *	returns either the value actually sensed, or zero
  * @direction_output: configures signal "offset" as output, or returns error
  * @set: assigns output value for signal "offset"
+ * @debounce: Adjust signal hardware debounce level
+ * @alt_func: configures signal as GPIO or alternative function
  * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
  *	implementation may not sleep
  * @dbg_show: optional routine to show contents in debugfs; default code
@@ -89,6 +91,12 @@ struct gpio_chip {
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
 
+	void			(*debounce)(struct gpio_chip *chip,
+						unsigned offset,
+						int value);
+	void			(*alt_func)(struct gpio_chip *chip,
+						unsigned offset, int value);
+
 	int			(*to_irq)(struct gpio_chip *chip,
 						unsigned offset);
 
@@ -118,7 +126,8 @@ extern void gpio_free(unsigned gpio);
 
 extern int gpio_direction_input(unsigned gpio);
 extern int gpio_direction_output(unsigned gpio, int value);
-
+extern void gpio_debounce(unsigned gpio, int value);
+extern void gpio_alt_func(unsigned gpio, int value);
 extern int gpio_get_value_cansleep(unsigned gpio);
 extern void gpio_set_value_cansleep(unsigned gpio, int value);
 
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index e10c49a..cf005d5 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -82,6 +82,16 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
 	WARN_ON(1);
 }
 
+static inline void gpio_debounce(unsigned gpio, int value)
+{
+	WARN_ON(1);
+}
+
+static inline void gpio_alt_func(unsigned gpio, int value)
+{
+	WARN_ON(1);
+}
+
 static inline int gpio_export(unsigned gpio, bool direction_may_change)
 {
 	/* GPIO can never have been requested or set as {in,out}put */
-- 
1.6.0.4

Re: [PATCH v2] gpiolib: Add gpio_debounce and gpio_alt_func features to GPIOLIB

[Ben Nizette]

On Wed, 2009-06-17 at 14:59 +0800, Alek Du wrote:
> Changes from v1:
> 
> Removed gpio_detect since we should do that with irq_chip.set_type function.
> 
> 
> From 6b3c9398acf338c263170fcb74c0b2b345ad5369 Mon Sep 17 00:00:00 2001
> From: Alek Du <alek.du@intel.com>
> Date: Wed, 17 Jun 2009 14:50:51 +0800
> Subject: [PATCH] GPIO: Add gpio_debounce and gpio_alt_func features to GPIOLIB
> 
> Add gpio_debounce and gpio_alt_func features to GPIOLIB:
> * gpio_debounce is to adjust signal HW debounce value (need HW support)
> * gpio_alt_func is to set GPIO as alternative function (need HW support)


Please justify the existence of these functions, particularly, who's
supposed to actually call them?  There's no real changelogging here.

First the hardware debounce call.  Hardware debounce functionality
varies massively between chips.  Therefore a driver cannot depend on any
particular behaviour unless it already knows what platform it's running
on.  If it knows the platform it can access the functions directly and
the interface needs no abstraction.  If the driver doesn't know the
platform it can't get any value from the call.  Worse, people are going
to start /expecting/ some behaviour from the call and wonder why their
driver fails subtly on slightly different systems.

Who's supposed to set the alternate functions?  As I see it, only the
board code.  The driver shouldn't ever have to do this itself, all the
pin mux'ing should be done well before the driver needs to access its
pins.  Even if the driver does need to set up it's own pins then it
needs to know /which/ alternate function it is which once again requires
platform knowledge.  If it requires platform knowledge it should be done
in platform code, not driver code, or can at least be done with
non-abstracted calls.

These both seem like needless feature creep and misdirection to me.

	--Ben.