* [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver @ 2008-05-12 10:17 Bryan Wu 2008-05-12 11:27 ` pHilipp Zabel 2008-05-12 16:55 ` David Brownell 0 siblings, 2 replies; 10+ messages in thread From: Bryan Wu @ 2008-05-12 10:17 UTC (permalink / raw) To: dbrownell, dmitry.torokhov, philipp.zabel Cc: linux-kernel, Michael Hennerich, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 10:17 [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver Bryan Wu @ 2008-05-12 11:27 ` pHilipp Zabel 2008-05-12 11:42 ` Mike Frysinger 2008-05-12 16:55 ` David Brownell 1 sibling, 1 reply; 10+ messages in thread From: pHilipp Zabel @ 2008-05-12 11:27 UTC (permalink / raw) To: Bryan Wu; +Cc: dbrownell, dmitry.torokhov, linux-kernel, Michael Hennerich 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 11:27 ` pHilipp Zabel @ 2008-05-12 11:42 ` Mike Frysinger 2008-05-12 12:11 ` Bryan Wu 0 siblings, 1 reply; 10+ messages in thread From: Mike Frysinger @ 2008-05-12 11:42 UTC (permalink / raw) To: pHilipp Zabel Cc: Bryan Wu, dbrownell, dmitry.torokhov, linux-kernel, Michael Hennerich 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 11:42 ` Mike Frysinger @ 2008-05-12 12:11 ` Bryan Wu 2008-05-12 12:47 ` Mike Frysinger 0 siblings, 1 reply; 10+ messages in thread From: Bryan Wu @ 2008-05-12 12:11 UTC (permalink / raw) To: Mike Frysinger Cc: pHilipp Zabel, dbrownell, dmitry.torokhov, linux-kernel, Michael Hennerich 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 12:11 ` Bryan Wu @ 2008-05-12 12:47 ` Mike Frysinger 2008-05-12 14:42 ` Bryan Wu 0 siblings, 1 reply; 10+ messages in thread From: Mike Frysinger @ 2008-05-12 12:47 UTC (permalink / raw) To: Bryan Wu Cc: pHilipp Zabel, dbrownell, dmitry.torokhov, linux-kernel, Michael Hennerich 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 12:47 ` Mike Frysinger @ 2008-05-12 14:42 ` Bryan Wu 2008-05-12 16:54 ` David Brownell 0 siblings, 1 reply; 10+ messages in thread From: Bryan Wu @ 2008-05-12 14:42 UTC (permalink / raw) To: Mike Frysinger Cc: pHilipp Zabel, dbrownell, dmitry.torokhov, linux-kernel, Michael Hennerich 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 14:42 ` Bryan Wu @ 2008-05-12 16:54 ` David Brownell 2008-05-12 16:59 ` Mike Frysinger 0 siblings, 1 reply; 10+ messages in thread From: David Brownell @ 2008-05-12 16:54 UTC (permalink / raw) To: Bryan Wu Cc: Mike Frysinger, pHilipp Zabel, dbrownell, dmitry.torokhov, linux-kernel, Michael Hennerich 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 16:54 ` David Brownell @ 2008-05-12 16:59 ` Mike Frysinger 2008-05-13 7:53 ` Hennerich, Michael 0 siblings, 1 reply; 10+ messages in thread From: Mike Frysinger @ 2008-05-12 16:59 UTC (permalink / raw) To: David Brownell Cc: Bryan Wu, pHilipp Zabel, dbrownell, dmitry.torokhov, linux-kernel, Michael Hennerich 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 16:59 ` Mike Frysinger @ 2008-05-13 7:53 ` Hennerich, Michael 0 siblings, 0 replies; 10+ messages in thread From: Hennerich, Michael @ 2008-05-13 7:53 UTC (permalink / raw) To: Mike Frysinger, David Brownell Cc: Bryan Wu, pHilipp Zabel, dbrownell, dmitry.torokhov, linux-kernel, Michael Hennerich >-----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver 2008-05-12 10:17 [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver Bryan Wu 2008-05-12 11:27 ` pHilipp Zabel @ 2008-05-12 16:55 ` David Brownell 1 sibling, 0 replies; 10+ messages in thread From: David Brownell @ 2008-05-12 16:55 UTC (permalink / raw) To: Bryan Wu Cc: dbrownell, dmitry.torokhov, philipp.zabel, linux-kernel, Michael Hennerich 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... ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2008-05-13 7:56 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-05-12 10:17 [PATCH 1/1] [INPUT/KEYPAD] gpio keypad: Replace current blackfin specific pfbutton driver with kernel generic gpio key driver Bryan Wu 2008-05-12 11:27 ` pHilipp Zabel 2008-05-12 11:42 ` Mike Frysinger 2008-05-12 12:11 ` Bryan Wu 2008-05-12 12:47 ` Mike Frysinger 2008-05-12 14:42 ` Bryan Wu 2008-05-12 16:54 ` David Brownell 2008-05-12 16:59 ` Mike Frysinger 2008-05-13 7:53 ` Hennerich, Michael 2008-05-12 16:55 ` David Brownell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox