diff mbox series

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

Message ID 20210110190529.46135-6-tony@atomide.com (mailing list archive)
State New
Headers show
Series Lost key-up interrupt handling for omap4-keypad | expand

Commit Message

Tony Lindgren Jan. 10, 2021, 7:05 p.m. UTC
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(+)

Comments

Tony Lindgren Jan. 11, 2021, 6:10 a.m. UTC | #1
* 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);
Dmitry Torokhov Jan. 11, 2021, 6:28 a.m. UTC | #2
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.
Pavel Machek Jan. 11, 2021, 8:33 a.m. UTC | #3
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
Tony Lindgren Jan. 11, 2021, 9:02 a.m. UTC | #4
* 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
diff mbox series

Patch

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);