* [PATCH] Input: amimouse - convert to platform remove callback returning void
From: Uwe Kleine-König @ 2023-12-01 13:37 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, kernel
The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart
from emitting a warning) and this typically results in resource leaks.
To improve here there is a quest to make the remove callback return
void. In the first step of this quest all drivers are converted to
.remove_new(), which already returns void. Eventually after all drivers
are converted, .remove_new() will be renamed to .remove().
Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,
some time ago I sent a series[1] with the intention to convert all input
drivers in one go. The coccinelle script I used for that missed this
driver however because of the __exit_p usage. So here comes a separate
patch.
Best regards
Uwe
[1] https://lore.kernel.org/r/20230920125829.1478827-1-u.kleine-koenig@pengutronix.de
drivers/input/mouse/amimouse.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/input/mouse/amimouse.c b/drivers/input/mouse/amimouse.c
index a50e50354832..cda0c3ff5a28 100644
--- a/drivers/input/mouse/amimouse.c
+++ b/drivers/input/mouse/amimouse.c
@@ -125,16 +125,15 @@ static int __init amimouse_probe(struct platform_device *pdev)
return 0;
}
-static int __exit amimouse_remove(struct platform_device *pdev)
+static void __exit amimouse_remove(struct platform_device *pdev)
{
struct input_dev *dev = platform_get_drvdata(pdev);
input_unregister_device(dev);
- return 0;
}
static struct platform_driver amimouse_driver = {
- .remove = __exit_p(amimouse_remove),
+ .remove_new = __exit_p(amimouse_remove),
.driver = {
.name = "amiga-mouse",
},
--
2.42.0
^ permalink raw reply related
* Re: [PATCH] Input: pxrc - simplify mutex handling with guard macro
From: Johan Hovold @ 2023-12-01 13:29 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <20231201-pxrc-guard-v1-1-38937e657368@gmail.com>
On Fri, Dec 01, 2023 at 01:08:45PM +0100, Marcus Folkesson wrote:
> Use the guard(mutex) macro for handle mutex lock/unlocks.
>
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
A couple of drive-by comments below.
> ---
> drivers/input/joystick/pxrc.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
> index ea2bf5951d67..3c3bf7179b46 100644
> --- a/drivers/input/joystick/pxrc.c
> +++ b/drivers/input/joystick/pxrc.c
> @@ -5,15 +5,17 @@
> * Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
> */
>
> -#include <linux/kernel.h>
> +#include <linux/cleanup.h>
> #include <linux/errno.h>
> -#include <linux/slab.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> #include <linux/uaccess.h>
> +
> #include <linux/usb.h>
> #include <linux/usb/input.h>
> -#include <linux/mutex.h>
> -#include <linux/input.h>
Looks like an unrelated change.
> #define PXRC_VENDOR_ID 0x1781
> #define PXRC_PRODUCT_ID 0x0898
> @@ -89,25 +91,20 @@ static int pxrc_open(struct input_dev *input)
> dev_err(&pxrc->intf->dev,
> "%s - usb_submit_urb failed, error: %d\n",
> __func__, retval);
> - retval = -EIO;
> - goto out;
> + return -EIO;
> }
>
> pxrc->is_open = true;
> -
> -out:
> - mutex_unlock(&pxrc->pm_mutex);
> - return retval;
> + return 0;
> }
Eh, this looks obviously broken. Did you not test this before
submitting? I assume lockdep would complain loudly too.
You're apparently the author of this driver and can test it, but I fear
the coming onslaught of untested guard conversions from the "cleanup"
crew. Not sure I find the result generally more readable either.
Johan
^ permalink raw reply
* [PATCH v2] Input: pxrc - simplify mutex handling with guard macro
From: Marcus Folkesson @ 2023-12-01 13:17 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Marcus Folkesson
Use the guard(mutex) macro for handle mutex lock/unlocks.
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
Changes in v2:
- Add guard in pxrc_open()
- Link to v1: https://lore.kernel.org/r/20231201-pxrc-guard-v1-1-38937e657368@gmail.com
---
drivers/input/joystick/pxrc.c | 29 ++++++++++++-----------------
1 file changed, 12 insertions(+), 17 deletions(-)
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index ea2bf5951d67..d4b699418361 100644
--- a/drivers/input/joystick/pxrc.c
+++ b/drivers/input/joystick/pxrc.c
@@ -5,15 +5,17 @@
* Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
*/
-#include <linux/kernel.h>
+#include <linux/cleanup.h>
#include <linux/errno.h>
-#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
#include <linux/uaccess.h>
+
#include <linux/usb.h>
#include <linux/usb/input.h>
-#include <linux/mutex.h>
-#include <linux/input.h>
#define PXRC_VENDOR_ID 0x1781
#define PXRC_PRODUCT_ID 0x0898
@@ -83,31 +85,26 @@ static int pxrc_open(struct input_dev *input)
struct pxrc *pxrc = input_get_drvdata(input);
int retval;
- mutex_lock(&pxrc->pm_mutex);
+ guard(mutex)(&pxrc->pm_mutex);
retval = usb_submit_urb(pxrc->urb, GFP_KERNEL);
if (retval) {
dev_err(&pxrc->intf->dev,
"%s - usb_submit_urb failed, error: %d\n",
__func__, retval);
- retval = -EIO;
- goto out;
+ return -EIO;
}
pxrc->is_open = true;
-
-out:
- mutex_unlock(&pxrc->pm_mutex);
- return retval;
+ return 0;
}
static void pxrc_close(struct input_dev *input)
{
struct pxrc *pxrc = input_get_drvdata(input);
- mutex_lock(&pxrc->pm_mutex);
+ guard(mutex)(&pxrc->pm_mutex);
usb_kill_urb(pxrc->urb);
pxrc->is_open = false;
- mutex_unlock(&pxrc->pm_mutex);
}
static void pxrc_free_urb(void *_pxrc)
@@ -208,10 +205,9 @@ static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
{
struct pxrc *pxrc = usb_get_intfdata(intf);
- mutex_lock(&pxrc->pm_mutex);
+ guard(mutex)(&pxrc->pm_mutex);
if (pxrc->is_open)
usb_kill_urb(pxrc->urb);
- mutex_unlock(&pxrc->pm_mutex);
return 0;
}
@@ -221,11 +217,10 @@ static int pxrc_resume(struct usb_interface *intf)
struct pxrc *pxrc = usb_get_intfdata(intf);
int retval = 0;
- mutex_lock(&pxrc->pm_mutex);
+ guard(mutex)(&pxrc->pm_mutex);
if (pxrc->is_open && usb_submit_urb(pxrc->urb, GFP_KERNEL) < 0)
retval = -EIO;
- mutex_unlock(&pxrc->pm_mutex);
return retval;
}
---
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
change-id: 20231201-pxrc-guard-03dc35771b36
Best regards,
--
Marcus Folkesson <marcus.folkesson@gmail.com>
^ permalink raw reply related
* [PATCH] Input: pxrc - simplify mutex handling with guard macro
From: Marcus Folkesson @ 2023-12-01 12:08 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Marcus Folkesson
Use the guard(mutex) macro for handle mutex lock/unlocks.
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
drivers/input/joystick/pxrc.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c
index ea2bf5951d67..3c3bf7179b46 100644
--- a/drivers/input/joystick/pxrc.c
+++ b/drivers/input/joystick/pxrc.c
@@ -5,15 +5,17 @@
* Copyright (C) 2018 Marcus Folkesson <marcus.folkesson@gmail.com>
*/
-#include <linux/kernel.h>
+#include <linux/cleanup.h>
#include <linux/errno.h>
-#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
#include <linux/uaccess.h>
+
#include <linux/usb.h>
#include <linux/usb/input.h>
-#include <linux/mutex.h>
-#include <linux/input.h>
#define PXRC_VENDOR_ID 0x1781
#define PXRC_PRODUCT_ID 0x0898
@@ -89,25 +91,20 @@ static int pxrc_open(struct input_dev *input)
dev_err(&pxrc->intf->dev,
"%s - usb_submit_urb failed, error: %d\n",
__func__, retval);
- retval = -EIO;
- goto out;
+ return -EIO;
}
pxrc->is_open = true;
-
-out:
- mutex_unlock(&pxrc->pm_mutex);
- return retval;
+ return 0;
}
static void pxrc_close(struct input_dev *input)
{
struct pxrc *pxrc = input_get_drvdata(input);
- mutex_lock(&pxrc->pm_mutex);
+ guard(mutex)(&pxrc->pm_mutex);
usb_kill_urb(pxrc->urb);
pxrc->is_open = false;
- mutex_unlock(&pxrc->pm_mutex);
}
static void pxrc_free_urb(void *_pxrc)
@@ -208,10 +205,9 @@ static int pxrc_suspend(struct usb_interface *intf, pm_message_t message)
{
struct pxrc *pxrc = usb_get_intfdata(intf);
- mutex_lock(&pxrc->pm_mutex);
+ guard(mutex)(&pxrc->pm_mutex);
if (pxrc->is_open)
usb_kill_urb(pxrc->urb);
- mutex_unlock(&pxrc->pm_mutex);
return 0;
}
@@ -221,11 +217,10 @@ static int pxrc_resume(struct usb_interface *intf)
struct pxrc *pxrc = usb_get_intfdata(intf);
int retval = 0;
- mutex_lock(&pxrc->pm_mutex);
+ guard(mutex)(&pxrc->pm_mutex);
if (pxrc->is_open && usb_submit_urb(pxrc->urb, GFP_KERNEL) < 0)
retval = -EIO;
- mutex_unlock(&pxrc->pm_mutex);
return retval;
}
---
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
change-id: 20231201-pxrc-guard-03dc35771b36
Best regards,
--
Marcus Folkesson <marcus.folkesson@gmail.com>
^ permalink raw reply related
* Re: [PATCH v5 11/17] platform/x86/amd/pmf: Add capability to sideload of policy binary
From: Shyam Sundar S K @ 2023-12-01 10:29 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input
In-Reply-To: <58ccd66-7229-4c83-fa86-ea7d7ff96068@linux.intel.com>
Hi Ilpo,
On 11/21/2023 5:47 PM, Ilpo Järvinen wrote:
> On Fri, 17 Nov 2023, Shyam Sundar S K wrote:
>
>> A policy binary is OS agnostic, and the same policies are expected to work
>> across the OSes. At times it becomes difficult to debug when the policies
>> inside the policy binaries starts to misbehave. Add a way to sideload such
>> policies independently to debug them via a debugfs entry.
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> index 5f10e5c6335e..f73663c629fe 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>
>> +#ifdef CONFIG_AMD_PMF_DEBUG
>> +static ssize_t amd_pmf_get_pb_data(struct file *filp, const char __user *buf,
>> + size_t length, loff_t *pos)
>> +{
>> + struct amd_pmf_dev *dev = filp->private_data;
>> + int ret;
>> +
>> + /* Policy binary size cannot exceed POLICY_BUF_MAX_SZ */
>> + if (length > POLICY_BUF_MAX_SZ || length == 0)
>> + return -EINVAL;
>> +
>> + dev->policy_sz = length;
>> + if (copy_from_user(dev->policy_buf, buf, dev->policy_sz))
>> + return -EFAULT;
>> +
>> + ret = amd_pmf_start_policy_engine(dev);
>
> Is this call safe against concurrent invocations from two racing writes?
>
> Other than that, this change looked fine.
This path gets enabled only when CONFIG_AMD_PMF_DEBUG option is
enabled. Also when enabled, did not observe anything really unusual
(like races). So I have retained the same code in v6.
Kindly take a look.
Thanks,
Shyam
>
>> + if (ret)
>> + return -EINVAL;
>> +
>> + return length;
>> +}
>
>
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Linux regression tracking #update (Thorsten Leemhuis) @ 2023-12-01 8:24 UTC (permalink / raw)
To: Jiri Kosina, David Revoy
Cc: jason.gerecke@wacom.com, jose.exposito89@gmail.com,
ilya.ostapyshyn@gmail.com, Nils Fuhler, Ping Cheng,
Peter Hutterer, Benjamin Tissoires, linux-kernel, linux-input,
Linux kernel regressions list
In-Reply-To: <877c9bed-181a-4fc0-a876-e17de65c9e4d@leemhuis.info>
[TLDR: This mail in primarily relevant for Linux kernel regression
tracking. See link in footer if these mails annoy you.]
On 02.11.23 08:44, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:
> On 01.11.23 20:37, Jiri Kosina wrote:
>> On Wed, 1 Nov 2023, David Revoy wrote:
>>
>>> I am emailing to draw your attention and expertise to a problem I had
>>> earlier this week with my Xp-Pen Artist 24 Pro display tablet under
>>> Fedora Linux 38 KDE after a kernel update.
>>>
>>> The second button on my stylus changed from a right-click (which I could
>>> customise with xsetwacom or any GUI like kcm-tablet) to a button that
>>> feels 'hardcoded' and now switches the whole device to an eraser mode.
>>> This makes my main tool unusable.
> [...]
> Thanks for the report. To be sure the issue doesn't fall through the
> cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
> tracking bot:
>
> #regzbot ^introduced 276e14e6c3
> https://www.davidrevoy.com/article995/how-a-kernel-update-broke-my-stylus-need-help
This thread got long, but from briefly skimming it, it seems like the
regressions was dealt with somehow and everybody is happy. Please holler
if I got it wrong, as I hereby remove this from the tracking:
#regzbot resolve: solution found
#regzbot ignore-activity
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.
^ permalink raw reply
* Re: [PATCH 12/12] selftests/hid: tablets: be stricter for some transitions
From: Peter Hutterer @ 2023-12-01 5:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Benjamin Tissoires, Shuah Khan, linux-input,
linux-kselftest, linux-kernel
In-Reply-To: <20231129-wip-selftests-v1-12-ba15a1fe1b0d@kernel.org>
On Wed, Nov 29, 2023 at 04:24:37PM +0100, Benjamin Tissoires wrote:
> To accommodate for legacy devices, we rely on the last state of a
> transition to be valid:
> for example when we test PEN_IS_OUT_OF_RANGE to PEN_IS_IN_CONTACT,
> any "normal" device that reports an InRange bit would insert a
> PEN_IS_IN_RANGE state between the 2.
>
> This is of course valid, but this solution prevents to detect false
> releases emitted by some firmware:
> when pressing an "eraser mode" button, they might send an extra
> PEN_IS_OUT_OF_RANGE that we may want to filter.
>
> So define 2 sets of transitions: one that is the ideal behavior, and
> one that is OK, it won't break user space, but we have serious doubts
> if we are doing the right thing. And depending on the test, either
> ask only for valid transitions, or tolerate weird ones.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> tools/testing/selftests/hid/tests/test_tablet.py | 122 +++++++++++++++++++----
> 1 file changed, 104 insertions(+), 18 deletions(-)
>
> diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
> index f24cf2e168a4..625dd9dcb935 100644
> --- a/tools/testing/selftests/hid/tests/test_tablet.py
> +++ b/tools/testing/selftests/hid/tests/test_tablet.py
> @@ -109,7 +109,7 @@ class PenState(Enum):
>
> return cls((touch, tool, button))
>
> - def apply(self, events) -> "PenState":
> + def apply(self, events, strict) -> "PenState":
fwiw, if you're doing type annotations anyway, it'd be nice to do them
for args as well, `strict: bool` in this case.
> if libevdev.EV_SYN.SYN_REPORT in events:
> raise ValueError("EV_SYN is in the event sequence")
> touch = self.touch
> @@ -148,13 +148,97 @@ class PenState(Enum):
> button = None
>
> new_state = PenState((touch, tool, button))
> - assert (
> - new_state in self.valid_transitions()
> - ), f"moving from {self} to {new_state} is forbidden"
> + if strict:
> + assert (
> + new_state in self.valid_transitions()
> + ), f"moving from {self} to {new_state} is forbidden"
> + else:
> + assert (
> + new_state in self.historical_tolerated_transitions()
> + ), f"moving from {self} to {new_state} is forbidden"
>
> return new_state
>
> def valid_transitions(self) -> Tuple["PenState", ...]:
> + """Following the state machine in the URL above.
> +
> + Note that those transitions are from the evdev point of view, not HID"""
> + if self == PenState.PEN_IS_OUT_OF_RANGE:
> + return (
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_ERASING,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE:
> + return (
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_CONTACT,
> + )
> +
> + if self == PenState.PEN_IS_IN_CONTACT:
> + return (
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_RANGE,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
> + return (
> + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_ERASING,
> + )
> +
> + if self == PenState.PEN_IS_ERASING:
> + return (
> + PenState.PEN_IS_ERASING,
> + PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
> + return (
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + )
> +
> + if self == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
> + return (
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
> + return (
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + )
> +
> + if self == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
> + return (
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + )
> +
> + return tuple()
> +
> + def historical_tolerated_transitions(self) -> Tuple["PenState", ...]:
s/historically/ to be grammatically correct, I guess.
> """Following the state machine in the URL above, with a couple of addition
> for skipping the in-range state, due to historical reasons.
>
> @@ -678,10 +762,12 @@ class BaseTest:
> self.debug_reports(r, uhdev, events)
> return events
>
> - def validate_transitions(self, from_state, pen, evdev, events):
> + def validate_transitions(self, from_state, pen, evdev, events, allow_intermediate_states):
> # check that the final state is correct
> pen.assert_expected_input_events(evdev)
>
> + state = from_state
> +
> # check that the transitions are valid
> sync_events = []
> while libevdev.InputEvent(libevdev.EV_SYN.SYN_REPORT) in events:
> @@ -691,12 +777,12 @@ class BaseTest:
> events = events[idx + 1 :]
>
> # now check for a valid transition
> - from_state = from_state.apply(sync_events)
> + state = state.apply(sync_events, not allow_intermediate_states)
>
> if events:
> - from_state = from_state.apply(sync_events)
> + state = state.apply(sync_events, not allow_intermediate_states)
>
> - def _test_states(self, state_list, scribble):
> + def _test_states(self, state_list, scribble, allow_intermediate_states):
> """Internal method to test against a list of
> transition between states.
> state_list is a list of PenState objects
> @@ -711,7 +797,7 @@ class BaseTest:
> p = Pen(50, 60)
> uhdev.move_to(p, PenState.PEN_IS_OUT_OF_RANGE)
> events = self.post(uhdev, p)
> - self.validate_transitions(cur_state, p, evdev, events)
> + self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
>
> cur_state = p.current_state
>
> @@ -720,14 +806,14 @@ class BaseTest:
> p.x += 1
> p.y -= 1
> events = self.post(uhdev, p)
> - self.validate_transitions(cur_state, p, evdev, events)
> + self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
> assert len(events) >= 3 # X, Y, SYN
> uhdev.move_to(p, state)
> if scribble and state != PenState.PEN_IS_OUT_OF_RANGE:
> p.x += 1
> p.y -= 1
> events = self.post(uhdev, p)
> - self.validate_transitions(cur_state, p, evdev, events)
> + self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
> cur_state = p.current_state
>
> @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
> @@ -740,7 +826,7 @@ class BaseTest:
> we don't have Invert nor Erase bits, so just move in/out-of-range or proximity.
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
> """
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, False)
not everyone's cup of tea but code like this becomes more immediately
readable as:
self._test_states(state_list, scribble, allow_intermediate_states=False)
Anyway, this looks good to me (esp the intention) and is
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Cheers,
Peter
>
> @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
> @pytest.mark.parametrize(
> @@ -754,7 +840,7 @@ class BaseTest:
> """This is not adhering to the Windows Pen Implementation state machine
> but we should expect the kernel to behave properly, mostly for historical
> reasons."""
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, True)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Barrel Switch" not in uhdev.fields,
> @@ -770,7 +856,7 @@ class BaseTest:
> )
> def test_valid_primary_button_pen_states(self, state_list, scribble):
> """Rework the transition state machine by adding the primary button."""
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, False)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields,
> @@ -786,7 +872,7 @@ class BaseTest:
> )
> def test_valid_secondary_button_pen_states(self, state_list, scribble):
> """Rework the transition state machine by adding the secondary button."""
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, False)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Invert" not in uhdev.fields,
> @@ -806,7 +892,7 @@ class BaseTest:
> to erase.
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
> """
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, False)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Invert" not in uhdev.fields,
> @@ -826,7 +912,7 @@ class BaseTest:
> to erase.
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
> """
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, True)
>
> @pytest.mark.skip_if_uhdev(
> lambda uhdev: "Invert" not in uhdev.fields,
> @@ -843,7 +929,7 @@ class BaseTest:
> For example, a pen that has the eraser button might wobble between
> touching and erasing if the tablet doesn't enforce the Windows
> state machine."""
> - self._test_states(state_list, scribble)
> + self._test_states(state_list, scribble, True)
>
>
> class GXTP_pen(PenDigitizer):
>
> --
> 2.41.0
>
^ permalink raw reply
* Re: [PATCH 09/12] selftests/hid: tablets: add variants of states with buttons
From: Peter Hutterer @ 2023-12-01 5:41 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Benjamin Tissoires, Shuah Khan, linux-input,
linux-kselftest, linux-kernel
In-Reply-To: <20231129-wip-selftests-v1-9-ba15a1fe1b0d@kernel.org>
On Wed, Nov 29, 2023 at 04:24:34PM +0100, Benjamin Tissoires wrote:
> Turns out that there are transitions that are unlikely to happen:
> for example, having both the tip switch and a button being changed
> at the same time (in the same report) would require either a very talented
> and precise user or a very bad hardware with a very low sampling rate.
>
> So instead of manually building the button test by hand and forgetting
> about some cases, let's reuse the state machine and transitions we have.
>
> This patch only adds the states and the valid transitions. The actual
> tests will be replaced later.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> tools/testing/selftests/hid/tests/test_tablet.py | 170 +++++++++++++++++++++--
> 1 file changed, 157 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
> index 83f6501fe984..80269d1a0f0a 100644
> --- a/tools/testing/selftests/hid/tests/test_tablet.py
> +++ b/tools/testing/selftests/hid/tests/test_tablet.py
> @@ -21,22 +21,66 @@ logger = logging.getLogger("hidtools.test.tablet")
> class PenState(Enum):
> """Pen states according to Microsoft reference:
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
> - """
>
> - PEN_IS_OUT_OF_RANGE = (False, None)
> - PEN_IS_IN_RANGE = (False, libevdev.EV_KEY.BTN_TOOL_PEN)
> - PEN_IS_IN_CONTACT = (True, libevdev.EV_KEY.BTN_TOOL_PEN)
> - PEN_IS_IN_RANGE_WITH_ERASING_INTENT = (False, libevdev.EV_KEY.BTN_TOOL_RUBBER)
> - PEN_IS_ERASING = (True, libevdev.EV_KEY.BTN_TOOL_RUBBER)
> + We extend it with the various buttons when we need to check them.
> + """
>
> - def __init__(self, touch, tool):
It'd be nice to have a comment here what the False refers to. Even
nicer would be an enum class BtnTouch.DOWN so the code is instantly
readable :)
Cheers,
Peter
> + PEN_IS_OUT_OF_RANGE = (False, None, None)
> + PEN_IS_IN_RANGE = (False, libevdev.EV_KEY.BTN_TOOL_PEN, None)
> + PEN_IS_IN_RANGE_WITH_BUTTON = (
> + False,
> + libevdev.EV_KEY.BTN_TOOL_PEN,
> + libevdev.EV_KEY.BTN_STYLUS,
> + )
> + PEN_IS_IN_RANGE_WITH_SECOND_BUTTON = (
> + False,
> + libevdev.EV_KEY.BTN_TOOL_PEN,
> + libevdev.EV_KEY.BTN_STYLUS2,
> + )
> + PEN_IS_IN_CONTACT = (True, libevdev.EV_KEY.BTN_TOOL_PEN, None)
> + PEN_IS_IN_CONTACT_WITH_BUTTON = (
> + True,
> + libevdev.EV_KEY.BTN_TOOL_PEN,
> + libevdev.EV_KEY.BTN_STYLUS,
> + )
> + PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON = (
> + True,
> + libevdev.EV_KEY.BTN_TOOL_PEN,
> + libevdev.EV_KEY.BTN_STYLUS2,
> + )
> + PEN_IS_IN_RANGE_WITH_ERASING_INTENT = (False, libevdev.EV_KEY.BTN_TOOL_RUBBER, None)
> + PEN_IS_IN_RANGE_WITH_ERASING_INTENT_WITH_BUTTON = (
> + False,
> + libevdev.EV_KEY.BTN_TOOL_RUBBER,
> + libevdev.EV_KEY.BTN_STYLUS,
> + )
> + PEN_IS_IN_RANGE_WITH_ERASING_INTENT_WITH_SECOND_BUTTON = (
> + False,
> + libevdev.EV_KEY.BTN_TOOL_RUBBER,
> + libevdev.EV_KEY.BTN_STYLUS2,
> + )
> + PEN_IS_ERASING = (True, libevdev.EV_KEY.BTN_TOOL_RUBBER, None)
> + PEN_IS_ERASING_WITH_BUTTON = (
> + True,
> + libevdev.EV_KEY.BTN_TOOL_RUBBER,
> + libevdev.EV_KEY.BTN_STYLUS,
> + )
> + PEN_IS_ERASING_WITH_SECOND_BUTTON = (
> + True,
> + libevdev.EV_KEY.BTN_TOOL_RUBBER,
> + libevdev.EV_KEY.BTN_STYLUS2,
> + )
> +
> + def __init__(self, touch, tool, button):
> self.touch = touch
> self.tool = tool
> + self.button = button
>
> @classmethod
> def from_evdev(cls, evdev) -> "PenState":
> touch = bool(evdev.value[libevdev.EV_KEY.BTN_TOUCH])
> tool = None
> + button = None
> if (
> evdev.value[libevdev.EV_KEY.BTN_TOOL_RUBBER]
> and not evdev.value[libevdev.EV_KEY.BTN_TOOL_PEN]
> @@ -53,7 +97,17 @@ class PenState(Enum):
> ):
> raise ValueError("2 tools are not allowed")
>
> - return cls((touch, tool))
> + # we take only the highest button in account
> + for b in [libevdev.EV_KEY.BTN_STYLUS, libevdev.EV_KEY.BTN_STYLUS2]:
> + if bool(evdev.value[b]):
> + button = b
> +
> + # the kernel tends to insert an EV_SYN once removing the tool, so
> + # the button will be released after
> + if tool is None:
> + button = None
> +
> + return cls((touch, tool, button))
>
> def apply(self, events) -> "PenState":
> if libevdev.EV_SYN.SYN_REPORT in events:
> @@ -62,6 +116,8 @@ class PenState(Enum):
> touch_found = False
> tool = self.tool
> tool_found = False
> + button = self.button
> + button_found = False
>
> for ev in events:
> if ev == libevdev.InputEvent(libevdev.EV_KEY.BTN_TOUCH):
> @@ -76,12 +132,22 @@ class PenState(Enum):
> if tool_found:
> raise ValueError(f"duplicated BTN_TOOL_* in {events}")
> tool_found = True
> - if ev.value:
> - tool = ev.code
> - else:
> - tool = None
> + tool = ev.code if ev.value else None
> + elif ev in (
> + libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS),
> + libevdev.InputEvent(libevdev.EV_KEY.BTN_STYLUS2),
> + ):
> + if button_found:
> + raise ValueError(f"duplicated BTN_STYLUS* in {events}")
> + button_found = True
> + button = ev.code if ev.value else None
>
> - new_state = PenState((touch, tool))
> + # the kernel tends to insert an EV_SYN once removing the tool, so
> + # the button will be released after
> + if tool is None:
> + button = None
> +
> + new_state = PenState((touch, tool, button))
> assert (
> new_state in self.valid_transitions()
> ), f"moving from {self} to {new_state} is forbidden"
> @@ -97,14 +163,20 @@ class PenState(Enum):
> return (
> PenState.PEN_IS_OUT_OF_RANGE,
> PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
> PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> PenState.PEN_IS_ERASING,
> )
>
> if self == PenState.PEN_IS_IN_RANGE:
> return (
> PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> PenState.PEN_IS_OUT_OF_RANGE,
> PenState.PEN_IS_IN_CONTACT,
> )
> @@ -112,6 +184,8 @@ class PenState(Enum):
> if self == PenState.PEN_IS_IN_CONTACT:
> return (
> PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> PenState.PEN_IS_IN_RANGE,
> PenState.PEN_IS_OUT_OF_RANGE,
> )
> @@ -130,6 +204,38 @@ class PenState(Enum):
> PenState.PEN_IS_OUT_OF_RANGE,
> )
>
> + if self == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
> + return (
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + )
> +
> + if self == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
> + return (
> + PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + )
> +
> + if self == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
> + return (
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_RANGE,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + )
> +
> + if self == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
> + return (
> + PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_IN_CONTACT,
> + PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
> + PenState.PEN_IS_OUT_OF_RANGE,
> + )
> +
> return tuple()
>
> @staticmethod
> @@ -364,26 +470,64 @@ class PenDigitizer(base.UHIDTestDevice):
> pen.xtilt = 0
> pen.ytilt = 0
> pen.twist = 0
> + pen.barrelswitch = False
> + pen.secondarybarrelswitch = False
> elif state == PenState.PEN_IS_IN_RANGE:
> pen.tipswitch = False
> pen.inrange = True
> pen.invert = False
> pen.eraser = False
> + pen.barrelswitch = False
> + pen.secondarybarrelswitch = False
> elif state == PenState.PEN_IS_IN_CONTACT:
> pen.tipswitch = True
> pen.inrange = True
> pen.invert = False
> pen.eraser = False
> + pen.barrelswitch = False
> + pen.secondarybarrelswitch = False
> + elif state == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
> + pen.tipswitch = False
> + pen.inrange = True
> + pen.invert = False
> + pen.eraser = False
> + pen.barrelswitch = True
> + pen.secondarybarrelswitch = False
> + elif state == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
> + pen.tipswitch = True
> + pen.inrange = True
> + pen.invert = False
> + pen.eraser = False
> + pen.barrelswitch = True
> + pen.secondarybarrelswitch = False
> + elif state == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
> + pen.tipswitch = False
> + pen.inrange = True
> + pen.invert = False
> + pen.eraser = False
> + pen.barrelswitch = False
> + pen.secondarybarrelswitch = True
> + elif state == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
> + pen.tipswitch = True
> + pen.inrange = True
> + pen.invert = False
> + pen.eraser = False
> + pen.barrelswitch = False
> + pen.secondarybarrelswitch = True
> elif state == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
> pen.tipswitch = False
> pen.inrange = True
> pen.invert = True
> pen.eraser = False
> + pen.barrelswitch = False
> + pen.secondarybarrelswitch = False
> elif state == PenState.PEN_IS_ERASING:
> pen.tipswitch = False
> pen.inrange = True
> pen.invert = False
> pen.eraser = True
> + pen.barrelswitch = False
> + pen.secondarybarrelswitch = False
>
> pen.current_state = state
>
>
> --
> 2.41.0
>
^ permalink raw reply
* Re: [PATCH 02/12] selftests/hid: vmtest.sh: allow finer control on the build steps
From: Peter Hutterer @ 2023-12-01 5:34 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Benjamin Tissoires, Shuah Khan, linux-input,
linux-kselftest, linux-kernel
In-Reply-To: <20231129-wip-selftests-v1-2-ba15a1fe1b0d@kernel.org>
On Wed, Nov 29, 2023 at 04:24:27PM +0100, Benjamin Tissoires wrote:
> vmtest.sh works great for a one shot test, but not so much for CI where
> I want to build (with different configs) the bzImage in a separate
> job than the one I am running it.
>
> Add a "build_only" option to specify whether we need to boot the currently
> built kernel in the vm.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> tools/testing/selftests/hid/vmtest.sh | 42 ++++++++++++++++++++---------------
> 1 file changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/tools/testing/selftests/hid/vmtest.sh b/tools/testing/selftests/hid/vmtest.sh
> index 301b4e162336..52ada972833b 100755
> --- a/tools/testing/selftests/hid/vmtest.sh
> +++ b/tools/testing/selftests/hid/vmtest.sh
> @@ -32,7 +32,7 @@ DEFAULT_COMMAND="pip3 install hid-tools; make -C tools/testing/selftests TARGETS
> usage()
> {
> cat <<EOF
> -Usage: $0 [-i] [-s] [-d <output_dir>] -- [<command>]
> +Usage: $0 [-j N] [-s] [-b] [-d <output_dir>] -- [<command>]
>
> <command> is the command you would normally run when you are in
> the source kernel direcory. e.g:
> @@ -55,6 +55,7 @@ Options:
>
> -u) Update the boot2container script to a newer version.
> -d) Update the output directory (default: ${OUTPUT_DIR})
> + -b) Run the only build steps for the kernel and the selftests
typo: "run only the"
Cheers,
Peter
> -j) Number of jobs for compilation, similar to -j in make
> (default: ${NUM_COMPILE_JOBS})
> -s) Instead of powering off the VM, start an interactive
> @@ -191,8 +192,9 @@ main()
> local command="${DEFAULT_COMMAND}"
> local update_b2c="no"
> local debug_shell="no"
> + local build_only="no"
>
> - while getopts ':hsud:j:' opt; do
> + while getopts ':hsud:j:b' opt; do
> case ${opt} in
> u)
> update_b2c="yes"
> @@ -207,6 +209,9 @@ main()
> command="/bin/sh"
> debug_shell="yes"
> ;;
> + b)
> + build_only="yes"
> + ;;
> h)
> usage
> exit 0
> @@ -226,8 +231,7 @@ main()
> shift $((OPTIND -1))
>
> # trap 'catch "$?"' EXIT
> -
> - if [[ "${debug_shell}" == "no" ]]; then
> + if [[ "${build_only}" == "no" && "${debug_shell}" == "no" ]]; then
> if [[ $# -eq 0 ]]; then
> echo "No command specified, will run ${DEFAULT_COMMAND} in the vm"
> else
> @@ -267,24 +271,26 @@ main()
> update_kconfig "${kernel_checkout}" "${kconfig_file}"
>
> recompile_kernel "${kernel_checkout}" "${make_command}"
> + update_selftests "${kernel_checkout}" "${make_command}"
>
> - if [[ "${update_b2c}" == "no" && ! -f "${b2c}" ]]; then
> - echo "vm2c script not found in ${b2c}"
> - update_b2c="yes"
> - fi
> + if [[ "${build_only}" == "no" ]]; then
> + if [[ "${update_b2c}" == "no" && ! -f "${b2c}" ]]; then
> + echo "vm2c script not found in ${b2c}"
> + update_b2c="yes"
> + fi
>
> - if [[ "${update_b2c}" == "yes" ]]; then
> - download $B2C_URL $b2c
> - chmod +x $b2c
> - fi
> + if [[ "${update_b2c}" == "yes" ]]; then
> + download $B2C_URL $b2c
> + chmod +x $b2c
> + fi
>
> - update_selftests "${kernel_checkout}" "${make_command}"
> - run_vm "${kernel_checkout}" $b2c "${kernel_bzimage}" "${command}"
> - if [[ "${debug_shell}" != "yes" ]]; then
> - echo "Logs saved in ${OUTPUT_DIR}/${LOG_FILE}"
> - fi
> + run_vm "${kernel_checkout}" $b2c "${kernel_bzimage}" "${command}"
> + if [[ "${debug_shell}" != "yes" ]]; then
> + echo "Logs saved in ${OUTPUT_DIR}/${LOG_FILE}"
> + fi
>
> - exit $(cat ${OUTPUT_DIR}/${EXIT_STATUS_FILE})
> + exit $(cat ${OUTPUT_DIR}/${EXIT_STATUS_FILE})
> + fi
> }
>
> main "$@"
>
> --
> 2.41.0
>
^ permalink raw reply
* Re[4]: [PATCH] input: gpio-keys - optimize wakeup sequence.
From: Abhishek Kumar Singh @ 2023-12-01 4:27 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, robh@kernel.org
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
SRI-N IT Security
In-Reply-To: <CGME20231017103415epcms5p2f8f5b28a8f5d71055622b82f71b0fc93@epcms5p5>
Dear Dmitry,
Greetings!!
Could you please help to review and update on this?
Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer,
Samsung Electronics, Noida-India
--------- Original Message ---------
Sender : Abhishek Kumar Singh <abhi1.singh@samsung.com>System Power Part /SRI-Noida/Samsung Electronics
Date : 2023-11-07 13:59 (GMT+5:30)
Title : Re[2]: [PATCH] input: gpio-keys - optimize wakeup sequence.
Dear Dmitry,
Greetings!!!
Thank you so much for your response.
I checked in detailed again and observed the below points, please help to review
and approve it.
There is ISR "gpio_keys_gpio_isr" which is called when the key state is 1 i.e.
key pressed.
Therefore modified code will not impact on the existing driver code.
//For key pressed event:
<3>[ 549.180072] I[0: swapper/0: 0] gpio_keys_gpio_isr
<3>[ 549.196126] [1: kworker/1:1: 78] gpio_keys_gpio_work_func
<3>[ 549.196198] [1: kworker/1:1: 78] gpio-keys soc:gpio_keys: gpio_keys_gpio_report_event key = 115, value = 1
Performance:
I have calculated the differece between entry & exit time
with modified and without modified code and observed that
0.3ms extra computation time in current scenario in each entry/exit time.
Because below APIs will not be called in every resume functions:
1. static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
2. static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
3. gpiod_get_value_cansleep(bdata->gpiod);
4. input_event(input, type, *bdata->code, state);
5. input_sync(input)
So we can save 0.3ms computation time, resume operations will faster and save battery as well.
With changes:
Line 311960: 07-18 16:50:09.359 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:20:37.573207725 UTC
Line 312626: 07-18 16:50:42.123 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:20.503579404 UTC
Line 312627: 07-18 16:50:42.123 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:20.503656644 UTC
Line 313301: 07-18 16:52:24.182 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:33.865626325 UTC
Line 313302: 07-18 16:52:24.182 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:33.865724502 UTC
Line 313572: 07-18 16:52:35.111 root 0 0 I [1: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:37.678468979 UTC
Line 313573: 07-18 16:52:35.111 root 0 0 I [1: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:37.678566167 UTC
Line 314209: 07-18 16:52:43.598 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:23:05.925340634 UTC
Line 314210: 07-18 16:52:43.598 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:23:05.925439384 UTC
Without changes:
Line 372095: 07-18 16:10:24.250 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:43:38.592548979 UTC
Line 372344: 07-18 16:13:45.439 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:11.589164226 UTC
Line 372346: 07-18 16:13:45.439 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:11.589514955 UTC
Line 372573: 07-18 16:14:13.414 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:22.606227138 UTC
Line 372575: 07-18 16:14:13.414 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:22.606490107 UTC
Line 372944: 07-18 16:14:26.732 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:29.024121927 UTC
Line 372946: 07-18 16:14:26.732 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:29.024528958 UTC
Line 373181: 07-18 16:14:30.790 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:30.904866770 UTC
Line 373183: 07-18 16:14:30.790 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:30.905126353 UTC
Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India
--------- Original Message ---------
Sender : dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com>
Date : 2023-10-29 07:42 (GMT+5:30)
Title : Re: [PATCH] input: gpio-keys - optimize wakeup sequence.
To : Abhishek Kumar Singh<abhi1.singh@samsung.com>
CC : robh@kernel.org<robh@kernel.org>, linux-input@vger.kernel.org<linux-input@vger.kernel.org>, linux-kernel@vger.kernel.org<linux-kernel@vger.kernel.org>, SRI-N IT Security<sri-n.itsec@samsung.com>
Hi Abhishek,
On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
>
>
> The patch removes unused many APIs call chain for every suspend/resume of the device
> if no key press event triggered.
>
>
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
>
>
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
>
>
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time
Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.
I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.
Thanks.
--
Dmitry
Thanks and Regards,
Thanks and Regards,
Thanks and Regards,
^ permalink raw reply
* [dtor-input:next] BUILD SUCCESS a42b4bd51b2aec77fb38f1ce0f41846055a8d33c
From: kernel test robot @ 2023-12-01 0:19 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
branch HEAD: a42b4bd51b2aec77fb38f1ce0f41846055a8d33c dt-bindings: input: mediatek,pmic-keys: Drop incomplete example
elapsed time: 1461m
configs tested: 226
configs skipped: 2
The following configs have been built successfully.
More configs may be tested in the coming days.
tested configs:
alpha allnoconfig gcc
alpha allyesconfig gcc
alpha defconfig gcc
arc allmodconfig gcc
arc allnoconfig gcc
arc allyesconfig gcc
arc defconfig gcc
arc nsim_700_defconfig gcc
arc nsimosci_hs_smp_defconfig gcc
arc randconfig-001-20231130 gcc
arc randconfig-001-20231201 gcc
arc randconfig-002-20231130 gcc
arc randconfig-002-20231201 gcc
arc vdk_hs38_defconfig gcc
arm allmodconfig gcc
arm allnoconfig gcc
arm assabet_defconfig gcc
arm defconfig clang
arm exynos_defconfig gcc
arm gemini_defconfig gcc
arm randconfig-001-20231130 gcc
arm randconfig-001-20231201 gcc
arm randconfig-002-20231130 gcc
arm randconfig-002-20231201 gcc
arm randconfig-003-20231130 gcc
arm randconfig-003-20231201 gcc
arm randconfig-004-20231130 gcc
arm randconfig-004-20231201 gcc
arm spear6xx_defconfig gcc
arm64 allmodconfig clang
arm64 allnoconfig gcc
arm64 defconfig gcc
arm64 randconfig-001-20231130 gcc
arm64 randconfig-001-20231201 gcc
arm64 randconfig-002-20231130 gcc
arm64 randconfig-002-20231201 gcc
arm64 randconfig-003-20231130 gcc
arm64 randconfig-003-20231201 gcc
arm64 randconfig-004-20231130 gcc
arm64 randconfig-004-20231201 gcc
csky allnoconfig gcc
csky defconfig gcc
csky randconfig-001-20231130 gcc
csky randconfig-001-20231201 gcc
csky randconfig-002-20231130 gcc
csky randconfig-002-20231201 gcc
hexagon allmodconfig clang
hexagon allnoconfig clang
hexagon allyesconfig clang
hexagon defconfig clang
i386 allmodconfig clang
i386 allnoconfig clang
i386 allyesconfig clang
i386 defconfig gcc
i386 randconfig-011-20231130 clang
i386 randconfig-011-20231201 clang
i386 randconfig-012-20231130 clang
i386 randconfig-012-20231201 clang
i386 randconfig-013-20231130 clang
i386 randconfig-013-20231201 clang
i386 randconfig-014-20231130 clang
i386 randconfig-014-20231201 clang
i386 randconfig-015-20231130 clang
i386 randconfig-015-20231201 clang
i386 randconfig-016-20231130 clang
i386 randconfig-016-20231201 clang
loongarch alldefconfig gcc
loongarch allmodconfig gcc
loongarch allnoconfig gcc
loongarch allyesconfig gcc
loongarch defconfig gcc
loongarch randconfig-001-20231130 gcc
loongarch randconfig-001-20231201 gcc
loongarch randconfig-002-20231130 gcc
loongarch randconfig-002-20231201 gcc
m68k allmodconfig gcc
m68k allnoconfig gcc
m68k allyesconfig gcc
m68k defconfig gcc
m68k hp300_defconfig gcc
m68k m5249evb_defconfig gcc
m68k m5272c3_defconfig gcc
m68k virt_defconfig gcc
microblaze allmodconfig gcc
microblaze allnoconfig gcc
microblaze allyesconfig gcc
microblaze defconfig gcc
mips allmodconfig gcc
mips allnoconfig clang
mips allyesconfig gcc
mips bmips_be_defconfig gcc
mips db1xxx_defconfig gcc
mips fuloong2e_defconfig gcc
mips rb532_defconfig gcc
nios2 allmodconfig gcc
nios2 allnoconfig gcc
nios2 allyesconfig gcc
nios2 defconfig gcc
nios2 randconfig-001-20231130 gcc
nios2 randconfig-001-20231201 gcc
nios2 randconfig-002-20231130 gcc
nios2 randconfig-002-20231201 gcc
openrisc allmodconfig gcc
openrisc allnoconfig gcc
openrisc allyesconfig gcc
openrisc defconfig gcc
openrisc or1ksim_defconfig gcc
parisc allmodconfig gcc
parisc allnoconfig gcc
parisc allyesconfig gcc
parisc defconfig gcc
parisc randconfig-001-20231130 gcc
parisc randconfig-001-20231201 gcc
parisc randconfig-002-20231130 gcc
parisc randconfig-002-20231201 gcc
parisc64 defconfig gcc
powerpc allmodconfig clang
powerpc allnoconfig gcc
powerpc allyesconfig clang
powerpc bamboo_defconfig gcc
powerpc ep8248e_defconfig gcc
powerpc maple_defconfig gcc
powerpc randconfig-001-20231130 gcc
powerpc randconfig-001-20231201 gcc
powerpc randconfig-002-20231130 gcc
powerpc randconfig-002-20231201 gcc
powerpc randconfig-003-20231130 gcc
powerpc randconfig-003-20231201 gcc
powerpc taishan_defconfig gcc
powerpc tqm8541_defconfig gcc
powerpc warp_defconfig gcc
powerpc64 randconfig-001-20231130 gcc
powerpc64 randconfig-001-20231201 gcc
powerpc64 randconfig-002-20231130 gcc
powerpc64 randconfig-002-20231201 gcc
powerpc64 randconfig-003-20231130 gcc
powerpc64 randconfig-003-20231201 gcc
riscv allmodconfig gcc
riscv allnoconfig clang
riscv allyesconfig gcc
riscv defconfig gcc
riscv randconfig-001-20231130 gcc
riscv randconfig-001-20231201 gcc
riscv randconfig-002-20231130 gcc
riscv randconfig-002-20231201 gcc
riscv rv32_defconfig clang
s390 allmodconfig gcc
s390 allnoconfig gcc
s390 allyesconfig gcc
s390 debug_defconfig gcc
s390 defconfig gcc
sh allmodconfig gcc
sh allnoconfig gcc
sh allyesconfig gcc
sh ap325rxa_defconfig gcc
sh apsh4ad0a_defconfig gcc
sh defconfig gcc
sh hp6xx_defconfig gcc
sh polaris_defconfig gcc
sh r7780mp_defconfig gcc
sh randconfig-001-20231130 gcc
sh randconfig-001-20231201 gcc
sh randconfig-002-20231130 gcc
sh randconfig-002-20231201 gcc
sh rsk7269_defconfig gcc
sh sdk7780_defconfig gcc
sh sdk7786_defconfig gcc
sh se7751_defconfig gcc
sh secureedge5410_defconfig gcc
sh sh03_defconfig gcc
sh sh7724_generic_defconfig gcc
sh sh7770_generic_defconfig gcc
sh sh7785lcr_32bit_defconfig gcc
sh shmin_defconfig gcc
sh ul2_defconfig gcc
sparc alldefconfig gcc
sparc allmodconfig gcc
sparc allyesconfig gcc
sparc sparc32_defconfig gcc
sparc64 allmodconfig gcc
sparc64 allyesconfig gcc
sparc64 defconfig gcc
sparc64 randconfig-001-20231130 gcc
sparc64 randconfig-001-20231201 gcc
sparc64 randconfig-002-20231130 gcc
sparc64 randconfig-002-20231201 gcc
um allmodconfig clang
um allnoconfig clang
um allyesconfig clang
um defconfig gcc
um i386_defconfig gcc
um randconfig-001-20231130 gcc
um randconfig-001-20231201 gcc
um randconfig-002-20231130 gcc
um randconfig-002-20231201 gcc
um x86_64_defconfig gcc
x86_64 allnoconfig gcc
x86_64 allyesconfig clang
x86_64 buildonly-randconfig-001-20231130 gcc
x86_64 buildonly-randconfig-002-20231130 gcc
x86_64 buildonly-randconfig-003-20231130 gcc
x86_64 buildonly-randconfig-004-20231130 gcc
x86_64 buildonly-randconfig-005-20231130 gcc
x86_64 buildonly-randconfig-006-20231130 gcc
x86_64 defconfig gcc
x86_64 kexec gcc
x86_64 randconfig-011-20231130 gcc
x86_64 randconfig-012-20231130 gcc
x86_64 randconfig-013-20231130 gcc
x86_64 randconfig-014-20231130 gcc
x86_64 randconfig-015-20231130 gcc
x86_64 randconfig-016-20231130 gcc
x86_64 randconfig-071-20231130 gcc
x86_64 randconfig-072-20231130 gcc
x86_64 randconfig-073-20231130 gcc
x86_64 randconfig-074-20231130 gcc
x86_64 randconfig-075-20231130 gcc
x86_64 randconfig-076-20231130 gcc
x86_64 rhel-8.3-rust clang
x86_64 rhel-8.3 gcc
xtensa allnoconfig gcc
xtensa allyesconfig gcc
xtensa randconfig-001-20231130 gcc
xtensa randconfig-001-20231201 gcc
xtensa randconfig-002-20231130 gcc
xtensa randconfig-002-20231201 gcc
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: David Revoy @ 2023-11-30 22:25 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: José Expósito, Eric GOUYER, Illia Ostapyshyn, jkosina,
jason.gerecke, linux-input, linux-kernel
In-Reply-To: <CAO-hwJKnH=Brhq7Jv020qQLROarvFiewnRb__0ZF9WVqDuqxLQ@mail.gmail.com>
Hi Benjamin,
> I've updated the HID-BPF filter, and you can find it in the latest pipeline[0].
> I've removed the extra "Stylus" and you should have a better
> experience with the upper button now.
> [0] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/jobs/52148274
Thanks for the update!
> I think we are done with the XP-Pen Pro 24. But now I wonder if the
> Pro 16 (gen2) doesn't also have those glitches.
> Could you send me the same debug sequence as the last time
> (transitions from/to touching the surface while holding the upper
> button) but on the 16 now?
Sure, here is the same action, three time but now on the Pro 16 (Gen2):
https://www.peppercarrot.com/extras/mailing-list/hid-records/XPPEN-Artist-16-Pro-Gen2/2023-11-30_XPPEN-Artist-16-Pro-Gen2_pen_tip-contact-and-press-release-upper-stylus-button-while-pressed-x3.txt
Have a good end of week,
David.
^ permalink raw reply
* Re: [PATCH v2] HID: uhid: replace deprecated strncpy with strscpy
From: Kees Cook @ 2023-11-30 21:59 UTC (permalink / raw)
To: David Rheinsberg, Jiri Kosina, Benjamin Tissoires, Justin Stitt
Cc: Kees Cook, linux-input, linux-kernel, linux-hardening
In-Reply-To: <20231003-strncpy-drivers-hid-uhid-c-v2-1-6a501402581e@google.com>
On Tue, 03 Oct 2023 21:01:58 +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> A suitable replacement is `strscpy` [2] due to the fact that it
> guarantees NUL-termination on the destination buffer without
> unnecessarily NUL-padding.
>
> [...]
Applied to for-next/hardening, thanks!
[1/1] HID: uhid: replace deprecated strncpy with strscpy
https://git.kernel.org/kees/c/d4011f6817ae
Take care,
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] Input: i8042 - add nomux quirk for Acer P459-G2-M
From: Dmitry Torokhov @ 2023-11-30 20:04 UTC (permalink / raw)
To: Esther Shimanovich
Cc: LKML, linux-input, Hans de Goede, Jonathan Denose,
Mattijs Korpershoek, Werner Sembach
In-Reply-To: <20231130195615.v2.1.Ibe78a9df97ecd18dc227a5cff67d3029631d9c11@changeid>
On Thu, Nov 30, 2023 at 07:56:19PM +0000, Esther Shimanovich wrote:
> After the laptop lid is opened, and the device resumes from S3 deep
> sleep, if the user presses a keyboard key while the screen is still black,
> the mouse and keyboard become unusable.
>
> Enabling this quirk prevents this behavior from occurring.
>
> Signed-off-by: Esther Shimanovich <eshimanovich@chromium.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH v2] Input: i8042 - add nomux quirk for Acer P459-G2-M
From: Esther Shimanovich @ 2023-11-30 19:56 UTC (permalink / raw)
To: LKML
Cc: linux-input, Esther Shimanovich, Dmitry Torokhov, Hans de Goede,
Jonathan Denose, Mattijs Korpershoek, Werner Sembach
After the laptop lid is opened, and the device resumes from S3 deep
sleep, if the user presses a keyboard key while the screen is still black,
the mouse and keyboard become unusable.
Enabling this quirk prevents this behavior from occurring.
Signed-off-by: Esther Shimanovich <eshimanovich@chromium.org>
---
Hello! Thanks for the prompt response. I had previously tried the
i8042.nomux=1 quirk which didn't seem to work. But I was trying it
together with other quirks, which may have changed the behavior.
When I applied the nomux quirk to the driver, that did fix the bug
on my device. Submitting the new patch!
Changes in v2:
- change noloop to nomux
drivers/input/serio/i8042-acpipnpio.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
index 028e45bd050bf..612c4e3427cef 100644
--- a/drivers/input/serio/i8042-acpipnpio.h
+++ b/drivers/input/serio/i8042-acpipnpio.h
@@ -360,6 +360,14 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
},
.driver_data = (void *)(SERIO_QUIRK_DRITEK)
},
+ {
+ /* Acer TravelMate P459-G2-M */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate P459-G2-M"),
+ },
+ .driver_data = (void *)(SERIO_QUIRK_NOMUX)
+ },
{
/* Amoi M636/A737 */
.matches = {
--
2.43.0.rc2.451.g8631bc7472-goog
^ permalink raw reply related
* Re: [PATCH 3/4] Input: omap-keypad - Drop optional GPIO support
From: Tony Lindgren @ 2023-11-30 12:15 UTC (permalink / raw)
To: Linus Walleij; +Cc: Dmitry Torokhov, linux-input
In-Reply-To: <20231129-descriptors-input-v1-3-9433162914a3@linaro.org>
* Linus Walleij <linus.walleij@linaro.org> [231129 13:51]:
> Remove the support for these unused GPIOs, if need be support can
> be reestablished in an organized fashion using GPIO descriptors.
Sounds good to me:
Reviewed-by: Tony Lindgren <tony@atomide.com>
^ permalink raw reply
* Re: [PATCH v4 5/5] ARM: dts: omap4-embt2ws: enable 32K clock on WLAN
From: Tony Lindgren @ 2023-11-30 12:03 UTC (permalink / raw)
To: Andreas Kemnade
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, mturquette, sboyd, linux-input, devicetree,
linux-kernel, linux-omap, linux-clk
In-Reply-To: <20231007065330.GP34982@atomide.com>
* Tony Lindgren <tony@atomide.com> [231007 06:53]:
> * Andreas Kemnade <andreas@kemnade.info> [230916 13:05]:
> > WLAN did only work if clock was left enabled by the original system,
> > so make it fully enable the needed resources itself.
>
> Seems applying this dts change before the clock patch is applied
> would break wlan so please let me know when this is safe to apply.
Applying into omap-for-v6.8/dt thanks.
Tony
^ permalink raw reply
* [PATCH] HID: core: clean up some inconsistent indenting
From: Jiapeng Chong @ 2023-11-30 3:13 UTC (permalink / raw)
To: jikos
Cc: benjamin.tissoires, linux-input, linux-kernel, Jiapeng Chong,
Abaci Robot
No functional modification involved.
drivers/hid/hid-core.c:2781 hid_add_device() warn: inconsistent indenting.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=7664
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
drivers/hid/hid-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index e0181218ad85..249107b6c863 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2778,10 +2778,10 @@ int hid_add_device(struct hid_device *hdev)
/*
* Check for the mandatory transport channel.
*/
- if (!hdev->ll_driver->raw_request) {
+ if (!hdev->ll_driver->raw_request) {
hid_err(hdev, "transport driver missing .raw_request()\n");
return -EINVAL;
- }
+ }
/*
* Read the device report descriptor once and use as template
--
2.20.1.7.g153144c
^ permalink raw reply related
* Re: [PATCH v4 2/4] Input: cs40l50 - Add cirrus haptics base support
From: James Ogletree @ 2023-11-29 22:22 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Lee Jones, Fred Treven, Ben Bright, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <ZWKW6BY9C3a9covT@nixie71>
>>>> +
>>>> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL);
>>>> + if (!words)
>>>> + return -ENOMEM;
>>>> +
>>>> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg,
>>>> + words, haptics->dsp.pseq_size);
>>>> + if (error)
>>>> + goto err_free;
>>>> +
>>>> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) {
>>>> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]);
>>>> + switch (operation) {
>>>> + case PSEQ_OP_END:
>>>> + case PSEQ_OP_WRITE_UNLOCK:
>>>> + num_words = PSEQ_OP_END_WORDS;
>>>> + break;
>>>> + case PSEQ_OP_WRITE_ADDR8:
>>>> + case PSEQ_OP_WRITE_H16:
>>>> + case PSEQ_OP_WRITE_L16:
>>>> + num_words = PSEQ_OP_WRITE_X16_WORDS;
>>>> + break;
>>>> + case PSEQ_OP_WRITE_FULL:
>>>> + num_words = PSEQ_OP_WRITE_FULL_WORDS;
>>>> + break;
>>>> + default:
>>>> + error = -EINVAL;
>>>> + dev_err(haptics->dev, "Unsupported op: %u\n", operation);
>>>> + goto err_free;
>>>> + }
>>>> +
>>>> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL);
>>>> + if (!op) {
>>>> + error = -ENOMEM;
>>>> + goto err_free;
>>>> + }
>>>> +
>>>> + op->size = num_words * sizeof(u32);
>>>> + memcpy(op->words, &words[i], op->size);
>>>> + op->offset = i * sizeof(u32);
>>>> + op->operation = operation;
>>>> + list_add(&op->list, &haptics->pseq_head);
>>>> +
>>>> + if (operation == PSEQ_OP_END)
>>>> + break;
>>>> + }
>>>> +
>>>> + if (operation != PSEQ_OP_END)
>>>> + error = -ENOENT;
>>>> +
>>>> +err_free:
>>>> + kfree(words);
>>>> +
>>>> + return error;
>>>> +}
>>>
>>> My first thought as I reviewed this patch was that this and the lot
>>> of pseq-related functions are not necessarily related to haptics, but
>>> rather CS40L50 register access and housekeeping in general.
>>>
>>> I seem to recall on L25 and friends that the the power-on sequencer,
>>> i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory
>>> that can play back a series of address/data pairs when the device
>>> comes out of hibernation, and any registers written during runtime
>>> must also be mirrored to the PSEQ for "playback" later. Is that still
>>> the case here?
>>>
>>> Assuming so, these functions seem like they belong in the MFD, not
>>> an input-specific library, because they will presumably be used by
>>> the codec driver as well, since that driver will write registers to
>>> set BCLK/LRCK ratio, etc.
>>>
>>> Therefore, I think it makes more sense for these functions to move to
>>> the MFD, which can then export them for use by the input/FF and codec
>>> children.
>>
>> I think the problem with moving the write sequencer code to the MFD
>> driver is that it would go unused in a codec-only environment. We only
>> need to write to the PSEQ when we want to maintain register settings
>> throughout hibernation cycles, and it isn’t possible to hibernate when
>> there is data streaming to the device. So the PSEQ will never be used
>> in the codec driver.
>
> I agree that in practice, the write sequencer would technically go unused
> in a codec-only implementation. However, that is because the ASoC core
> happens to write all pertinent registers ahead-of-time each time a stream
> starts. That is a property of the ASoC core and not L50; my feeling is that
> the driver should not be structured based on what one of the subsystems
> associated with it happens to do, but rather the nature of the hardware.
>
> Some specific reasons I think the MFD is a better home for the pseq code:
>
> 1. The write sequencer is a housekeeping function derived from the way
> the hardware implements its power management; it doesn't have anything
> to do with haptics. My opinion is that facilities supporting application-
> agnostic functions belong in the MFD, for all children to access, even
> if only one happens to do so today.
>
> 2. Over the course of the IC's life, you may be required to add errata
> writes after the IC is taken out of reset; these presumably would need
> to be "scheduled" in the write sequencer as well. These wouldn't make
> logical sense to add in the input driver, and they would be missed in
> the theoretical codec-only case.
>
> 3. This device has a programmable DSP; it wouldn't be unheard of for a
> customer to ask for a new function down the road that begets a third
> child device. Perhaps a customer asks for a special .wmfw file that
> repurposes the SDOUT pin as a PWM output for an LED, and now you must
> add a pwm child. That's a made-up example of course, but in general we
> want to structure things in such a way that rip-up is minimal in case
> requirements change in the future.
Great points. I agree now that the write sequencer code ought not to go in
cirrus_haptics.c. After talking it over with the internal team, I am considering
moving the write sequencer interface to cs_dsp.c. It’s an already existing
library with both Cirrus haptics and audio users. It seems to dodge your
concerns above and avoids a new common library as you suggested
below. Do you have any concerns on this approach over putting it in MFD?
>>> This leaves cirrus_haptics.* with only a few functions related to
>>> starting and stopping work, which seem specific enough to just live
>>> in cs40l50-vibra.c. Presumably many of those could be re-used by
>>> the L30 down the road, but in that case I think we'd be looking to
>>> actually re-use the L50 driver and simply add a compatible string
>>> for L30.
>>>
>>> I recall L30 has some overhead that L50 does not, which may make
>>> it hairy for cs40l50-vibra.c to support both. But in that case,
>>> you could always fork a cs40l30-vibra.c with its own compatible
>>> string, then have the L50 MFD selectively load the correct child
>>> based on device ID. To summarize, we should have:
>>>
>>> * drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery,
>>> IRQ handling, exported PSEQ functions, etc.
>>> * sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from
>>> the MFD.
>>> * drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop
>>> work, also uses PSEQ library from the MFD.
>>>
>>> And down the road, depending on complexity, maybe we also have:
>>>
>>> * drivers/input/misc/cs40l30-vibra.c: another input/FF driver that
>>> includes other functionality that didn't really fit in cs40l50-vibra.c;
>>> also uses PSEQ library from, and is loaded by, the original L50 MFD.
>>> If this driver duplicates small bits of cs40l50-vibra.c, it's not the
>>> end of the world.
>>>
>>> All of these files would #include include/linux/mfd/cs40l50.h. And
>>> finally, cirrus_haptics.* simply go away. Same idea, just slightly
>>> more scalable, and closer to common design patterns.
>>
>>
>> I understand that it is a common design pattern to selectively load
>> devices from the MFD driver. If I could summarize my thoughts on why
>> that would not be fitting here, it’s that the L26 and L50 share a ton of
>> input FF related work, and not enough “MFD core” related work.
>> Between errata differences, power supply configurations, regmap
>> configurations, interrupt register differences, it would seem to make for
>> a very awkward, scrambled MFD driver. Moreover, I think I will be moving
>> firmware download to the MFD driver, and that alone constitutes a
>> significant incompatibility because firmware downloading is compulsory
>> on L26, not so on L50.
>>
>> On the other hand, I want to emphasize the amount that L26 and
>> L50 share when it comes to the Input FF callbacks. The worker
>> functions in cirrus_haptics.c are bare-bones for this first
>> submission, but were designed to be totally generic and scalable to
>> the needs of L26 and all future Cirrus input drivers. While it might appear
>> too specific for L26, everything currently in cirrus_haptics is usable by
>> L26 as-is.
>>
>> For the above reasons I favor the current approach.
>>
>
> Likewise, if the input-related functions of L26 and L50 are nearly identical,
> then it's also perfectly acceptable for both drivers/mfd/cs40l26.c and
> drivers/mfd/cs40l50.c to load drivers/input/misc/cs40l50-vibra.c, which
> supports both L26 and L50 haptics-related functions. You're already doing
> something similar, but I disagree on the following:
>
> 1. Rather than have a library referenced by two drivers that support children
> which are largely the same in a logcial sense, just have a single driver that
> supports two children.
Your point here is clear and makes sense to me, especially now with the write
sequencer interface moving out. After considering the similarities and
differences closer, I am still a little wary. Maybe you can help me with these
concerns:
1. In the current implementation, drivers would be able to configure their own
input FF capabilities, and selectively register to input FF callbacks. L50 does
not register to the set_gain callback, whereas L26 does. I anticipate future
divergences, such as one driver supporting different effect types (see
the L50-specific error checking in cs40l50_add()). This would be exacerbated
by any future additional children.
2. This may be my lack of imagination, but in the current implementation it
seems much easier to develop new haptic features that don’t apply to all the
users of the library. One would simply wrap the feature in a boolean in
cirrus_haptics, which clients can take or leave. In the one driver
implementation, it seems you would have to find some clever, generalized
way of determining whether or not a feature can be used. This would also
seem to be exacerbated by any future additional children.
3. The current implementation provides for the individual drivers to setup
the haptics interface in whatever way peculiar to that device, whether that
interface be static (L50) or dependent on the loaded firmware (L26).
Since I am moving around a lot of code in and out of both -vibra.c and the
library for the next version, I think it would be helpful for me to wait until the
next version is submitted to decide on this. Would that be acceptable?
>
>
>>
>>>> +static void cs_hap_vibe_stop_worker(struct work_struct *work)
>>>> +{
>>>> + struct cs_hap *haptics = container_of(work, struct cs_hap,
>>>> + vibe_stop_work);
>>>> + int error;
>>>> +
>>>> + if (haptics->runtime_pm) {
>>>> + error = pm_runtime_resume_and_get(haptics->dev);
>>>> + if (error < 0) {
>>>> + haptics->stop_error = error;
>>>> + return;
>>>> + }
>>>> + }
>>>> +
>>>> + mutex_lock(&haptics->lock);
>>>> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
>>>> + haptics->dsp.stop_cmd);
>>>> + mutex_unlock(&haptics->lock);
>>>> +
>>>> + if (haptics->runtime_pm) {
>>>> + pm_runtime_mark_last_busy(haptics->dev);
>>>> + pm_runtime_put_autosuspend(haptics->dev);
>>>> + }
>>>> +
>>>> + haptics->stop_error = error;
>>>
>>> This seems like another argument for not separating the input/FF child
>>> from the meat of the driver; it just seems messy to pass around error
>>> codes within a driver's private data like this.
>>
>> I removed the start_error and stop_error members. However I think the
>> erase_error and add_error need to stay. I think this is more of a symptom
>> of the Input FF layer requiring error reporting and having to use workqueues
>> for those Input FF callbacks, than it is to do with the separation of these
>> functions from the driver. Point being, even if these were moved, we would still
>> need these *_error members. Let me know if I understood you right here.
>
> Sure, but why do adding and removing waveforms require workqueues? The DA7280
> driver doesn't do this; what is different in this case? (That's a genuine
> question, not an assertion that what you have is wrong, although it seems
> unique based on my limited search).
The reason why we have worker items for upload and erase input FF callbacks is
because we need to ensure their ordering with playback worker items, and we need
those worker items because the Input FF layer calls playbacks in atomic context.
Best,
James
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: mediatek,pmic-keys: Drop incomplete example
From: Dmitry Torokhov @ 2023-11-29 21:20 UTC (permalink / raw)
To: Rob Herring
Cc: Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, Chen Zhong, linux-input, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <20231128214816.3975893-1-robh@kernel.org>
On Tue, Nov 28, 2023 at 03:48:16PM -0600, Rob Herring wrote:
> The example for the Mediatek PMIC keys is incomplete as the binding is
> the full PMIC, not just the sub-functions. It is preferred for MFD
> examples to be complete in the top-level MFD device binding rather than
> piecemeal in each sub-function binding.
>
> This also fixes an undocumented (by schema) compatible warning for
> "mediatek,mt6397".
>
> Signed-off-by: Rob Herring <robh@kernel.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: sprd,sc27xx-vibrator: Drop incomplete example
From: Dmitry Torokhov @ 2023-11-29 21:19 UTC (permalink / raw)
To: Rob Herring
Cc: Krzysztof Kozlowski, Conor Dooley, Orson Zhai, Baolin Wang,
Chunyan Zhang, linux-input, devicetree, linux-kernel
In-Reply-To: <20231128214809.3975719-1-robh@kernel.org>
On Tue, Nov 28, 2023 at 03:48:09PM -0600, Rob Herring wrote:
> The example for the Spreadtrum SC27xx PMIC vibrator is incomplete as the
> binding is the full PMIC, not just the sub-functions. It is preferred
> for MFD examples to be complete in the top-level MFD device binding
> rather than piecemeal in each sub-function binding.
>
> This also fixes an undocumented (by schema) compatible warning for
> "sprd,sc2731".
>
> Signed-off-by: Rob Herring <robh@kernel.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: i8042 - add noloop quirk for Acer P459-G2-M
From: Dmitry Torokhov @ 2023-11-29 19:31 UTC (permalink / raw)
To: Esther Shimanovich
Cc: LKML, linux-input, Hans de Goede, Jonathan Denose,
Mattijs Korpershoek, Werner Sembach
In-Reply-To: <20231128214851.1.Ibc66f1742765467808fb28a394f8f35af920aa49@changeid>
On Tue, Nov 28, 2023 at 09:48:54PM +0000, Esther Shimanovich wrote:
> On the Acer P450-G2-M, after the user opens the laptop lid and the device
> resumes from S3 deep sleep, the screen remains dark for a few seconds.
> If the user presses a keyboard key while the screen is still dark, the
> mouse and keyboard stop functioning.
>
> To fix this bug, enable the noloop quirk.
Are you sure that you want to disable loop and not mux mode? The only
time we issue the "AUX LOOP" command on resume is when we try to restore
MUX configuration...
>
> Signed-off-by: Esther Shimanovich <eshimanovich@chromium.org>
> ---
>
> drivers/input/serio/i8042-acpipnpio.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
> index 028e45bd050bf..b81539bacb931 100644
> --- a/drivers/input/serio/i8042-acpipnpio.h
> +++ b/drivers/input/serio/i8042-acpipnpio.h
> @@ -941,6 +941,14 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
> },
> .driver_data = (void *)(SERIO_QUIRK_NOPNP)
> },
> + {
> + /* Acer TravelMate P459-G2-M */
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate P459-G2-M"),
> + },
> + .driver_data = (void *)(SERIO_QUIRK_NOLOOP)
> + },
> {
> /* ULI EV4873 - AUX LOOP does not work properly */
> .matches = {
> --
> 2.43.0.rc1.413.gea7ed67945-goog
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: Requesting your attention and expertise regarding a Tablet/Kernel issue
From: Benjamin Tissoires @ 2023-11-29 15:32 UTC (permalink / raw)
To: David Revoy
Cc: José Expósito, Eric GOUYER, Illia Ostapyshyn, jkosina,
jason.gerecke, linux-input, linux-kernel
In-Reply-To: <CAO-hwJLinACPsk=mEHrEz_YJroknmm=9PcX8byHiqEDxqOConQ@mail.gmail.com>
On Fri, Nov 24, 2023 at 6:18 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi David,
>
> On Thu, Nov 23, 2023 at 11:12 PM David Revoy <davidrevoy@protonmail.com> wrote:
> >
> > Hi Benjamin,
> >
> > Sorry for late reply.
> >
> > > So it would be nice if you could try the artifacts of job 51600738[4].
> > > Again, download them (udev-hid-bpf_0.1.0-4-g5ab02ec.tar.xz), unpack,
> > > sudo ./install --verbose, then unplug/replug the artist Pro 24.
> >
> > Ok, the main change I experienced after installing is xsetwacom
> > listing the ID name of the device with twice the name Stylus on
> > "UGTABLET 24 inch PenDisplay Stylus stylus". Before it was only
> > "UGTABLET 24 inch PenDisplay stylus".
> >
> > $ xsetwacom --list
> > UGTABLET 24 inch PenDisplay Stylus stylus id: 10 type: STYLUS
> >
> > Not a big deal, but I thought it was worth to mention it.
>
> Oh, this might be because I added a debug device. Given that there are
> 2 devices on the HID node, then one is tagged as Stylus by the kernel.
> Nothing to worry about.
>
> >
> > > Then, I'll need the following sequence (ideally repeated twice or
> > > three times, given that your last record show a slight difference in
> > > the first and second attempt):
> > >
> > > - outside of the proximity of the sensor, press the upper button
> > > - approach the stylus to the surface keeping the upper button pressed
> > > - touch the surface with the tip while holding the upper button pressed
> > > - release the upper button while keeping the tip pressed (like previously)
> > > - press once again the upper button while keeping the tip touching the
> > > surface (like previously)
> > > - lift of the pen, keeping the upper button pressed, and still in
> > > range of the sensor
> > > - remove the pen from the proximity of the sensor entirely (move away
> > > 20 cm or so), while still keeping the upper button pressed
> > >
> > > It's actually longer to describe than to execute :)
> > >
> >
> > Thank you for the detailed steps. True, it makes sens once
> > practising it. I made the gesture three time on:
> >
> > https://www.peppercarrot.com/extras/mailing-list/hid-records/XPPEN-Artist-24-Pro/2023-11-23_XPPEN-Artist-24-Pro_pen_tip-contact-and-press-release-upper-stylus-button-while-pressed-x3.txt
>
> Thanks a lot. And of course this device doesn't react in the way I expected :)
>
> Transitions from/to the tip touching the surface while the second
> button is pressed are normal, there are no extra events...
>
> But this also showed that the previous filter was better when pressing
> the upper button while touching the tip on the surface, because now we
> get another spurious event that was filtered before (and because it
> was filtered, I thought it was not there).
>
> Anyway, I couldn't rewrite the filter today, but I'll work on it next
> week for sure.
I've updated the HID-BPF filter, and you can find it in the latest pipeline[0].
I've removed the extra "Stylus" and you should have a better
experience with the upper button now.
>
>
> >
> > > But I would also totally understand that you had enough debugging and
> > > you would rather focus on using the tablets, instead of debugging
> > > them. In which case, someone else from the community might help me.
> >
> > No problem for continue testing, I'm around! Thank you again
> > for improving the behavior of the tablets.
> >
I think we are done with the XP-Pen Pro 24. But now I wonder if the
Pro 16 (gen2) doesn't also have those glitches.
Could you send me the same debug sequence as the last time
(transitions from/to touching the surface while holding the upper
button) but on the 16 now?
There is a chance I'll need the same filter to remove the extra left
click that might appear when you press the upper button while touching
the surface.
Cheers,
Benjamin
[0] https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/jobs/52148274
^ permalink raw reply
* [PATCH 12/12] selftests/hid: tablets: be stricter for some transitions
From: Benjamin Tissoires @ 2023-11-29 15:24 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231129-wip-selftests-v1-0-ba15a1fe1b0d@kernel.org>
To accommodate for legacy devices, we rely on the last state of a
transition to be valid:
for example when we test PEN_IS_OUT_OF_RANGE to PEN_IS_IN_CONTACT,
any "normal" device that reports an InRange bit would insert a
PEN_IS_IN_RANGE state between the 2.
This is of course valid, but this solution prevents to detect false
releases emitted by some firmware:
when pressing an "eraser mode" button, they might send an extra
PEN_IS_OUT_OF_RANGE that we may want to filter.
So define 2 sets of transitions: one that is the ideal behavior, and
one that is OK, it won't break user space, but we have serious doubts
if we are doing the right thing. And depending on the test, either
ask only for valid transitions, or tolerate weird ones.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/tests/test_tablet.py | 122 +++++++++++++++++++----
1 file changed, 104 insertions(+), 18 deletions(-)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index f24cf2e168a4..625dd9dcb935 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -109,7 +109,7 @@ class PenState(Enum):
return cls((touch, tool, button))
- def apply(self, events) -> "PenState":
+ def apply(self, events, strict) -> "PenState":
if libevdev.EV_SYN.SYN_REPORT in events:
raise ValueError("EV_SYN is in the event sequence")
touch = self.touch
@@ -148,13 +148,97 @@ class PenState(Enum):
button = None
new_state = PenState((touch, tool, button))
- assert (
- new_state in self.valid_transitions()
- ), f"moving from {self} to {new_state} is forbidden"
+ if strict:
+ assert (
+ new_state in self.valid_transitions()
+ ), f"moving from {self} to {new_state} is forbidden"
+ else:
+ assert (
+ new_state in self.historical_tolerated_transitions()
+ ), f"moving from {self} to {new_state} is forbidden"
return new_state
def valid_transitions(self) -> Tuple["PenState", ...]:
+ """Following the state machine in the URL above.
+
+ Note that those transitions are from the evdev point of view, not HID"""
+ if self == PenState.PEN_IS_OUT_OF_RANGE:
+ return (
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_ERASING,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE:
+ return (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_CONTACT,
+ )
+
+ if self == PenState.PEN_IS_IN_CONTACT:
+ return (
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT:
+ return (
+ PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_ERASING,
+ )
+
+ if self == PenState.PEN_IS_ERASING:
+ return (
+ PenState.PEN_IS_ERASING,
+ PenState.PEN_IS_IN_RANGE_WITH_ERASING_INTENT,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE_WITH_BUTTON:
+ return (
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ )
+
+ if self == PenState.PEN_IS_IN_CONTACT_WITH_BUTTON:
+ return (
+ PenState.PEN_IS_IN_CONTACT_WITH_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE_WITH_BUTTON,
+ )
+
+ if self == PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON:
+ return (
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ )
+
+ if self == PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON:
+ return (
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ )
+
+ return tuple()
+
+ def historical_tolerated_transitions(self) -> Tuple["PenState", ...]:
"""Following the state machine in the URL above, with a couple of addition
for skipping the in-range state, due to historical reasons.
@@ -678,10 +762,12 @@ class BaseTest:
self.debug_reports(r, uhdev, events)
return events
- def validate_transitions(self, from_state, pen, evdev, events):
+ def validate_transitions(self, from_state, pen, evdev, events, allow_intermediate_states):
# check that the final state is correct
pen.assert_expected_input_events(evdev)
+ state = from_state
+
# check that the transitions are valid
sync_events = []
while libevdev.InputEvent(libevdev.EV_SYN.SYN_REPORT) in events:
@@ -691,12 +777,12 @@ class BaseTest:
events = events[idx + 1 :]
# now check for a valid transition
- from_state = from_state.apply(sync_events)
+ state = state.apply(sync_events, not allow_intermediate_states)
if events:
- from_state = from_state.apply(sync_events)
+ state = state.apply(sync_events, not allow_intermediate_states)
- def _test_states(self, state_list, scribble):
+ def _test_states(self, state_list, scribble, allow_intermediate_states):
"""Internal method to test against a list of
transition between states.
state_list is a list of PenState objects
@@ -711,7 +797,7 @@ class BaseTest:
p = Pen(50, 60)
uhdev.move_to(p, PenState.PEN_IS_OUT_OF_RANGE)
events = self.post(uhdev, p)
- self.validate_transitions(cur_state, p, evdev, events)
+ self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
cur_state = p.current_state
@@ -720,14 +806,14 @@ class BaseTest:
p.x += 1
p.y -= 1
events = self.post(uhdev, p)
- self.validate_transitions(cur_state, p, evdev, events)
+ self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
assert len(events) >= 3 # X, Y, SYN
uhdev.move_to(p, state)
if scribble and state != PenState.PEN_IS_OUT_OF_RANGE:
p.x += 1
p.y -= 1
events = self.post(uhdev, p)
- self.validate_transitions(cur_state, p, evdev, events)
+ self.validate_transitions(cur_state, p, evdev, events, allow_intermediate_states)
cur_state = p.current_state
@pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
@@ -740,7 +826,7 @@ class BaseTest:
we don't have Invert nor Erase bits, so just move in/out-of-range or proximity.
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
"""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, False)
@pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
@pytest.mark.parametrize(
@@ -754,7 +840,7 @@ class BaseTest:
"""This is not adhering to the Windows Pen Implementation state machine
but we should expect the kernel to behave properly, mostly for historical
reasons."""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, True)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Barrel Switch" not in uhdev.fields,
@@ -770,7 +856,7 @@ class BaseTest:
)
def test_valid_primary_button_pen_states(self, state_list, scribble):
"""Rework the transition state machine by adding the primary button."""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, False)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields,
@@ -786,7 +872,7 @@ class BaseTest:
)
def test_valid_secondary_button_pen_states(self, state_list, scribble):
"""Rework the transition state machine by adding the secondary button."""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, False)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
@@ -806,7 +892,7 @@ class BaseTest:
to erase.
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
"""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, False)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
@@ -826,7 +912,7 @@ class BaseTest:
to erase.
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-pen-states
"""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, True)
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
@@ -843,7 +929,7 @@ class BaseTest:
For example, a pen that has the eraser button might wobble between
touching and erasing if the tablet doesn't enforce the Windows
state machine."""
- self._test_states(state_list, scribble)
+ self._test_states(state_list, scribble, True)
class GXTP_pen(PenDigitizer):
--
2.41.0
^ permalink raw reply related
* [PATCH 11/12] selftests/hid: tablets: add a secondary barrel switch test
From: Benjamin Tissoires @ 2023-11-29 15:24 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Peter Hutterer
Cc: linux-input, linux-kselftest, linux-kernel, Benjamin Tissoires
In-Reply-To: <20231129-wip-selftests-v1-0-ba15a1fe1b0d@kernel.org>
Some tablets report 2 barrel switches. We better test those too.
Use the same transistions description from the primary button tests.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/tests/test_tablet.py | 67 ++++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/tools/testing/selftests/hid/tests/test_tablet.py b/tools/testing/selftests/hid/tests/test_tablet.py
index 440a87170db1..f24cf2e168a4 100644
--- a/tools/testing/selftests/hid/tests/test_tablet.py
+++ b/tools/testing/selftests/hid/tests/test_tablet.py
@@ -351,6 +351,56 @@ class PenState(Enum):
),
}
+ @staticmethod
+ def legal_transitions_with_secondary_button() -> Dict[str, Tuple["PenState", ...]]:
+ """We revisit the Windows Pen Implementation state machine:
+ we now have a secondary button.
+ Note: we don't looks for 2 buttons interactions.
+ """
+ return {
+ "hover-button": (PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,),
+ "hover-button -> out-of-range": (
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_OUT_OF_RANGE,
+ ),
+ "in-range -> button-press": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ ),
+ "in-range -> button-press -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> touch -> button-press -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ ),
+ "in-range -> touch -> button-press -> release -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> button-press -> touch -> release -> button-release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ "in-range -> button-press -> touch -> button-release -> release": (
+ PenState.PEN_IS_IN_RANGE,
+ PenState.PEN_IS_IN_RANGE_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT_WITH_SECOND_BUTTON,
+ PenState.PEN_IS_IN_CONTACT,
+ PenState.PEN_IS_IN_RANGE,
+ ),
+ }
+
@staticmethod
def tolerated_transitions() -> Dict[str, Tuple["PenState", ...]]:
"""This is not adhering to the Windows Pen Implementation state machine
@@ -429,6 +479,7 @@ class Pen(object):
self.width = 10
self.height = 10
self.barrelswitch = False
+ self.secondarybarrelswitch = False
self.invert = False
self.eraser = False
self.xtilt = 1
@@ -721,6 +772,22 @@ class BaseTest:
"""Rework the transition state machine by adding the primary button."""
self._test_states(state_list, scribble)
+ @pytest.mark.skip_if_uhdev(
+ lambda uhdev: "Secondary Barrel Switch" not in uhdev.fields,
+ "Device not compatible, missing Secondary Barrel Switch usage",
+ )
+ @pytest.mark.parametrize("scribble", [True, False], ids=["scribble", "static"])
+ @pytest.mark.parametrize(
+ "state_list",
+ [
+ pytest.param(v, id=k)
+ for k, v in PenState.legal_transitions_with_secondary_button().items()
+ ],
+ )
+ def test_valid_secondary_button_pen_states(self, state_list, scribble):
+ """Rework the transition state machine by adding the secondary button."""
+ self._test_states(state_list, scribble)
+
@pytest.mark.skip_if_uhdev(
lambda uhdev: "Invert" not in uhdev.fields,
"Device not compatible, missing Invert usage",
--
2.41.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox