[PATCHv5 0/5] Lost key-up interrupt handling for omap4-keypad

[PATCHv5 0/5] Lost key-up interrupt handling for omap4-keypad

[Tony Lindgren]

Hi,

This series updates omap4-keypad to disable unused long interrupts, and
implements the missing parts for the lost key-up interrupt quirk as
described in the silicon errata pdf.

These are based on Linux next with the following pending patch applied:

"Input: omap4-keypad - switch to use managed resources"

Regards,

Tony


Changes since v4:

- Rebase on Dmitry's devm patch

- Fix up kernel test robot warnings and mismerge of pm_ops when
  I rebased from v5.10 to Linux next

- Split up the patches further as suggested by Dmitry

Changes since v3:

- Use PM runtime to check the last pressed key is not stuck

- Simplify probe with devm

Changes since v2:

- Drop bogus level change, that already comes from device tree

- Scan keyboard in two phases and simplify using a bitmask

- Use mod_delayed_work and cancel_delayed_work_sync for the quirk

Tony Lindgren (5):
  Input: omap4-keypad - disable unused long interrupts
  Input: omap4-keypad - scan keys in two phases and simplify with
    bitmask
  Input: omap4-keypad - move rest of key scanning to a separate function
  Input: omap4-keypad - use PM runtime autosuspend
  Input: omap4-keypad - implement errata check for lost key-up events

 drivers/input/keyboard/omap4-keypad.c | 172 ++++++++++++++++++++------
 1 file changed, 132 insertions(+), 40 deletions(-)

-- 
2.30.0

[PATCH 1/5] Input: omap4-keypad - disable unused long interrupts

[Tony Lindgren]

We are not using the long events and they produce extra interrupts.
Let's not enable them at all.

Note that also the v3.0.8 Linux Android kernel has long interrupts
disabled.

Cc: Arthur Demchenkov <spinal.by@gmail.com>
Cc: Carl Philipp Klemm <philipp@uvos.xyz>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: ruleh <ruleh@gmx.de>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/omap4-keypad.c | 5 ++---
 1 file changed, 2 insertions(+), 3 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
@@ -176,10 +176,9 @@ static int omap4_keypad_open(struct input_dev *input)
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQENABLE,
-			OMAP4_DEF_IRQENABLE_EVENTEN |
-				OMAP4_DEF_IRQENABLE_LONGKEY);
+			OMAP4_DEF_IRQENABLE_EVENTEN);
 	kbd_writel(keypad_data, OMAP4_KBD_WAKEUPENABLE,
-			OMAP4_DEF_WUP_EVENT_ENA | OMAP4_DEF_WUP_LONG_KEY_ENA);
+			OMAP4_DEF_WUP_EVENT_ENA);
 
 	enable_irq(keypad_data->irq);
 
-- 
2.30.0

[PATCH 2/5] Input: omap4-keypad - scan keys in two phases and simplify with bitmask

[Tony Lindgren]

Because of errata i689 the keyboard can idle with state where no key
up interrupts are seen until after the next key press.

This means we need to first check for any lost key up events before
scanning for new down events.

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.

Let's also simplify things with for_each_set_bit() as suggested by
Dmitry Torokhov <dmitry.torokhov@gmail.com>.

Cc: Arthur Demchenkov <spinal.by@gmail.com>
Cc: Carl Philipp Klemm <philipp@uvos.xyz>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: ruleh <ruleh@gmx.de>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/omap4-keypad.c | 69 ++++++++++++++++++---------
 1 file changed, 46 insertions(+), 23 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
@@ -78,7 +78,7 @@ struct omap4_keypad {
 	u32 irqreg_offset;
 	unsigned int row_shift;
 	bool no_autorepeat;
-	unsigned char key_state[8];
+	u64 keys;
 	unsigned short *keymap;
 };
 
@@ -107,6 +107,41 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
 		     keypad_data->base + keypad_data->irqreg_offset + offset);
 }
 
+static int omap4_keypad_scan_state(struct omap4_keypad *keypad_data, u64 keys,
+				   bool down)
+{
+	struct input_dev *input_dev = keypad_data->input;
+	unsigned int col, row, code;
+	DECLARE_BITMAP(mask, 64);
+	unsigned long bit;
+	int events = 0;
+	bool key_down;
+	u64 changed;
+
+	changed = keys ^ keypad_data->keys;
+	bitmap_from_u64(mask, changed);
+
+	for_each_set_bit(bit, mask, keypad_data->rows * BITS_PER_BYTE) {
+		row = bit / BITS_PER_BYTE;
+		col = bit % BITS_PER_BYTE;
+		code = MATRIX_SCAN_CODE(row, col, keypad_data->row_shift);
+
+		if (BIT_ULL(bit) & keys)
+			key_down = true;
+		else
+			key_down = false;
+
+		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);
+		events++;
+	}
+
+	return events;
+}
 
 /* Interrupt handlers */
 static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
@@ -123,34 +158,22 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 {
 	struct omap4_keypad *keypad_data = dev_id;
 	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 *new_state = (u32 *) key_state;
+	u32 low, high;
+	u64 keys;
 
-	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
-	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
+	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
+	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
+	keys = low | (u64)high << 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 events for lost key-up interrupts */
+	omap4_keypad_scan_state(keypad_data, keys, 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, keys, true);
 
 	input_sync(input_dev);
 
-	memcpy(keypad_data->key_state, key_state,
-		sizeof(keypad_data->key_state));
+	keypad_data->keys = keys;
 
 	/* clear pending interrupts */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
-- 
2.30.0

[PATCH 3/5] Input: omap4-keypad - move rest of key scanning to a separate function

[Tony Lindgren]

Let's move rest of the key scanning code to omap4_keypad_scan_keys().
We will use omap4_keypad_scan_keys() also for implementing errata
handling to clear the stuck last key in the following patch.

Cc: Arthur Demchenkov <spinal.by@gmail.com>
Cc: Carl Philipp Klemm <philipp@uvos.xyz>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: ruleh <ruleh@gmx.de>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/omap4-keypad.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 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
@@ -71,6 +71,7 @@ struct omap4_keypad {
 
 	void __iomem *base;
 	unsigned int irq;
+	struct mutex lock;		/* for key scan */
 
 	unsigned int rows;
 	unsigned int cols;
@@ -154,16 +155,18 @@ static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
-static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
+static void omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, bool clear)
 {
-	struct omap4_keypad *keypad_data = dev_id;
 	struct input_dev *input_dev = keypad_data->input;
 	u32 low, high;
-	u64 keys;
+	u64 keys = 0;
 
-	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
-	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
-	keys = low | (u64)high << 32;
+	mutex_lock(&keypad_data->lock);
+	if (!clear) {
+		low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
+		high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
+		keys = low | (u64)high << 32;
+	}
 
 	/* Scan for key up events for lost key-up interrupts */
 	omap4_keypad_scan_state(keypad_data, keys, false);
@@ -175,6 +178,15 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 
 	keypad_data->keys = keys;
 
+	mutex_unlock(&keypad_data->lock);
+}
+
+static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
+{
+	struct omap4_keypad *keypad_data = dev_id;
+
+	omap4_keypad_scan_keys(keypad_data, false);
+
 	/* clear pending interrupts */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
@@ -307,6 +319,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	}
 
 	keypad_data->irq = irq;
+	mutex_init(&keypad_data->lock);
 
 	error = omap4_keypad_parse_dt(dev, keypad_data);
 	if (error)
-- 
2.30.0

[PATCH 4/5] Input: omap4-keypad - use PM runtime autosuspend

[Tony Lindgren]

Implement PM runtime autosuspend support to prepare for adding handling to
clear stuck last pressed key in the following patches. The hardware has
support for autoidle and can wake up to keypress events.

Cc: Arthur Demchenkov <spinal.by@gmail.com>
Cc: Carl Philipp Klemm <philipp@uvos.xyz>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: ruleh <ruleh@gmx.de>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/omap4-keypad.c | 49 +++++++++++++++++++++------
 1 file changed, 38 insertions(+), 11 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
@@ -184,6 +184,14 @@ static void omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, bool clear)
 static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 {
 	struct omap4_keypad *keypad_data = dev_id;
+	struct device *dev = keypad_data->input->dev.parent;
+	int error;
+
+	error = pm_runtime_get_sync(dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(dev);
+		return IRQ_NONE;
+	}
 
 	omap4_keypad_scan_keys(keypad_data, false);
 
@@ -191,14 +199,23 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return IRQ_HANDLED;
 }
 
 static int omap4_keypad_open(struct input_dev *input)
 {
 	struct omap4_keypad *keypad_data = input_get_drvdata(input);
+	struct device *dev = input->dev.parent;
+	int error;
 
-	pm_runtime_get_sync(input->dev.parent);
+	error = pm_runtime_get_sync(dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(dev);
+		return error;
+	}
 
 	disable_irq(keypad_data->irq);
 
@@ -217,6 +234,9 @@ static int omap4_keypad_open(struct input_dev *input)
 
 	enable_irq(keypad_data->irq);
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 }
 
@@ -234,14 +254,20 @@ static void omap4_keypad_stop(struct omap4_keypad *keypad_data)
 
 static void omap4_keypad_close(struct input_dev *input)
 {
-	struct omap4_keypad *keypad_data;
+	struct omap4_keypad *keypad_data = input_get_drvdata(input);
+	struct device *dev = input->dev.parent;
+	int error;
+
+	error = pm_runtime_get_sync(dev);
+	if (error < 0)
+		pm_runtime_put_noidle(dev);
 
-	keypad_data = input_get_drvdata(input);
 	disable_irq(keypad_data->irq);
 	omap4_keypad_stop(keypad_data);
 	enable_irq(keypad_data->irq);
 
-	pm_runtime_put_sync(input->dev.parent);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 }
 
 static int omap4_keypad_parse_dt(struct device *dev,
@@ -288,6 +314,7 @@ static int omap4_keypad_check_revision(struct device *dev,
 
 static void omap4_disable_pm(void *d)
 {
+	pm_runtime_dont_use_autosuspend(d);
 	pm_runtime_disable(d);
 }
 
@@ -320,6 +347,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 
 	keypad_data->irq = irq;
 	mutex_init(&keypad_data->lock);
+	platform_set_drvdata(pdev, keypad_data);
 
 	error = omap4_keypad_parse_dt(dev, keypad_data);
 	if (error)
@@ -329,6 +357,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	if (IS_ERR(keypad_data->base))
 		return PTR_ERR(keypad_data->base);
 
+	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
 	error = devm_add_action_or_reset(dev, omap4_disable_pm, dev);
@@ -350,15 +379,12 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 
 	error = omap4_keypad_check_revision(&pdev->dev,
 					    keypad_data);
-	if (!error) {
-		/* Ensure device does not raise interrupts */
-		omap4_keypad_stop(keypad_data);
-	}
-
-	pm_runtime_put_sync(&pdev->dev);
 	if (error)
 		return error;
 
+	/* Ensure device does not raise interrupts */
+	omap4_keypad_stop(keypad_data);
+
 	/* input device allocation */
 	keypad_data->input = input_dev = devm_input_allocate_device(dev);
 	if (!input_dev)
@@ -419,7 +445,8 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	if (error)
 		dev_warn(dev, "failed to set up wakeup irq: %d\n", error);
 
-	platform_set_drvdata(pdev, keypad_data);
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
 
 	return 0;
 }
-- 
2.30.0

[PATCH 5/5] Input: omap4-keypad - implement errata check for lost key-up events

[Tony Lindgren]

We are still missing handling for errata i689 related issues for the
case where we never see a key up interrupt for the last pressed key.

To fix the issue, we must scan the key state again after the keyboard
controller has idled to check if a key up event was missed. This is
described in the omap4 silicon errata documentation for Errata ID i689
"1.32 Keyboard Key Up Event Can Be Missed":

"When a key is released for a time shorter than the debounce time,
 in-between 2 key press (KP1 and KP2), the keyboard state machine will go
 to idle mode and will never detect the key release (after KP1, and also
 after KP2), and thus will never generate a new IRQ indicating the key
 release."

We can use PM runtime autosuspend features to check the keyboard state
after it enters idle.

Cc: Arthur Demchenkov <spinal.by@gmail.com>
Cc: Carl Philipp Klemm <philipp@uvos.xyz>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: ruleh <ruleh@gmx.de>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/omap4-keypad.c | 30 +++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

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
@@ -60,6 +60,8 @@
 	((((dbms) * 1000) / ((1 << ((ptv) + 1)) * (1000000 / 32768))) - 1)
 #define OMAP4_VAL_DEBOUNCINGTIME_16MS					\
 	OMAP4_KEYPAD_DEBOUNCINGTIME_MS(16, OMAP4_KEYPAD_PTV_DIV_128)
+#define OMAP4_KEYPAD_AUTOIDLE_MS	50	/* Approximate measured time */
+#define OMAP4_KEYPAD_IDLE_CHECK_MS	(OMAP4_KEYPAD_AUTOIDLE_MS / 2)
 
 enum {
 	KBD_REVISION_OMAP4 = 0,
@@ -312,6 +314,32 @@ static int omap4_keypad_check_revision(struct device *dev,
 	return 0;
 }
 
+/*
+ * Errata ID i689 "1.32 Keyboard Key Up Event Can Be Missed".
+ * Interrupt may not happen for key-up events. We must clear stuck
+ * key-up events after the keyboard hardware has auto-idled.
+ */
+static int __maybe_unused omap4_keypad_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
+	u32 active;
+
+	active = kbd_readl(keypad_data, OMAP4_KBD_STATEMACHINE);
+	if (active) {
+		pm_runtime_mark_last_busy(dev);
+		return -EBUSY;
+	}
+
+	omap4_keypad_scan_keys(keypad_data, true);
+
+	return 0;
+}
+
+static const struct dev_pm_ops omap4_keypad_pm_ops = {
+	SET_RUNTIME_PM_OPS(omap4_keypad_runtime_suspend, NULL, NULL)
+};
+
 static void omap4_disable_pm(void *d)
 {
 	pm_runtime_dont_use_autosuspend(d);
@@ -358,6 +386,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 		return PTR_ERR(keypad_data->base);
 
 	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, OMAP4_KEYPAD_IDLE_CHECK_MS);
 	pm_runtime_enable(dev);
 
 	error = devm_add_action_or_reset(dev, omap4_disable_pm, dev);
@@ -470,6 +499,7 @@ static struct platform_driver omap4_keypad_driver = {
 	.driver		= {
 		.name	= "omap4-keypad",
 		.of_match_table = omap_keypad_dt_match,
+		.pm = &omap4_keypad_pm_ops,
 	},
 };
 module_platform_driver(omap4_keypad_driver);
-- 
2.30.0

Re: [PATCH 2/5] Input: omap4-keypad - scan keys in two phases and simplify with bitmask

[Dmitry Torokhov]

Hi Tony,

On Sun, Jan 10, 2021 at 09:05:26PM +0200, Tony Lindgren wrote:
> Because of errata i689 the keyboard can idle with state where no key
> up interrupts are seen until after the next key press.
> 
> This means we need to first check for any lost key up events before
> scanning for new down events.
> 
> 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.
> 
> Let's also simplify things with for_each_set_bit() as suggested by
> Dmitry Torokhov <dmitry.torokhov@gmail.com>.
> 
> Cc: Arthur Demchenkov <spinal.by@gmail.com>
> Cc: Carl Philipp Klemm <philipp@uvos.xyz>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: ruleh <ruleh@gmx.de>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c | 69 ++++++++++++++++++---------
>  1 file changed, 46 insertions(+), 23 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
> @@ -78,7 +78,7 @@ struct omap4_keypad {
>  	u32 irqreg_offset;
>  	unsigned int row_shift;
>  	bool no_autorepeat;
> -	unsigned char key_state[8];
> +	u64 keys;
>  	unsigned short *keymap;
>  };
>  
> @@ -107,6 +107,41 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
>  		     keypad_data->base + keypad_data->irqreg_offset + offset);
>  }
>  
> +static int omap4_keypad_scan_state(struct omap4_keypad *keypad_data, u64 keys,
> +				   bool down)
> +{
> +	struct input_dev *input_dev = keypad_data->input;
> +	unsigned int col, row, code;
> +	DECLARE_BITMAP(mask, 64);
> +	unsigned long bit;
> +	int events = 0;
> +	bool key_down;
> +	u64 changed;
> +
> +	changed = keys ^ keypad_data->keys;
> +	bitmap_from_u64(mask, changed);
> +
> +	for_each_set_bit(bit, mask, keypad_data->rows * BITS_PER_BYTE) {
> +		row = bit / BITS_PER_BYTE;
> +		col = bit % BITS_PER_BYTE;
> +		code = MATRIX_SCAN_CODE(row, col, keypad_data->row_shift);
> +
> +		if (BIT_ULL(bit) & keys)
> +			key_down = true;
> +		else
> +			key_down = false;
> +
> +		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);
> +		events++;
> +	}
> +
> +	return events;
> +}
>  
>  /* Interrupt handlers */
>  static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
> @@ -123,34 +158,22 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
>  {
>  	struct omap4_keypad *keypad_data = dev_id;
>  	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 *new_state = (u32 *) key_state;
> +	u32 low, high;
> +	u64 keys;
>  
> -	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
> -	*(new_state + 1) = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
> +	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
> +	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
> +	keys = low | (u64)high << 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 events for lost key-up interrupts */
> +	omap4_keypad_scan_state(keypad_data, keys, 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, keys, true);
>  
>  	input_sync(input_dev);

Technically speaking, userspace is free to accumulate the events until
it receives EV_SYN/SYN_REPORT event and process the events in the event
packet in order it sees fit. So to achieve what you want, I think we
should issue 2 input_sync()s, one for the release block, and another is
for press. I think we can also simplify the code if we pass into the new
scan function exact set of keys that are being released or pressed.

How about the version below?

Thanks!

-- 
Dmitry


Input: omap4-keypad - scan keys in two phases and simplify with bitmask

From: Tony Lindgren <tony@atomide.com>

Because of errata i689 the keyboard can idle with state where no key
up interrupts are seen until after the next key press.

This means we need to first check for any lost key up events before
scanning for new down events.

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.

Let's also simplify things with for_each_set_bit() as suggested by
Dmitry Torokhov <dmitry.torokhov@gmail.com>.

Signed-off-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/omap4-keypad.c |   73 ++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 28 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index ab761aa66b6d..6dcf27af856d 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -78,7 +78,7 @@ struct omap4_keypad {
 	u32 irqreg_offset;
 	unsigned int row_shift;
 	bool no_autorepeat;
-	unsigned char key_state[8];
+	u64 keys;
 	unsigned short *keymap;
 };
 
@@ -107,6 +107,33 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
 		     keypad_data->base + keypad_data->irqreg_offset + offset);
 }
 
