[PATCH] [RFC] gpio-keys: let platform code specify the trigger type

[PATCH] [RFC] gpio-keys: let platform code specify the trigger type

[Uwe Kleine-König]

The intend for this change is that not all platform's irqs support
triggering on both edges.  Examples are ns9xxx[1] and txx9[2], and I
expect that there are more.

Provided that the platform data is initialized with zeros there is no
change in behavior if the new struct member 'trigger' isn't set in
platform code.

open points:
 - if only one trigger direction is used it should match active_low such
   that the button press generates the irq.
 - poll for button release instead of generate the release event
   directly after the press?
 - is it correct to input_sync() between press and release event?
 - sanitize button->trigger &= IRQF_TRIGGER_EDGE in gpio_keys_probe
   before passing it to request_irq?
 - a comment describing the trigger member of struct gpio_keys_button

I'd like to have polling support in this driver.  This could use
button->trigger == 0, so it might be sensible to add a
WARN_ON(!button->trigger) for now and wait some time before implementing
it.

[There is no MAINTAINER entry for gpio-keys, so I Cc: the last
contributors.] 

[1] The code to show that isn't in vanilla yet.  Basic machine support
    is in arch/arm/mach-ns9xxx.
[2] see txx9_irq_set_type in arch/mips/kernel/irq_txx9.c

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Dmitry Torokhov <dtor@mail.ru>
Cc: Herbert Valerio Riedel <hvr@gnu.org>

---
 drivers/input/keyboard/gpio_keys.c |   15 +++++++++++++--
 include/linux/gpio_keys.h          |    1 +
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index bbd00c3..43a4c0c 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -26,6 +26,10 @@
 
 #include <asm/gpio.h>
 
+#ifndef IRQF_TRIGGER_EDGE
+#define IRQF_TRIGGER_EDGE (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)
+#endif
+
 static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 {
 	int i;
@@ -43,6 +47,11 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 
 			input_event(input, type, button->code, !!state);
 			input_sync(input);
+
+			if (~button->trigger & IRQF_TRIGGER_EDGE) {
+				input_event(input, type, button->code, !state);
+				input_sync(input);
+			}
 			return IRQ_HANDLED;
 		}
 	}
@@ -105,9 +114,11 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 			goto fail;
 		}
 
+		if (!button->trigger)
+			button->trigger = IRQF_TRIGGER_EDGE;
+
 		error = request_irq(irq, gpio_keys_isr,
-				    IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_RISING |
-					IRQF_TRIGGER_FALLING,
+				    IRQF_SAMPLE_RANDOM | button->trigger,
 				    button->desc ? button->desc : "gpio_keys",
 				    pdev);
 		if (error) {
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index c6d3a9d..afe9d5d 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -9,6 +9,7 @@ struct gpio_keys_button {
 	char *desc;
 	int type;		/* input event type (EV_KEY, EV_SW) */
 	int wakeup;		/* configure the button as a wake-up source */
+	unsigned long trigger;
 };
 
 struct gpio_keys_platform_data {
-- 
1.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESENT] [RFC] gpio-keys: let platform code specify the trigger type

[Uwe Kleine-König]

The intend for this change is that not all platform's irqs support
triggering on both edges.  Examples are ns9xxx[1] and txx9[2], and I
expect that there are more.

Provided that the platform data is initialized with zeros there is no
change in behavior if the new struct member 'trigger' isn't set in
platform code.

open points:
 - if only one trigger direction is used it should match active_low such
   that the button press generates the irq.
 - poll for button release instead of generate the release event
   directly after the press?
 - is it correct to input_sync() between press and release event?
 - sanitize button->trigger &= IRQF_TRIGGER_EDGE in gpio_keys_probe
   before passing it to request_irq?
 - a comment describing the trigger member of struct gpio_keys_button

I'd like to have polling support in this driver.  This could use
button->trigger == 0, so it might be sensible to add a
WARN_ON(!button->trigger) for now and wait some time before implementing
it.

[There is no MAINTAINER entry for gpio-keys, so I Cc: the last
contributors.]

[1] The code to show that isn't in vanilla yet.  Basic machine support
    is in arch/arm/mach-ns9xxx.
[2] see txx9_irq_set_type in arch/mips/kernel/irq_txx9.c

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Dmitry Torokhov <dtor@mail.ru>
Cc: Herbert Valerio Riedel <hvr@gnu.org>

---
Hello,

I already sent this mail a week ago, so far without any feedback.  So I just
start a new try.

Best regards
Uwe

 drivers/input/keyboard/gpio_keys.c |   15 +++++++++++++--
 include/linux/gpio_keys.h          |    1 +
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index bbd00c3..43a4c0c 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -26,6 +26,10 @@
 
 #include <asm/gpio.h>
 
+#ifndef IRQF_TRIGGER_EDGE
+#define IRQF_TRIGGER_EDGE (IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING)
+#endif
+
 static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 {
 	int i;
@@ -43,6 +47,11 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 
 			input_event(input, type, button->code, !!state);
 			input_sync(input);
+
+			if (~button->trigger & IRQF_TRIGGER_EDGE) {
+				input_event(input, type, button->code, !state);
+				input_sync(input);
+			}
 			return IRQ_HANDLED;
 		}
 	}
@@ -105,9 +114,11 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)
 			goto fail;
 		}
 
+		if (!button->trigger)
+			button->trigger = IRQF_TRIGGER_EDGE;
+
 		error = request_irq(irq, gpio_keys_isr,
-				    IRQF_SAMPLE_RANDOM | IRQF_TRIGGER_RISING |
-					IRQF_TRIGGER_FALLING,
+				    IRQF_SAMPLE_RANDOM | button->trigger,
 				    button->desc ? button->desc : "gpio_keys",
 				    pdev);
 		if (error) {
diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
index c6d3a9d..afe9d5d 100644
--- a/include/linux/gpio_keys.h
+++ b/include/linux/gpio_keys.h
@@ -9,6 +9,7 @@ struct gpio_keys_button {
 	char *desc;
 	int type;		/* input event type (EV_KEY, EV_SW) */
 	int wakeup;		/* configure the button as a wake-up source */
+	unsigned long trigger;
 };
 
 struct gpio_keys_platform_data {
-- 
1.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESENT] [RFC] gpio-keys: let platform code specify the trigger type

[Dmitry Torokhov]

Hi Uwe,

On Thu, Jul 10, 2008 at 10:58:44AM +0200, Uwe Kleine-König wrote:
> The intend for this change is that not all platform's irqs support
> triggering on both edges.  Examples are ns9xxx[1] and txx9[2], and I
> expect that there are more.
> 
> Provided that the platform data is initialized with zeros there is no
> change in behavior if the new struct member 'trigger' isn't set in
> platform code.
> 

OK, makes sense. Unfortunately the patch conflicts with debounce support
that is in 'next' branch of my tree so I can't apply it as is. Actually,
with debounce support is may not even be needed in the present form.

> open points:
>  - if only one trigger direction is used it should match active_low such
>    that the button press generates the irq.
>  - poll for button release instead of generate the release event
>    directly after the press?

Yes, I think that would be the best option.

>  - is it correct to input_sync() between press and release event?
Yes, button press and release are 2 distinct states of the device and so
it is prudent to have input_sync in between.

>  - sanitize button->trigger &= IRQF_TRIGGER_EDGE in gpio_keys_probe
>    before passing it to request_irq?

Would be nice, together with WARN() in case it needed stanitizing.

>  - a comment describing the trigger member of struct gpio_keys_button
> 
> I'd like to have polling support in this driver.  This could use
> button->trigger == 0, so it might be sensible to add a
> WARN_ON(!button->trigger) for now and wait some time before implementing
> it.
>

I am not sure if addin a pure polling mode to this driver is a best
idea. Maybe writing a separate one based on input-polldev will allow
keep them both simpler than combined one.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESENT] [RFC] gpio-keys: let platform code specify the trigger type

[Uwe Kleine-König]

Hello Dmitry,

Dmitry Torokhov wrote:
> On Thu, Jul 10, 2008 at 10:58:44AM +0200, Uwe Kleine-König wrote:
> > The intend for this change is that not all platform's irqs support
> > triggering on both edges.  Examples are ns9xxx[1] and txx9[2], and I
> > expect that there are more.
> >
> > Provided that the platform data is initialized with zeros there is no
> > change in behavior if the new struct member 'trigger' isn't set in
> > platform code.
> >
> 
> OK, makes sense. Unfortunately the patch conflicts with debounce support
> that is in 'next' branch of my tree so I can't apply it as is. Actually,
> with debounce support is may not even be needed in the present form.
So I will resend an updated version when the details are resolved.

> > open points:
> >  - if only one trigger direction is used it should match active_low such
> >    that the button press generates the irq.
> >  - poll for button release instead of generate the release event
> >    directly after the press?
> 
> Yes, I think that would be the best option.
OK.

> >  - is it correct to input_sync() between press and release event?
> Yes, button press and release are 2 distinct states of the device and so
> it is prudent to have input_sync in between.
OK.
 
> >  - sanitize button->trigger &= IRQF_TRIGGER_EDGE in gpio_keys_probe
> >    before passing it to request_irq?
> 
> Would be nice, together with WARN() in case it needed stanitizing.
OK.
 
> >  - a comment describing the trigger member of struct gpio_keys_button
> >
> > I'd like to have polling support in this driver.  This could use
> > button->trigger == 0, so it might be sensible to add a
> > WARN_ON(!button->trigger) for now and wait some time before implementing
> > it.
> >
> 
> I am not sure if addin a pure polling mode to this driver is a best
> idea. Maybe writing a separate one based on input-polldev will allow
> keep them both simpler than combined one.
So you suggest to implement polling in gpio-keys only for button release
(if triggering for both doesn't work) and add a new driver that does
polling only?

I havn't looked deeply, but maybe it makes sense to export some
functions from drivers/input/input-polldev.c and use them as applicable
in the gpio-keys driver?

Thanks and best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html