[PATCHv2,4/3] Input: omap4-keypad - scan the keys in two phases to detect lost key-up
diff mbox series

Message ID 20200228180243.GM37466@atomide.com
State New
Headers show
Series
  • Lost key-up interrupt handling for omap4-keypad
Related show

Commit Message

Tony Lindgren Feb. 28, 2020, 6:02 p.m. UTC
In addition to handling errata i689 for idle with state, we must also
check for lost key up interrupts on fast key presses.

For example rapidly pressing shift-shift-j can sometimes produce a J
instead of j. Let's fix the issue by scanning the keyboard in two
phases. First we scan for any key up events that we may have missed,
and then we scan for key down events.

Cc: Arthur Demchenkov <spinal.by@gmail.com>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/omap4-keypad.c | 48 ++++++++++++++++++---------
 1 file changed, 32 insertions(+), 16 deletions(-)

Comments

'Dmitry Torokhov' March 6, 2020, 7:04 p.m. UTC | #1
On Fri, Feb 28, 2020 at 10:02:43AM -0800, Tony Lindgren wrote:
> In addition to handling errata i689 for idle with state, we must also
> check for lost key up interrupts on fast key presses.
> 
> For example rapidly pressing shift-shift-j can sometimes produce a J
> instead of j. Let's fix the issue by scanning the keyboard in two
> phases. First we scan for any key up events that we may have missed,
> and then we scan for key down events.
> 
> Cc: Arthur Demchenkov <spinal.by@gmail.com>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c | 48 ++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -109,6 +109,34 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
>  		     keypad_data->base + keypad_data->irqreg_offset + offset);
>  }
>  
> +static void omap4_keypad_scan_state(struct omap4_keypad *keypad_data,
> +				    unsigned char *key_state,
> +				    bool down)
> +{
> +	struct input_dev *input_dev = keypad_data->input;
> +	unsigned int col, row, code, changed;
> +	bool key_down;
> +
> +	for (row = 0; row < keypad_data->rows; row++) {
> +		changed = key_state[row] ^ keypad_data->key_state[row];
> +		if (!changed)
> +			continue;
> +
> +		for (col = 0; col < keypad_data->cols; col++) {
> +			if (changed & (1 << col)) {


Looking at all too this, key state is 64 bit value, so if we make it a
bitmap and use "for_each_set_bit()" we can simplify this scanning code.

Thanks.
Tony Lindgren March 6, 2020, 7:09 p.m. UTC | #2
* Dmitry Torokhov <dmitry.torokhov@gmail.com> [200306 19:05]:
> On Fri, Feb 28, 2020 at 10:02:43AM -0800, Tony Lindgren wrote:
> > In addition to handling errata i689 for idle with state, we must also
> > check for lost key up interrupts on fast key presses.
> > 
> > For example rapidly pressing shift-shift-j can sometimes produce a J
> > instead of j. Let's fix the issue by scanning the keyboard in two
> > phases. First we scan for any key up events that we may have missed,
> > and then we scan for key down events.
> > 
> > Cc: Arthur Demchenkov <spinal.by@gmail.com>
> > Cc: Merlijn Wajer <merlijn@wizzup.org>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Sebastian Reichel <sre@kernel.org>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  drivers/input/keyboard/omap4-keypad.c | 48 ++++++++++++++++++---------
> >  1 file changed, 32 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> > --- a/drivers/input/keyboard/omap4-keypad.c
> > +++ b/drivers/input/keyboard/omap4-keypad.c
> > @@ -109,6 +109,34 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
> >  		     keypad_data->base + keypad_data->irqreg_offset + offset);
> >  }
> >  
> > +static void omap4_keypad_scan_state(struct omap4_keypad *keypad_data,
> > +				    unsigned char *key_state,
> > +				    bool down)
> > +{
> > +	struct input_dev *input_dev = keypad_data->input;
> > +	unsigned int col, row, code, changed;
> > +	bool key_down;
> > +
> > +	for (row = 0; row < keypad_data->rows; row++) {
> > +		changed = key_state[row] ^ keypad_data->key_state[row];
> > +		if (!changed)
> > +			continue;
> > +
> > +		for (col = 0; col < keypad_data->cols; col++) {
> > +			if (changed & (1 << col)) {
> 
> 
> Looking at all too this, key state is 64 bit value, so if we make it a
> bitmap and use "for_each_set_bit()" we can simplify this scanning code.

OK good idea, I'll take a look.

Regards,

Tony

Patch
diff mbox series

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -109,6 +109,34 @@  static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
 		     keypad_data->base + keypad_data->irqreg_offset + offset);
 }
 
+static void omap4_keypad_scan_state(struct omap4_keypad *keypad_data,
+				    unsigned char *key_state,
+				    bool down)
+{
+	struct input_dev *input_dev = keypad_data->input;
+	unsigned int col, row, code, changed;
+	bool key_down;
+
+	for (row = 0; row < keypad_data->rows; row++) {
+		changed = key_state[row] ^ keypad_data->key_state[row];
+		if (!changed)
+			continue;
+
+		for (col = 0; col < keypad_data->cols; col++) {
+			if (changed & (1 << col)) {
+				code = MATRIX_SCAN_CODE(row, col,
+						keypad_data->row_shift);
+				key_down = key_state[row] & (1 << col);
+				if (key_down != down)
+					continue;
+				input_event(input_dev, EV_MSC, MSC_SCAN, code);
+				input_report_key(input_dev,
+						 keypad_data->keymap[code],
+						 key_down);
+			}
+		}
+	}
+}
 
 /* Interrupt handlers */
 static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
@@ -125,7 +153,6 @@  static bool omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, bool clear)
 {
 	struct input_dev *input_dev = keypad_data->input;
 	unsigned char key_state[ARRAY_SIZE(keypad_data->key_state)];
-	unsigned int col, row, code, changed;
 	u32 *rows_lo = (u32 *)key_state;
 	u32 *rows_hi = rows_lo + 1;
 
@@ -138,22 +165,11 @@  static bool omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, bool clear)
 		*rows_hi = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
 	}
 
-	for (row = 0; row < keypad_data->rows; row++) {
-		changed = key_state[row] ^ keypad_data->key_state[row];
-		if (!changed)
-			continue;
+	/* Scan for key up evetns for lost key-up interrupts */
+	omap4_keypad_scan_state(keypad_data, key_state, false);
 
-		for (col = 0; col < keypad_data->cols; col++) {
-			if (changed & (1 << col)) {
-				code = MATRIX_SCAN_CODE(row, col,
-						keypad_data->row_shift);
-				input_event(input_dev, EV_MSC, MSC_SCAN, code);
-				input_report_key(input_dev,
-						 keypad_data->keymap[code],
-						 key_state[row] & (1 << col));
-			}
-		}
-	}
+	/* Scan for key down events */
+	omap4_keypad_scan_state(keypad_data, key_state, true);
 
 	input_sync(input_dev);