+static int omap4_keypad_report_keys(struct omap4_keypad *keypad_data,
+				    u64 keys, bool down)
+{
+	struct input_dev *input_dev = keypad_data->input;
+	unsigned int col, row, code;
+	DECLARE_BITMAP(mask, 64);
+	unsigned long bit;
+	int events = 0;
+
+	bitmap_from_u64(mask, keys);
+
+	for_each_set_bit(bit, mask, keypad_data->rows * BITS_PER_BYTE) {
+		row = bit / BITS_PER_BYTE;
+		col = bit % BITS_PER_BYTE;
+		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], down);
+
+		events++;
+	}
+
+	if (events)
+		input_sync(input_dev);
+
+	return events;
+}
 
 /* Interrupt handlers */
 static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
@@ -122,35 +149,25 @@ static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
 static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 {
 	struct omap4_keypad *keypad_data = dev_id;
-	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 *new_state = (u32 *) key_state;
-
-	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
-	*(new_state + 1) = 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;
-
-		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));
-			}
-		}
-	}
+	u32 low, high;
+	u64 keys, changed;
+
+	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
+	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
+	keys = low | (u64)high << 32;
+
+	changed = keys ^ keypad_data->keys;
+
+	/*
+	 * Report key up events separately and first. This matters in case we
+	 * lost key-up interrupt and just now catching up.
+	 */
+	omap4_keypad_report_keys(keypad_data, changed & ~keys, false);
 
-	input_sync(input_dev);
+	/* Report key down events */
+	omap4_keypad_report_keys(keypad_data, changed & keys, true);
 
-	memcpy(keypad_data->key_state, key_state,
-		sizeof(keypad_data->key_state));
+	keypad_data->keys = keys;
 
 	/* clear pending interrupts */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,

Re: [PATCH 3/5] Input: omap4-keypad - move rest of key scanning to a separate function

[Dmitry Torokhov]

On Sun, Jan 10, 2021 at 09:05:27PM +0200, Tony Lindgren wrote:
> Let's move rest of the key scanning code to omap4_keypad_scan_keys().
> We will use omap4_keypad_scan_keys() also for implementing errata
> handling to clear the stuck last key in the following patch.

And this one will become then...

-- 
Dmitry


Input: omap4-keypad - move rest of key scanning to a separate function

From: Tony Lindgren <tony@atomide.com>

Let's move rest of the key scanning code to omap4_keypad_scan_keys().
We will use omap4_keypad_scan_keys() also for implementing errata
handling to clear the stuck last key in the following patch.

Signed-off-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/keyboard/omap4-keypad.c |   39 ++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
index 6dcf27af856d..c48705dd049b 100644
--- a/drivers/input/keyboard/omap4-keypad.c
+++ b/drivers/input/keyboard/omap4-keypad.c
@@ -71,6 +71,7 @@ struct omap4_keypad {
 
 	void __iomem *base;
 	unsigned int irq;
+	struct mutex lock;		/* for key scan */
 
 	unsigned int rows;
 	unsigned int cols;
@@ -135,6 +136,28 @@ static int omap4_keypad_report_keys(struct omap4_keypad *keypad_data,
 	return events;
 }
 
+static void omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, u64 keys)
+{
+	u64 changed;
+
+	mutex_lock(&keypad_data->lock);
+
+	changed = keys ^ keypad_data->keys;
+
+	/*
+	 * Report key up events separately and first. This matters in case we
+	 * lost key-up interrupt and just now catching up.
+	 */
+	omap4_keypad_report_keys(keypad_data, changed & ~keys, false);
+
+	/* Report key down events */
+	omap4_keypad_report_keys(keypad_data, changed & keys, true);
+
+	keypad_data->keys = keys;
+
+	mutex_unlock(&keypad_data->lock);
+}
+
 /* Interrupt handlers */
 static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
 {
@@ -150,24 +173,13 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 {
 	struct omap4_keypad *keypad_data = dev_id;
 	u32 low, high;
-	u64 keys, changed;
+	u64 keys;
 
 	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
 	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
 	keys = low | (u64)high << 32;
 
-	changed = keys ^ keypad_data->keys;
-
-	/*
-	 * Report key up events separately and first. This matters in case we
-	 * lost key-up interrupt and just now catching up.
-	 */
-	omap4_keypad_report_keys(keypad_data, changed & ~keys, false);
-
-	/* Report key down events */
-	omap4_keypad_report_keys(keypad_data, changed & keys, true);
-
-	keypad_data->keys = keys;
+	omap4_keypad_scan_keys(keypad_data, keys);
 
 	/* clear pending interrupts */
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
@@ -300,6 +312,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	}
 
 	keypad_data->irq = irq;
+	mutex_init(&keypad_data->lock);
 
 	error = omap4_keypad_parse_dt(dev, keypad_data);
 	if (error)

Re: [PATCH 4/5] Input: omap4-keypad - use PM runtime autosuspend

[Dmitry Torokhov]

Hi Tony,

On Sun, Jan 10, 2021 at 09:05:28PM +0200, Tony Lindgren wrote:
> @@ -350,15 +379,12 @@ static int omap4_keypad_probe(struct platform_device *pdev)
>  
>  	error = omap4_keypad_check_revision(&pdev->dev,
>  					    keypad_data);
> -	if (!error) {
> -		/* Ensure device does not raise interrupts */
> -		omap4_keypad_stop(keypad_data);
> -	}
> -
> -	pm_runtime_put_sync(&pdev->dev);

Why are we moving this down? The idea was to make sure the power usage
counters are correct even if we exit probe early.

Can we call pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend()
here?

>  	if (error)
>  		return error;
>  
> +	/* Ensure device does not raise interrupts */
> +	omap4_keypad_stop(keypad_data);
> +
>  	/* input device allocation */
>  	keypad_data->input = input_dev = devm_input_allocate_device(dev);
>  	if (!input_dev)
> @@ -419,7 +445,8 @@ static int omap4_keypad_probe(struct platform_device *pdev)
>  	if (error)
>  		dev_warn(dev, "failed to set up wakeup irq: %d\n", error);
>  
> -	platform_set_drvdata(pdev, keypad_data);
> +	pm_runtime_mark_last_busy(&pdev->dev);
> +	pm_runtime_put_autosuspend(&pdev->dev);
>  
>  	return 0;
>  }
> -- 
> 2.30.0

Thanks.

-- 
Dmitry

Re: [PATCH 4/5] Input: omap4-keypad - use PM runtime autosuspend

[Tony Lindgren]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 05:01]:
> Hi Tony,
> 
> On Sun, Jan 10, 2021 at 09:05:28PM +0200, Tony Lindgren wrote:
> > @@ -350,15 +379,12 @@ static int omap4_keypad_probe(struct platform_device *pdev)
> >  
> >  	error = omap4_keypad_check_revision(&pdev->dev,
> >  					    keypad_data);
> > -	if (!error) {
> > -		/* Ensure device does not raise interrupts */
> > -		omap4_keypad_stop(keypad_data);
> > -	}
> > -
> > -	pm_runtime_put_sync(&pdev->dev);
> 
> Why are we moving this down? The idea was to make sure the power usage
> counters are correct even if we exit probe early.

Yes you are right, omap4_keypad_close() won't help with balancing the
get if we exit early.

> Can we call pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend()
> here?

Yes that should work as there's no more register access during the probe.

Regards,

Tony

Re: [PATCH 2/5] Input: omap4-keypad - scan keys in two phases and simplify with bitmask

[Tony Lindgren]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 04:48]:
> Technically speaking, userspace is free to accumulate the events until
> it receives EV_SYN/SYN_REPORT event and process the events in the event
> packet in order it sees fit. So to achieve what you want, I think we
> should issue 2 input_sync()s, one for the release block, and another is
> for press. I think we can also simplify the code if we pass into the new
> scan function exact set of keys that are being released or pressed.
> 
> How about the version below?

Yes that works nicely.

Thanks,

Tony


> 
> Input: omap4-keypad - scan keys in two phases and simplify with bitmask
> 
> From: Tony Lindgren <tony@atomide.com>
> 
> Because of errata i689 the keyboard can idle with state where no key
> up interrupts are seen until after the next key press.
> 
> This means we need to first check for any lost key up events before
> scanning for new down events.
> 
> 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.
> 
> Let's also simplify things with for_each_set_bit() as suggested by
> Dmitry Torokhov <dmitry.torokhov@gmail.com>.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |   73 ++++++++++++++++++++-------------
>  1 file changed, 45 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index ab761aa66b6d..6dcf27af856d 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -78,7 +78,7 @@ struct omap4_keypad {
>  	u32 irqreg_offset;
>  	unsigned int row_shift;
>  	bool no_autorepeat;
> -	unsigned char key_state[8];
> +	u64 keys;
>  	unsigned short *keymap;
>  };
>  
> @@ -107,6 +107,33 @@ static void kbd_write_irqreg(struct omap4_keypad *keypad_data,
>  		     keypad_data->base + keypad_data->irqreg_offset + offset);
>  }
>  
> +static int omap4_keypad_report_keys(struct omap4_keypad *keypad_data,
> +				    u64 keys, bool down)
> +{
> +	struct input_dev *input_dev = keypad_data->input;
> +	unsigned int col, row, code;
> +	DECLARE_BITMAP(mask, 64);
> +	unsigned long bit;
> +	int events = 0;
> +
> +	bitmap_from_u64(mask, keys);
> +
> +	for_each_set_bit(bit, mask, keypad_data->rows * BITS_PER_BYTE) {
> +		row = bit / BITS_PER_BYTE;
> +		col = bit % BITS_PER_BYTE;
> +		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], down);
> +
> +		events++;
> +	}
> +
> +	if (events)
> +		input_sync(input_dev);
> +
> +	return events;
> +}
>  
>  /* Interrupt handlers */
>  static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
> @@ -122,35 +149,25 @@ static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
>  static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
>  {
>  	struct omap4_keypad *keypad_data = dev_id;
> -	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 *new_state = (u32 *) key_state;
> -
> -	*new_state = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
> -	*(new_state + 1) = 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;
> -
> -		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));
> -			}
> -		}
> -	}
> +	u32 low, high;
> +	u64 keys, changed;
> +
> +	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
> +	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
> +	keys = low | (u64)high << 32;
> +
> +	changed = keys ^ keypad_data->keys;
> +
> +	/*
> +	 * Report key up events separately and first. This matters in case we
> +	 * lost key-up interrupt and just now catching up.
> +	 */
> +	omap4_keypad_report_keys(keypad_data, changed & ~keys, false);
>  
> -	input_sync(input_dev);
> +	/* Report key down events */
> +	omap4_keypad_report_keys(keypad_data, changed & keys, true);
>  
> -	memcpy(keypad_data->key_state, key_state,
> -		sizeof(keypad_data->key_state));
> +	keypad_data->keys = keys;
>  
>  	/* clear pending interrupts */
>  	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,

Re: [PATCH 3/5] Input: omap4-keypad - move rest of key scanning to a separate function

[Tony Lindgren]

* Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 04:50]:
> On Sun, Jan 10, 2021 at 09:05:27PM +0200, Tony Lindgren wrote:
> > Let's move rest of the key scanning code to omap4_keypad_scan_keys().
> > We will use omap4_keypad_scan_keys() also for implementing errata
> > handling to clear the stuck last key in the following patch.
> 
> And this one will become then...

Yes great, this works for me.

Thanks,

Tony


