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

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

[Bryan Wu]

From: Michael Hennerich <michael.hennerich@analog.com>

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 <michael.hennerich@analog.com>
Signed-off-by: Bryan Wu <cooloney@kernel.org>
---
 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 @@
 
 #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;
-- 
1.5.5

Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[pHilipp Zabel]

Hi,

On Mon, May 12, 2008 at 12:17 PM, Bryan Wu <cooloney@kernel.org> wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
>
>  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 <michael.hennerich@analog.com>
>  Signed-off-by: Bryan Wu <cooloney@kernel.org>
>  ---
>   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 @@
>
>   #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?

>  +
>  +#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;
>  --
>  1.5.5
>
>

regards
Philipp

Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[Mike Frysinger]

On Mon, May 12, 2008 at 7:27 AM, pHilipp Zabel <philipp.zabel@gmail.com> wrote:
> On Mon, May 12, 2008 at 12:17 PM, Bryan Wu <cooloney@kernel.org> wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>>  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 <michael.hennerich@analog.com>
>>  Signed-off-by: Bryan Wu <cooloney@kernel.org>
>>  ---
>>   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 @@
>>
>>   #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?

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

Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[Bryan Wu]

On Mon, May 12, 2008 at 7:42 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
> On Mon, May 12, 2008 at 7:27 AM, pHilipp Zabel <philipp.zabel@gmail.com> wrote:
>  > On Mon, May 12, 2008 at 12:17 PM, Bryan Wu <cooloney@kernel.org> wrote:
>  >> From: Michael Hennerich <michael.hennerich@analog.com>
>  >>
>  >>  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 <michael.hennerich@analog.com>
>  >>  Signed-off-by: Bryan Wu <cooloney@kernel.org>
>  >>  ---
>  >>   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 @@
>  >>
>  >>   #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?
>
>  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

Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[Mike Frysinger]

On Mon, May 12, 2008 at 8:11 AM, Bryan Wu <cooloney@kernel.org> wrote:
> On Mon, May 12, 2008 at 7:42 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
>> On Mon, May 12, 2008 at 7:27 AM, pHilipp Zabel
<philipp.zabel@gmail.com> wrote:
>>  > On Mon, May 12, 2008 at 12:17 PM, Bryan Wu <cooloney@kernel.org> wrote:
>>  >> From: Michael Hennerich <michael.hennerich@analog.com>
>>  >>
>>  >>  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 <michael.hennerich@analog.com>
>>  >>  Signed-off-by: Bryan Wu <cooloney@kernel.org>
>>  >>  ---
>>  >>   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 @@
>>  >>
>>  >>   #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?
>>
>>  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

Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[Bryan Wu]

On Mon, May 12, 2008 at 8:47 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
>
> On Mon, May 12, 2008 at 8:11 AM, Bryan Wu <cooloney@kernel.org> wrote:
>  > On Mon, May 12, 2008 at 7:42 PM, Mike Frysinger <vapier.adi@gmail.com> wrote:
>  >> On Mon, May 12, 2008 at 7:27 AM, pHilipp Zabel
>  <philipp.zabel@gmail.com> wrote:
>  >>  > On Mon, May 12, 2008 at 12:17 PM, Bryan Wu <cooloney@kernel.org> wrote:
>  >>  >> From: Michael Hennerich <michael.hennerich@analog.com>
>  >>  >>
>  >>  >>  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 <michael.hennerich@analog.com>
>  >>  >>  Signed-off-by: Bryan Wu <cooloney@kernel.org>
>  >>  >>  ---
>  >>  >>   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 @@
>  >>  >>
>  >>  >>   #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?
>  >>
>  >>  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
>

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

Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[David Brownell]

On Monday 12 May 2008, Bryan Wu wrote:
> 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

Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[David Brownell]

On Monday 12 May 2008, Bryan Wu wrote:
>                         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...

Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[Mike Frysinger]

On Mon, May 12, 2008 at 12:54 PM, David Brownell wrote:
> On Monday 12 May 2008, Bryan Wu wrote:
>  > 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

RE: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver

[Hennerich, Michael]

>-----Original Message-----
>From: Mike Frysinger [mailto:vapier.adi@gmail.com]
>Sent: Montag, 12. Mai 2008 19:00
>To: David Brownell
>Cc: Bryan Wu; pHilipp Zabel; dbrownell@users.sourceforge.net;
>dmitry.torokhov@gmail.com; linux-kernel@vger.kernel.org; Michael
Hennerich
>Subject: Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current
>blackfin specific pfbutton driver with kernel generic gpio key driver
>
>On Mon, May 12, 2008 at 12:54 PM, David Brownell wrote:
>> On Monday 12 May 2008, Bryan Wu wrote:
>>  > 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

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