gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

PATCH In review Bryan Wu 5 participants 10 messages May 12, 2008 – May 13, 2008 Input drivers

lore

Share & tools

lore

Bryan Wu

author

2008-05-12 10:17 UTC #0 lore raw

PATCH 1/1 gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

From: Michael Hennerich <[email protected]>

It's an actual deficiency in the hardware that we can't address, so it needs to be worked around in software.

Signed-off-by Michael Hennerich <[email protected]>

Signed-off-by Bryan Wu <[email protected]>

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

drivers/input/keyboard/gpio_keys.c +14 -1

index bbd00c3..d856eb9 100644
@@ -26,6 +26,18 @@
 
 #include <asm/gpio.h>
 
+#if defined(CONFIG_BLACKFIN) && !defined(BF548_FAMILY)
+
+/*
+ * On some Blackfin CPUs reading edge triggered
+ * GPIOs doesn't return the current value
+ */
+
+#define GPIOKEYS_EDGE_SENSE(x) set_gpio_edge(gpio, x)
+#else
+#define GPIOKEYS_EDGE_SENSE(x) do {} while (0)
+#endif
+
 static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 {
 	int i;

@@ -39,8 +51,9 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
 
 		if (irq == gpio_to_irq(gpio)) {
 			unsigned int type = button->type ?: EV_KEY;
+			GPIOKEYS_EDGE_SENSE(0);
 			int state = (gpio_get_value(gpio) ? 1 : 0) ^ button->active_low;
-
+			GPIOKEYS_EDGE_SENSE(1);
 			input_event(input, type, button->code, !!state);
 			input_sync(input);
 			return IRQ_HANDLED;

Signature · 1 line

1.5.5

pHilipp Zabel

2008-05-12 11:27 UTC #1 lore raw

Hi,

Quoted 17 lines · Bryan Wu

From: Michael Hennerich <[email protected]>
It's an actual deficiency in the hardware that we can't address, so it needs to be worked around in software.
Signed-off-by Michael Hennerich <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
  drivers/input/keyboard/gpio_keys.c |   15 ++++++++++++++-
  1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index bbd00c3..d856eb9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -26,6 +26,18 @@

Quoted 8 lines

  #include <asm/gpio.h>
+#if defined(CONFIG_BLACKFIN) && !defined(BF548_FAMILY) +
+/*
+ * On some Blackfin CPUs reading edge triggered
+ * GPIOs doesn't return the current value
+ */

If this is a generic problem, shouldn't this be addressed inside gpio_get_value?

Quoted 20 lines

+
+#define GPIOKEYS_EDGE_SENSE(x) set_gpio_edge(gpio, x) +#else
+#define GPIOKEYS_EDGE_SENSE(x) do {} while (0) +#endif
+
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
  {
        int i;
@@ -39,8 +51,9 @@ static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
                if (irq == gpio_to_irq(gpio)) {
                        unsigned int type = button->type ?: EV_KEY;
 +                       GPIOKEYS_EDGE_SENSE(0);
                        int state = (gpio_get_value(gpio) ? 1 : 0) ^ button->active_low;
 -
 +                       GPIOKEYS_EDGE_SENSE(1);
                        input_event(input, type, button->code, !!state);
                        input_sync(input);
                        return IRQ_HANDLED;

Quoted 4 lines

--
1.5.5

regards
Philipp

Mike Frysinger

2008-05-12 11:42 UTC #2 lore raw

Quoted 6 lines · pHilipp Zabel

Quoted 5 lines · Bryan Wu
From: Michael Hennerich <[email protected]>
It's an actual deficiency in the hardware that we can't address, so it needs to be worked around in software.

Quoted 7 lines

Quoted 7 lines
Signed-off-by Michael Hennerich <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
  drivers/input/keyboard/gpio_keys.c |   15 ++++++++++++++-
  1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c

b/drivers/input/keyboard/gpio_keys.c

Quoted 7 lines

Quoted 7 lines
index bbd00c3..d856eb9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -26,6 +26,18 @@
  #include <asm/gpio.h>

Quoted 8 lines

Quoted 6 lines
+#if defined(CONFIG_BLACKFIN) && !defined(BF548_FAMILY) +
+/*
+ * On some Blackfin CPUs reading edge triggered
+ * GPIOs doesn't return the current value
+ */
If this is a generic problem, shouldn't this be addressed inside gpio_get_value?

it's an issue only when the GPIO is an interrupt source and the trigger condition is set to both rising and falling. but i guess your point is that in gpio_get_value(), we can check to see if these conditions are met and if so, temporarily fiddle things there ?

Michael: that sounds reasonable, what do you think ? -mike

Bryan Wu

author

2008-05-12 12:12 UTC #3 lore raw

Quoted 31 lines · Mike Frysinger

Quoted 6 lines · pHilipp Zabel
Quoted 5 lines · Bryan Wu
From: Michael Hennerich <[email protected]>
It's an actual deficiency in the hardware that we can't address, so it needs to be worked around in software.
Quoted 7 lines
Quoted 7 lines
Signed-off-by Michael Hennerich <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
  drivers/input/keyboard/gpio_keys.c |   15 ++++++++++++++-
  1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c
b/drivers/input/keyboard/gpio_keys.c
Quoted 7 lines
Quoted 7 lines
index bbd00c3..d856eb9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -26,6 +26,18 @@
  #include <asm/gpio.h>
Quoted 8 lines
Quoted 6 lines
+#if defined(CONFIG_BLACKFIN) && !defined(BF548_FAMILY) +
+/*
+ * On some Blackfin CPUs reading edge triggered
+ * GPIOs doesn't return the current value
+ */
If this is a generic problem, shouldn't this be addressed inside gpio_get_value?

Quoted 8 lines

it's an issue only when the GPIO is an interrupt source and the trigger condition is set to both rising and falling. but i guess your point is that in gpio_get_value(), we can check to see if these conditions are met and if so, temporarily fiddle things there ?
Michael: that sounds reasonable, what do you think ? -mike

IMO, maybe it is feasible for fix this in generic GPIO layer, or will break other things?

-Bryan

Mike Frysinger

2008-05-12 12:47 UTC #4 lore raw

Quoted 2 lines · Bryan Wu

Quoted 1 line · Mike Frysinger
On Mon, May 12, 2008 at 7:27 AM, pHilipp Zabel

Quoted 21 lines · <[email protected]> wrote:

Quoted 21 lines
Quoted 6 lines · Bryan Wu
Quoted 5 lines
From: Michael Hennerich <[email protected]>
It's an actual deficiency in the hardware that we can't address, so it needs to be worked around in software.
Quoted 7 lines
Quoted 7 lines
Signed-off-by Michael Hennerich <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
  drivers/input/keyboard/gpio_keys.c |   15 ++++++++++++++-
  1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c
b/drivers/input/keyboard/gpio_keys.c
Quoted 7 lines
Quoted 7 lines
index bbd00c3..d856eb9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -26,6 +26,18 @@
  #include <asm/gpio.h>

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
+#if defined(CONFIG_BLACKFIN) && !defined(BF548_FAMILY) +
+/*
+ * On some Blackfin CPUs reading edge triggered
+ * GPIOs doesn't return the current value
+ */
If this is a generic problem, shouldn't this be addressed inside

gpio_get_value?

Quoted 10 lines

Quoted 7 lines
it's an issue only when the GPIO is an interrupt source and the trigger condition is set to both rising and falling. but i guess your point is that in gpio_get_value(), we can check to see if these conditions are met and if so, temporarily fiddle things there ?
Michael: that sounds reasonable, what do you think ?
IMO, maybe it is feasible for fix this in generic GPIO layer, or will break other things?

where are you referring to ? i think what's being proposed is to move these checks to arch/blackfin/kernel/bfin_gpio.c ... any call to gpio_get_value() under these circumstances will need the edge sense toggle ...
-mike

Bryan Wu

author

2008-05-12 14:42 UTC #5 lore raw

Quoted 44 lines · Mike Frysinger

Quoted 2 lines · Bryan Wu
Quoted 1 line · Mike Frysinger
On Mon, May 12, 2008 at 7:27 AM, pHilipp Zabel
Quoted 21 lines · <[email protected]> wrote:
Quoted 21 lines
Quoted 6 lines · Bryan Wu
Quoted 5 lines
From: Michael Hennerich <[email protected]>
It's an actual deficiency in the hardware that we can't address, so it needs to be worked around in software.
Quoted 7 lines
Quoted 7 lines
Signed-off-by Michael Hennerich <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
  drivers/input/keyboard/gpio_keys.c |   15 ++++++++++++++-
  1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c
b/drivers/input/keyboard/gpio_keys.c
Quoted 7 lines
Quoted 7 lines
index bbd00c3..d856eb9 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -26,6 +26,18 @@
  #include <asm/gpio.h>
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
+#if defined(CONFIG_BLACKFIN) && !defined(BF548_FAMILY) +
+/*
+ * On some Blackfin CPUs reading edge triggered
+ * GPIOs doesn't return the current value
+ */
If this is a generic problem, shouldn't this be addressed inside
gpio_get_value?
Quoted 8 lines
Quoted 7 lines
it's an issue only when the GPIO is an interrupt source and the trigger condition is set to both rising and falling. but i guess your point is that in gpio_get_value(), we can check to see if these conditions are met and if so, temporarily fiddle things there ?
Michael: that sounds reasonable, what do you think ?

Quoted 9 lines

Quoted 2 lines
IMO, maybe it is feasible for fix this in generic GPIO layer, or will break other things?
where are you referring to ? i think what's being proposed is to move these checks to arch/blackfin/kernel/bfin_gpio.c ... any call to gpio_get_value() under these circumstances will need the edge sense toggle ...
-mike

Right. I mean the generic GPIO layer in Blackfin port, not the common code.

And on the other hand, maybe there are also other hardware which has similar issue as Blackfin. So the common GPIO layer can take care of this, how do you think, David?

-Bryan

David Brownell

2008-05-12 16:54 UTC #6 lore raw

Quoted 5 lines · Bryan Wu

Right. I mean the generic GPIO layer in Blackfin port, not the common code.
And on the other hand, maybe there are also other hardware which has similar issue as Blackfin. So the common GPIO layer can take care of this, how do you think, David?

No; it's a Blackfin-specific design flaw, one that I've not seen in any other chip's GPIO support. So it should stay in the Blackfin support code. (Possibly even specialized for the specific chips which have that flaw/erratum.)

- Dave

Mike Frysinger

2008-05-12 17:00 UTC #7 lore raw

Quoted 11 lines · David Brownell

Quoted 5 lines · Bryan Wu
Right. I mean the generic GPIO layer in Blackfin port, not the common code.
And on the other hand, maybe there are also other hardware which has similar issue as Blackfin. So the common GPIO layer can take care of this, how do you think, David?
No; it's a Blackfin-specific design flaw, one that I've not seen in any other chip's GPIO support. So it should stay in the Blackfin support code. (Possibly even specialized for the specific chips which have that flaw/erratum.)

afaik, it was a design flaw, not an erratum ... newer parts have the situation rectified. but Michael would be able to comment more authoritatively on the topic. we already have the logic to only make the swap for parts that need it.
-mike

Hennerich, Michael

2008-05-13 07:56 UTC #8 lore raw

Quoted 8 lines

-----Original Message-----
From: Mike Frysinger [mailto:[email protected]]
Sent: Montag, 12. Mai 2008 19:00
To: David Brownell
Cc: Bryan Wu; pHilipp Zabel; [email protected]; [email protected]; [email protected]; Michael

Hennerich

Quoted 6 lines

Subject: Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver
Quoted 2 lines · David Brownell
Quoted 1 line · Bryan Wu
Right. I mean the generic GPIO layer in Blackfin port, not the

common

Quoted 3 lines

code.
Quoted 2 lines
Quoted 2 lines
And on the other hand, maybe there are also other hardware which

has

Quoted 2 lines

similar
Quoted 1 line
Quoted 1 line
issue as Blackfin. So the common GPIO layer can take care of this,

how

Quoted 7 lines

Quoted 6 lines
Quoted 1 line
do you think, David?
No; it's a Blackfin-specific design flaw, one that I've not seen in any other chip's GPIO support. So it should stay in the Blackfin support code. (Possibly even specialized for the specific chips which have that flaw/erratum.)

Quoted 5 lines

afaik, it was a design flaw, not an erratum ... newer parts have the situation rectified. but Michael would be able to comment more authoritatively on the topic. we already have the logic to only make the swap for parts that need it.
-mike

I agree with all. This hack patch shouldn't be discussed here. I must have overseen this while we discussed what to send upstream.

Please disregard.

-Michael

David Brownell

2008-05-12 16:56 UTC #9 lore raw

In reply to Bryan Wu

Quoted 5 lines · Bryan Wu

                        unsigned int type = button->type ?: EV_KEY;
+                       GPIOKEYS_EDGE_SENSE(0);
                        int state = (gpio_get_value(gpio) ? 1 : 0) ^ button->active_low; -
+                       GPIOKEYS_EDGE_SENSE(1);

By the way ... don't intersperse declarations like "int state" with code like what those EDGE_SENSE macros wraps...