> 
> Input: omap4-keypad - move rest of key scanning to a separate function
> 
> From: Tony Lindgren <tony@atomide.com>
> 
> Let's move rest of the key scanning code to omap4_keypad_scan_keys().
> We will use omap4_keypad_scan_keys() also for implementing errata
> handling to clear the stuck last key in the following patch.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/keyboard/omap4-keypad.c |   39 ++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c
> index 6dcf27af856d..c48705dd049b 100644
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> @@ -71,6 +71,7 @@ struct omap4_keypad {
>  
>  	void __iomem *base;
>  	unsigned int irq;
> +	struct mutex lock;		/* for key scan */
>  
>  	unsigned int rows;
>  	unsigned int cols;
> @@ -135,6 +136,28 @@ static int omap4_keypad_report_keys(struct omap4_keypad *keypad_data,
>  	return events;
>  }
>  
> +static void omap4_keypad_scan_keys(struct omap4_keypad *keypad_data, u64 keys)
> +{
> +	u64 changed;
> +
> +	mutex_lock(&keypad_data->lock);
> +
> +	changed = keys ^ keypad_data->keys;
> +
> +	/*
> +	 * Report key up events separately and first. This matters in case we
> +	 * lost key-up interrupt and just now catching up.
> +	 */
> +	omap4_keypad_report_keys(keypad_data, changed & ~keys, false);
> +
> +	/* Report key down events */
> +	omap4_keypad_report_keys(keypad_data, changed & keys, true);
> +
> +	keypad_data->keys = keys;
> +
> +	mutex_unlock(&keypad_data->lock);
> +}
> +
>  /* Interrupt handlers */
>  static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
>  {
> @@ -150,24 +173,13 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
>  {
>  	struct omap4_keypad *keypad_data = dev_id;
>  	u32 low, high;
> -	u64 keys, changed;
> +	u64 keys;
>  
>  	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
>  	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
>  	keys = low | (u64)high << 32;
>  
> -	changed = keys ^ keypad_data->keys;
> -
> -	/*
> -	 * Report key up events separately and first. This matters in case we
> -	 * lost key-up interrupt and just now catching up.
> -	 */
> -	omap4_keypad_report_keys(keypad_data, changed & ~keys, false);
> -
> -	/* Report key down events */
> -	omap4_keypad_report_keys(keypad_data, changed & keys, true);
> -
> -	keypad_data->keys = keys;
> +	omap4_keypad_scan_keys(keypad_data, keys);
>  
>  	/* clear pending interrupts */
>  	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
> @@ -300,6 +312,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
>  	}
>  
>  	keypad_data->irq = irq;
> +	mutex_init(&keypad_data->lock);
>  
>  	error = omap4_keypad_parse_dt(dev, keypad_data);
>  	if (error)

Re: [PATCH 4/5] Input: omap4-keypad - use PM runtime autosuspend

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [210111 05:13]:
> * Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 05:01]:
> > Hi Tony,
> > 
> > On Sun, Jan 10, 2021 at 09:05:28PM +0200, Tony Lindgren wrote:
> > > @@ -350,15 +379,12 @@ static int omap4_keypad_probe(struct platform_device *pdev)
> > >  
> > >  	error = omap4_keypad_check_revision(&pdev->dev,
> > >  					    keypad_data);
> > > -	if (!error) {
> > > -		/* Ensure device does not raise interrupts */
> > > -		omap4_keypad_stop(keypad_data);
> > > -	}
> > > -
> > > -	pm_runtime_put_sync(&pdev->dev);
> > 
> > Why are we moving this down? The idea was to make sure the power usage
> > counters are correct even if we exit probe early.
> 
> Yes you are right, omap4_keypad_close() won't help with balancing the
> get if we exit early.
> 
> > Can we call pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend()
> > here?
> 
> Yes that should work as there's no more register access during the probe.

Below is an updated version. I updated probe to use dev instead of
&pdev->dev since we have it there. Does this look OK to you?

Regards,

Tony

8< ---------------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Sun, 10 Jan 2021 17:38:15 +0200
Subject: [PATCH] Input: omap4-keypad - use PM runtime autosuspend

Implement PM runtime autosuspend support to prepare for adding handling to
clear stuck last pressed key in the following patches. The hardware has
support for autoidle and can wake up to keypress events.

Let's also update omap4_keypad_probe() to use dev instead of &pdev->dev
since we already have it from the earlier changes.

Cc: Arthur Demchenkov <spinal.by@gmail.com>
Cc: Carl Philipp Klemm <philipp@uvos.xyz>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: ruleh <ruleh@gmx.de>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/omap4-keypad.c | 51 ++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 12 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
@@ -172,9 +172,17 @@ static irqreturn_t omap4_keypad_irq_handler(int irq, void *dev_id)
 static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 {
 	struct omap4_keypad *keypad_data = dev_id;
+	struct device *dev = keypad_data->input->dev.parent;
 	u32 low, high;
+	int error;
 	u64 keys;
 
+	error = pm_runtime_get_sync(dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(dev);
+		return IRQ_NONE;
+	}
+
 	low = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE31_0);
 	high = kbd_readl(keypad_data, OMAP4_KBD_FULLCODE63_32);
 	keys = low | (u64)high << 32;
@@ -185,14 +193,23 @@ static irqreturn_t omap4_keypad_irq_thread_fn(int irq, void *dev_id)
 	kbd_write_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS,
 			 kbd_read_irqreg(keypad_data, OMAP4_KBD_IRQSTATUS));
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return IRQ_HANDLED;
 }
 
 static int omap4_keypad_open(struct input_dev *input)
 {
 	struct omap4_keypad *keypad_data = input_get_drvdata(input);
+	struct device *dev = input->dev.parent;
+	int error;
 
-	pm_runtime_get_sync(input->dev.parent);
+	error = pm_runtime_get_sync(dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(dev);
+		return error;
+	}
 
 	disable_irq(keypad_data->irq);
 
@@ -211,6 +228,9 @@ static int omap4_keypad_open(struct input_dev *input)
 
 	enable_irq(keypad_data->irq);
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 }
 
@@ -228,14 +248,20 @@ static void omap4_keypad_stop(struct omap4_keypad *keypad_data)
 
 static void omap4_keypad_close(struct input_dev *input)
 {
-	struct omap4_keypad *keypad_data;
+	struct omap4_keypad *keypad_data = input_get_drvdata(input);
+	struct device *dev = input->dev.parent;
+	int error;
+
+	error = pm_runtime_get_sync(dev);
+	if (error < 0)
+		pm_runtime_put_noidle(dev);
 
-	keypad_data = input_get_drvdata(input);
 	disable_irq(keypad_data->irq);
 	omap4_keypad_stop(keypad_data);
 	enable_irq(keypad_data->irq);
 
-	pm_runtime_put_sync(input->dev.parent);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 }
 
 static int omap4_keypad_parse_dt(struct device *dev,
@@ -282,6 +308,7 @@ static int omap4_keypad_check_revision(struct device *dev,
 
 static void omap4_disable_pm(void *d)
 {
+	pm_runtime_dont_use_autosuspend(d);
 	pm_runtime_disable(d);
 }
 
@@ -314,6 +341,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 
 	keypad_data->irq = irq;
 	mutex_init(&keypad_data->lock);
+	platform_set_drvdata(pdev, keypad_data);
 
 	error = omap4_keypad_parse_dt(dev, keypad_data);
 	if (error)
@@ -323,6 +351,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	if (IS_ERR(keypad_data->base))
 		return PTR_ERR(keypad_data->base);
 
+	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
 	error = devm_add_action_or_reset(dev, omap4_disable_pm, dev);
@@ -335,21 +364,21 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	 * Enable clocks for the keypad module so that we can read
 	 * revision register.
 	 */
-	error = pm_runtime_get_sync(&pdev->dev);
+	error = pm_runtime_get_sync(dev);
 	if (error) {
-		dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n");
-		pm_runtime_put_noidle(&pdev->dev);
+		dev_err(dev, "pm_runtime_get_sync() failed\n");
+		pm_runtime_put_noidle(dev);
 		return error;
 	}
 
-	error = omap4_keypad_check_revision(&pdev->dev,
-					    keypad_data);
+	error = omap4_keypad_check_revision(dev, keypad_data);
 	if (!error) {
 		/* Ensure device does not raise interrupts */
 		omap4_keypad_stop(keypad_data);
 	}
 
-	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 	if (error)
 		return error;
 
@@ -413,8 +442,6 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 	if (error)
 		dev_warn(dev, "failed to set up wakeup irq: %d\n", error);
 
-	platform_set_drvdata(pdev, keypad_data);
-
 	return 0;
 }
 
-- 
2.30.0

Re: [PATCH 5/5] Input: omap4-keypad - implement errata check for lost key-up events

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [210110 19:08]:
> --- a/drivers/input/keyboard/omap4-keypad.c
> +++ b/drivers/input/keyboard/omap4-keypad.c
> +/*
> + * Errata ID i689 "1.32 Keyboard Key Up Event Can Be Missed".
> + * Interrupt may not happen for key-up events. We must clear stuck
> + * key-up events after the keyboard hardware has auto-idled.
> + */
> +static int __maybe_unused omap4_keypad_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> +	u32 active;
> +
> +	active = kbd_readl(keypad_data, OMAP4_KBD_STATEMACHINE);
> +	if (active) {
> +		pm_runtime_mark_last_busy(dev);
> +		return -EBUSY;
> +	}
> +
> +	omap4_keypad_scan_keys(keypad_data, true);
> +
> +	return 0;
> +}

