2024-01-15 05:00:19

by Kai-Heng Feng

[permalink] [raw]
Subject: [PATCH v2] HID: i2c-hid: Skip SET_POWER SLEEP for Cirque touchpad on system suspend

There's a Cirque touchpad that wakes system up without anything touched
the touchpad. The input report is empty when this happens.
The reason is stated in HID over I2C spec, 7.2.8.2:
"If the DEVICE wishes to wake the HOST from its low power state, it can
issue a wake by asserting the interrupt."

This is fine if OS can put system back to suspend by identifying input
wakeup count stays the same on resume, like Chrome OS Dark Resume [0].
But for regular distro such policy is lacking.

Though the change doesn't bring any impact on power consumption for
touchpad is minimal, other i2c-hid device may depends on SLEEP control
power. So use a quirk to limit the change scope.

[0] https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_manager/docs/dark_resume.md

Signed-off-by: Kai-Heng Feng <[email protected]>
---
v2:
- Use quirk instead of applying the change universally.

drivers/hid/hid-ids.h | 3 +++
drivers/hid/i2c-hid/i2c-hid-core.c | 6 +++++-
2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index fb30e228d35f..828a5c022c64 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -298,6 +298,9 @@

#define USB_VENDOR_ID_CIDC 0x1677

+#define I2C_VENDOR_ID_CIRQUE 0x0488
+#define I2C_PRODUCT_ID_CIRQUE_1063 0x1063
+
#define USB_VENDOR_ID_CJTOUCH 0x24b8
#define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0020 0x0020
#define USB_DEVICE_ID_CJTOUCH_MULTI_TOUCH_0040 0x0040
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 90f316ae9819..2df1ab3c31cc 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -49,6 +49,7 @@
#define I2C_HID_QUIRK_RESET_ON_RESUME BIT(2)
#define I2C_HID_QUIRK_BAD_INPUT_SIZE BIT(3)
#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET BIT(4)
+#define I2C_HID_QUIRK_NO_SLEEP_ON_SUSPEND BIT(5)

/* Command opcodes */
#define I2C_HID_OPCODE_RESET 0x01
@@ -131,6 +132,8 @@ static const struct i2c_hid_quirks {
I2C_HID_QUIRK_RESET_ON_RESUME },
{ USB_VENDOR_ID_ITE, I2C_DEVICE_ID_ITE_LENOVO_LEGION_Y720,
I2C_HID_QUIRK_BAD_INPUT_SIZE },
+ { I2C_VENDOR_ID_CIRQUE, I2C_PRODUCT_ID_CIRQUE_1063,
+ I2C_HID_QUIRK_NO_SLEEP_ON_SUSPEND },
/*
* Sending the wakeup after reset actually break ELAN touchscreen controller
*/
@@ -956,7 +959,8 @@ static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff)
return ret;

/* Save some power */
- i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+ if (!(ihid->quirks & I2C_HID_QUIRK_NO_SLEEP_ON_SUSPEND))
+ i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);

disable_irq(client->irq);

--
2.34.1



2024-01-17 20:35:54

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH v2] HID: i2c-hid: Skip SET_POWER SLEEP for Cirque touchpad on system suspend

Hi,

On Sun, Jan 14, 2024 at 8:51 PM Kai-Heng Feng
<[email protected]> wrote:
>
> There's a Cirque touchpad that wakes system up without anything touched
> the touchpad. The input report is empty when this happens.
> The reason is stated in HID over I2C spec, 7.2.8.2:
> "If the DEVICE wishes to wake the HOST from its low power state, it can
> issue a wake by asserting the interrupt."
>
> This is fine if OS can put system back to suspend by identifying input
> wakeup count stays the same on resume, like Chrome OS Dark Resume [0].
> But for regular distro such policy is lacking.
>
> Though the change doesn't bring any impact on power consumption for
> touchpad is minimal, other i2c-hid device may depends on SLEEP control
> power. So use a quirk to limit the change scope.
>
> [0] https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_manager/docs/dark_resume.md
>
> Signed-off-by: Kai-Heng Feng <[email protected]>
> ---
> v2:
> - Use quirk instead of applying the change universally.
>
> drivers/hid/hid-ids.h | 3 +++
> drivers/hid/i2c-hid/i2c-hid-core.c | 6 +++++-
> 2 files changed, 8 insertions(+), 1 deletion(-)

This seems OK to me. As per my repy to v1, it doesn't feel fully
explained what's going on. Why does it only wake the system without
touches if you put it to sleep first? Is it truly a hardware quirk
with the Cirque touchpad or is it a bug on the board you have it
hooked up to? In any case, perhaps it doesn't matter since you said
you measured power here and, on this touchpad it doesn't seem to save
significant extra power to go into sleep mode. ...so I guess I'd be OK
w/

Reviewed-by: Douglas Anderson <[email protected]>

[1] https://lore.kernel.org/r/[email protected]

2024-01-23 10:26:49

by Jiri Kosina

[permalink] [raw]
Subject: Re: [PATCH v2] HID: i2c-hid: Skip SET_POWER SLEEP for Cirque touchpad on system suspend

On Wed, 17 Jan 2024, Doug Anderson wrote:

> > There's a Cirque touchpad that wakes system up without anything
> > touched the touchpad. The input report is empty when this happens. The
> > reason is stated in HID over I2C spec, 7.2.8.2: "If the DEVICE wishes
> > to wake the HOST from its low power state, it can issue a wake by
> > asserting the interrupt."
> >
> > This is fine if OS can put system back to suspend by identifying input
> > wakeup count stays the same on resume, like Chrome OS Dark Resume [0].
> > But for regular distro such policy is lacking.
> >
> > Though the change doesn't bring any impact on power consumption for
> > touchpad is minimal, other i2c-hid device may depends on SLEEP control
> > power. So use a quirk to limit the change scope.
> >
> > [0] https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_manager/docs/dark_resume.md
> >
> > Signed-off-by: Kai-Heng Feng <[email protected]>
> > ---
> > v2:
> > - Use quirk instead of applying the change universally.
> >
> > drivers/hid/hid-ids.h | 3 +++
> > drivers/hid/i2c-hid/i2c-hid-core.c | 6 +++++-
> > 2 files changed, 8 insertions(+), 1 deletion(-)
>
> This seems OK to me. As per my repy to v1, it doesn't feel fully
> explained what's going on. Why does it only wake the system without
> touches if you put it to sleep first? Is it truly a hardware quirk with
> the Cirque touchpad or is it a bug on the board you have it hooked up
> to? In any case, perhaps it doesn't matter since you said you measured
> power here and, on this touchpad it doesn't seem to save significant
> extra power to go into sleep mode. ...so I guess I'd be OK w/
>
> Reviewed-by: Douglas Anderson <[email protected]>

Applied, thanks.

--
Jiri Kosina
SUSE Labs