So with the improvments done, here we need to replace the true above with 0
so when the hardware is idle we clear any stuck keys. Updated patch below.

Regards,

Tony

8< -----------------------
From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Thu, 17 Dec 2020 07:54:32 +0200
Subject: [PATCH] Input: omap4-keypad - implement errata check for lost
 key-up events

We are still missing handling for errata i689 related issues for the
case where we never see a key up interrupt for the last pressed key.

To fix the issue, we must scan the key state again after the keyboard
controller has idled to check if a key up event was missed. This is
described in the omap4 silicon errata documentation for Errata ID i689
"1.32 Keyboard Key Up Event Can Be Missed":

"When a key is released for a time shorter than the debounce time,
 in-between 2 key press (KP1 and KP2), the keyboard state machine will go
 to idle mode and will never detect the key release (after KP1, and also
 after KP2), and thus will never generate a new IRQ indicating the key
 release."

We can use PM runtime autosuspend features to check the keyboard state
after it enters idle.

Cc: Arthur Demchenkov <spinal.by@gmail.com>
Cc: Carl Philipp Klemm <philipp@uvos.xyz>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: ruleh <ruleh@gmx.de>
Cc: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/input/keyboard/omap4-keypad.c | 30 +++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

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
@@ -60,6 +60,8 @@
 	((((dbms) * 1000) / ((1 << ((ptv) + 1)) * (1000000 / 32768))) - 1)
 #define OMAP4_VAL_DEBOUNCINGTIME_16MS					\
 	OMAP4_KEYPAD_DEBOUNCINGTIME_MS(16, OMAP4_KEYPAD_PTV_DIV_128)
+#define OMAP4_KEYPAD_AUTOIDLE_MS	50	/* Approximate measured time */
+#define OMAP4_KEYPAD_IDLE_CHECK_MS	(OMAP4_KEYPAD_AUTOIDLE_MS / 2)
 
 enum {
 	KBD_REVISION_OMAP4 = 0,
@@ -306,6 +308,32 @@ static int omap4_keypad_check_revision(struct device *dev,
 	return 0;
 }
 
+/*
+ * Errata ID i689 "1.32 Keyboard Key Up Event Can Be Missed".
+ * Interrupt may not happen for key-up events. We must clear stuck
+ * key-up events after the keyboard hardware has auto-idled.
+ */
+static int __maybe_unused omap4_keypad_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
+	u32 active;
+
+	active = kbd_readl(keypad_data, OMAP4_KBD_STATEMACHINE);
+	if (active) {
+		pm_runtime_mark_last_busy(dev);
+		return -EBUSY;
+	}
+
+	omap4_keypad_scan_keys(keypad_data, 0);
+
+	return 0;
+}
+
+static const struct dev_pm_ops omap4_keypad_pm_ops = {
+	SET_RUNTIME_PM_OPS(omap4_keypad_runtime_suspend, NULL, NULL)
+};
+
 static void omap4_disable_pm(void *d)
 {
 	pm_runtime_dont_use_autosuspend(d);
@@ -352,6 +380,7 @@ static int omap4_keypad_probe(struct platform_device *pdev)
 		return PTR_ERR(keypad_data->base);
 
 	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, OMAP4_KEYPAD_IDLE_CHECK_MS);
 	pm_runtime_enable(dev);
 
 	error = devm_add_action_or_reset(dev, omap4_disable_pm, dev);
@@ -464,6 +493,7 @@ static struct platform_driver omap4_keypad_driver = {
 	.driver		= {
 		.name	= "omap4-keypad",
 		.of_match_table = omap_keypad_dt_match,
+		.pm = &omap4_keypad_pm_ops,
 	},
 };
 module_platform_driver(omap4_keypad_driver);
-- 
2.30.0

Re: [PATCH 1/5] Input: omap4-keypad - disable unused long interrupts

[Dmitry Torokhov]

On Sun, Jan 10, 2021 at 09:05:25PM +0200, Tony Lindgren wrote:
> We are not using the long events and they produce extra interrupts.
> Let's not enable them at all.
> 
> Note that also the v3.0.8 Linux Android kernel has long interrupts
> disabled.
> 
> Cc: Arthur Demchenkov <spinal.by@gmail.com>
> Cc: Carl Philipp Klemm <philipp@uvos.xyz>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: ruleh <ruleh@gmx.de>
> Cc: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Applied, thank you.

-- 
Dmitry

Re: [PATCH 2/5] Input: omap4-keypad - scan keys in two phases and simplify with bitmask

[Dmitry Torokhov]

On Mon, Jan 11, 2021 at 07:56:54AM +0200, Tony Lindgren wrote:
> * Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 04:48]:
> > Technically speaking, userspace is free to accumulate the events until
> > it receives EV_SYN/SYN_REPORT event and process the events in the event
> > packet in order it sees fit. So to achieve what you want, I think we
> > should issue 2 input_sync()s, one for the release block, and another is
> > for press. I think we can also simplify the code if we pass into the new
> > scan function exact set of keys that are being released or pressed.
> > 
> > How about the version below?
> 
> Yes that works nicely.

Excellent, applied, thank you.

-- 
Dmitry

Re: [PATCH 3/5] Input: omap4-keypad - move rest of key scanning to a separate function

[Dmitry Torokhov]

On Mon, Jan 11, 2021 at 07:57:39AM +0200, Tony Lindgren wrote:
> * Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 04:50]:
> > On Sun, Jan 10, 2021 at 09:05:27PM +0200, Tony Lindgren wrote:
> > > Let's move rest of the key scanning code to omap4_keypad_scan_keys().
> > > We will use omap4_keypad_scan_keys() also for implementing errata
> > > handling to clear the stuck last key in the following patch.
> > 
> > And this one will become then...
> 
> Yes great, this works for me.

OK, thanks, applied.

-- 
Dmitry

Re: [PATCH 4/5] Input: omap4-keypad - use PM runtime autosuspend

[Dmitry Torokhov]

On Mon, Jan 11, 2021 at 08:02:50AM +0200, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [210111 05:13]:
> > * Dmitry Torokhov <dmitry.torokhov@gmail.com> [210111 05:01]:
> > > Hi Tony,
> > > 
> > > On Sun, Jan 10, 2021 at 09:05:28PM +0200, Tony Lindgren wrote:
> > > > @@ -350,15 +379,12 @@ static int omap4_keypad_probe(struct platform_device *pdev)
> > > >  
> > > >  	error = omap4_keypad_check_revision(&pdev->dev,
> > > >  					    keypad_data);
> > > > -	if (!error) {
> > > > -		/* Ensure device does not raise interrupts */
> > > > -		omap4_keypad_stop(keypad_data);
> > > > -	}
> > > > -
> > > > -	pm_runtime_put_sync(&pdev->dev);
> > > 
> > > Why are we moving this down? The idea was to make sure the power usage
> > > counters are correct even if we exit probe early.
> > 
> > Yes you are right, omap4_keypad_close() won't help with balancing the
> > get if we exit early.
> > 
> > > Can we call pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend()
> > > here?
> > 
> > Yes that should work as there's no more register access during the probe.
> 
> Below is an updated version. I updated probe to use dev instead of
> &pdev->dev since we have it there. Does this look OK to you?

Yep, looks good, applied.

-- 
Dmitry

Re: [PATCH 5/5] Input: omap4-keypad - implement errata check for lost key-up events

[Dmitry Torokhov]

On Mon, Jan 11, 2021 at 08:10:18AM +0200, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [210110 19:08]:
> > --- a/drivers/input/keyboard/omap4-keypad.c
> > +++ b/drivers/input/keyboard/omap4-keypad.c
> > +/*
> > + * Errata ID i689 "1.32 Keyboard Key Up Event Can Be Missed".
> > + * Interrupt may not happen for key-up events. We must clear stuck
> > + * key-up events after the keyboard hardware has auto-idled.
> > + */
> > +static int __maybe_unused omap4_keypad_runtime_suspend(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct omap4_keypad *keypad_data = platform_get_drvdata(pdev);
> > +	u32 active;
> > +
> > +	active = kbd_readl(keypad_data, OMAP4_KBD_STATEMACHINE);
> > +	if (active) {
> > +		pm_runtime_mark_last_busy(dev);
> > +		return -EBUSY;
> > +	}
> > +
> > +	omap4_keypad_scan_keys(keypad_data, true);
> > +
> > +	return 0;
> > +}
> 
> So with the improvments done, here we need to replace the true above with 0
> so when the hardware is idle we clear any stuck keys. Updated patch below.

Applied, thank you.

-- 
Dmitry

Re: [PATCH 5/5] Input: omap4-keypad - implement errata check for lost key-up events

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

Hi!

> We are still missing handling for errata i689 related issues for the
> case where we never see a key up interrupt for the last pressed key.
> 
> To fix the issue, we must scan the key state again after the keyboard
> controller has idled to check if a key up event was missed. This is
> described in the omap4 silicon errata documentation for Errata ID i689
> "1.32 Keyboard Key Up Event Can Be Missed":
> 
> "When a key is released for a time shorter than the debounce time,
>  in-between 2 key press (KP1 and KP2), the keyboard state machine will go
>  to idle mode and will never detect the key release (after KP1, and also
>  after KP2), and thus will never generate a new IRQ indicating the key
>  release."
> 
> We can use PM runtime autosuspend features to check the keyboard state
> after it enters idle.

I thought about this and... is it reasonable?

Autosuspend is now required for correct operation. But autosuspend is
optional feature, configurable by user, and may not be available
depending on .config.

Do we need some other solution?
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 5/5] Input: omap4-keypad - implement errata check for lost key-up events

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [210111 08:34]:
> Hi!
> 
> > We are still missing handling for errata i689 related issues for the
> > case where we never see a key up interrupt for the last pressed key.
> > 
> > To fix the issue, we must scan the key state again after the keyboard
> > controller has idled to check if a key up event was missed. This is
> > described in the omap4 silicon errata documentation for Errata ID i689
> > "1.32 Keyboard Key Up Event Can Be Missed":
> > 
> > "When a key is released for a time shorter than the debounce time,
> >  in-between 2 key press (KP1 and KP2), the keyboard state machine will go
> >  to idle mode and will never detect the key release (after KP1, and also
> >  after KP2), and thus will never generate a new IRQ indicating the key
> >  release."
> > 
> > We can use PM runtime autosuspend features to check the keyboard state
> > after it enters idle.
> 
> I thought about this and... is it reasonable?
> 
> Autosuspend is now required for correct operation. But autosuspend is
> optional feature, configurable by user, and may not be available
> depending on .config.

Well suspending hardware that has (lost) events pending is wrong. So we
need to do this delayed hardware check at least when runtime suspending
the device.

> Do we need some other solution?

Not sure if other places make sense to do this check as we need to wait
about 50ms for hardware to autoidle, and only then check if there are
events pending, and then clear the pending events. The PM runtime suspend
function seems like a natural place to do this.

If PM runtime autosuspend is disabled, the issue with last pressed key
getting stuck sometimes can still happen like it did earlier. That issue
has always been there for past 10 or so years and nobody else bothered to
fix it so I'm not too worried about it.

With this series we already fix the bigger issue anyways where rapidly
pressing the keys would have the previous key stuck. Like rapid pressing
of shift shift j would produce an upper case J instead of j.

Naturally there is nothing stopping us from adding additional other places
to call omap4_keypad_scan_keys() as needed now though if you have some
good ideas for that :)

Regards,

Tony