* [PATCH] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4
@ 2025-09-09 11:00 Mario Limonciello (AMD)
2025-09-11 13:49 ` Benjamin Tissoires
2025-09-16 8:26 ` Benjamin Tissoires
0 siblings, 2 replies; 5+ messages in thread
From: Mario Limonciello (AMD) @ 2025-09-09 11:00 UTC (permalink / raw)
To: mario.limonciello, jikos, bentiss; +Cc: Mario Limonciello (AMD), linux-input
Dell systems utilize an EC-based touchpad emulation when the ACPI
touchpad _DSM is not invoked. This emulation acts as a secondary
master on the I2C bus, designed for scenarios where the I2C touchpad
driver is absent, such as in BIOS menus. Typically, loading the
i2c-hid module triggers the _DSM at initialization, disabling the
EC-based emulation.
However, if the i2c-hid module is missing from the boot kernel
used for hibernation snapshot restoration, the _DSM remains
uncalled, resulting in dual masters on the I2C bus and
subsequent arbitration errors. This issue arises when i2c-hid
resides in the rootfs instead of the kernel or initramfs.
To address this, switch from using the SYSTEM_SLEEP_PM_OPS()
macro to dedicated callbacks, introducing a specific
callback for restoring the S4 image. This callback ensures
the _DSM is invoked.
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
drivers/hid/i2c-hid/i2c-hid-acpi.c | 8 ++++++++
drivers/hid/i2c-hid/i2c-hid-core.c | 28 +++++++++++++++++++++++++++-
drivers/hid/i2c-hid/i2c-hid.h | 2 ++
3 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
index 1b49243adb16a..abd700a101f46 100644
--- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
@@ -76,6 +76,13 @@ static int i2c_hid_acpi_get_descriptor(struct i2c_hid_acpi *ihid_acpi)
return hid_descriptor_address;
}
+static void i2c_hid_acpi_restore_sequence(struct i2chid_ops *ops)
+{
+ struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
+
+ i2c_hid_acpi_get_descriptor(ihid_acpi);
+}
+
static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops)
{
struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
@@ -96,6 +103,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
ihid_acpi->adev = ACPI_COMPANION(dev);
ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail;
+ ihid_acpi->ops.restore_sequence = i2c_hid_acpi_restore_sequence;
ret = i2c_hid_acpi_get_descriptor(ihid_acpi);
if (ret < 0)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index d3912e3f2f13a..3257aa87be898 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -961,6 +961,14 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid)
ihid->ops->shutdown_tail(ihid->ops);
}
+static void i2c_hid_core_restore_sequence(struct i2c_hid *ihid)
+{
+ if (!ihid->ops->restore_sequence)
+ return;
+
+ ihid->ops->restore_sequence(ihid->ops);
+}
+
static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff)
{
struct i2c_client *client = ihid->client;
@@ -1360,8 +1368,26 @@ static int i2c_hid_core_pm_resume(struct device *dev)
return i2c_hid_core_resume(ihid);
}
+static int i2c_hid_core_pm_restore(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct i2c_hid *ihid = i2c_get_clientdata(client);
+
+ if (ihid->is_panel_follower)
+ return 0;
+
+ i2c_hid_core_restore_sequence(ihid);
+
+ return i2c_hid_core_resume(ihid);
+}
+
const struct dev_pm_ops i2c_hid_core_pm = {
- SYSTEM_SLEEP_PM_OPS(i2c_hid_core_pm_suspend, i2c_hid_core_pm_resume)
+ .suspend = pm_sleep_ptr(i2c_hid_core_pm_suspend),
+ .resume = pm_sleep_ptr(i2c_hid_core_pm_resume),
+ .freeze = pm_sleep_ptr(i2c_hid_core_pm_suspend),
+ .thaw = pm_sleep_ptr(i2c_hid_core_pm_resume),
+ .poweroff = pm_sleep_ptr(i2c_hid_core_pm_suspend),
+ .restore = pm_sleep_ptr(i2c_hid_core_pm_restore),
};
EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
index 2c7b66d5caa0f..1724a435c783a 100644
--- a/drivers/hid/i2c-hid/i2c-hid.h
+++ b/drivers/hid/i2c-hid/i2c-hid.h
@@ -27,11 +27,13 @@ static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
* @power_up: do sequencing to power up the device.
* @power_down: do sequencing to power down the device.
* @shutdown_tail: called at the end of shutdown.
+ * @restore_sequence: hibernation restore sequence.
*/
struct i2chid_ops {
int (*power_up)(struct i2chid_ops *ops);
void (*power_down)(struct i2chid_ops *ops);
void (*shutdown_tail)(struct i2chid_ops *ops);
+ void (*restore_sequence)(struct i2chid_ops *ops);
};
int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4
2025-09-09 11:00 [PATCH] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4 Mario Limonciello (AMD)
@ 2025-09-11 13:49 ` Benjamin Tissoires
2025-09-11 13:58 ` Mario Limonciello
2025-09-16 8:26 ` Benjamin Tissoires
1 sibling, 1 reply; 5+ messages in thread
From: Benjamin Tissoires @ 2025-09-11 13:49 UTC (permalink / raw)
To: Mario Limonciello (AMD); +Cc: mario.limonciello, jikos, linux-input
Hi Mario,
On Sep 09 2025, Mario Limonciello (AMD) wrote:
> Dell systems utilize an EC-based touchpad emulation when the ACPI
> touchpad _DSM is not invoked. This emulation acts as a secondary
> master on the I2C bus, designed for scenarios where the I2C touchpad
> driver is absent, such as in BIOS menus. Typically, loading the
> i2c-hid module triggers the _DSM at initialization, disabling the
> EC-based emulation.
>
> However, if the i2c-hid module is missing from the boot kernel
> used for hibernation snapshot restoration, the _DSM remains
> uncalled, resulting in dual masters on the I2C bus and
> subsequent arbitration errors. This issue arises when i2c-hid
> resides in the rootfs instead of the kernel or initramfs.
In the downstream Red Hat bug it was mentioned that the kernel
configuration had an impact though. But as I understand it now that I'm
re-reading it for the 4th time:
- on stock fedora kernel (i2c-hid in initramfs): works
- on stock RHEL kernel (i2c-hid in initramfs): bug but config issue
- on vanilla kernel without i2c-hid in initramfs: bug fixed by that
patch
- on vanilla kernel with i2c-hid in initramfs: works
So that patch is needed no matter the config bug we have there and it
will also fix that config bug. Am I correct?
>
> To address this, switch from using the SYSTEM_SLEEP_PM_OPS()
> macro to dedicated callbacks, introducing a specific
> callback for restoring the S4 image. This callback ensures
> the _DSM is invoked.
I'm a little bit hesitant to include this patch without proper testing.
Did you run this through a wider range of laptops than just the
problematic one?
Other than that, patch looks good to me, but I would have liked to have
another opinion on it.
Cheers,
Benjamin
>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> drivers/hid/i2c-hid/i2c-hid-acpi.c | 8 ++++++++
> drivers/hid/i2c-hid/i2c-hid-core.c | 28 +++++++++++++++++++++++++++-
> drivers/hid/i2c-hid/i2c-hid.h | 2 ++
> 3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> index 1b49243adb16a..abd700a101f46 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> @@ -76,6 +76,13 @@ static int i2c_hid_acpi_get_descriptor(struct i2c_hid_acpi *ihid_acpi)
> return hid_descriptor_address;
> }
>
> +static void i2c_hid_acpi_restore_sequence(struct i2chid_ops *ops)
> +{
> + struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
> +
> + i2c_hid_acpi_get_descriptor(ihid_acpi);
> +}
> +
> static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops)
> {
> struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
> @@ -96,6 +103,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
>
> ihid_acpi->adev = ACPI_COMPANION(dev);
> ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail;
> + ihid_acpi->ops.restore_sequence = i2c_hid_acpi_restore_sequence;
>
> ret = i2c_hid_acpi_get_descriptor(ihid_acpi);
> if (ret < 0)
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index d3912e3f2f13a..3257aa87be898 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -961,6 +961,14 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid)
> ihid->ops->shutdown_tail(ihid->ops);
> }
>
> +static void i2c_hid_core_restore_sequence(struct i2c_hid *ihid)
> +{
> + if (!ihid->ops->restore_sequence)
> + return;
> +
> + ihid->ops->restore_sequence(ihid->ops);
> +}
> +
> static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff)
> {
> struct i2c_client *client = ihid->client;
> @@ -1360,8 +1368,26 @@ static int i2c_hid_core_pm_resume(struct device *dev)
> return i2c_hid_core_resume(ihid);
> }
>
> +static int i2c_hid_core_pm_restore(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct i2c_hid *ihid = i2c_get_clientdata(client);
> +
> + if (ihid->is_panel_follower)
> + return 0;
> +
> + i2c_hid_core_restore_sequence(ihid);
> +
> + return i2c_hid_core_resume(ihid);
> +}
> +
> const struct dev_pm_ops i2c_hid_core_pm = {
> - SYSTEM_SLEEP_PM_OPS(i2c_hid_core_pm_suspend, i2c_hid_core_pm_resume)
> + .suspend = pm_sleep_ptr(i2c_hid_core_pm_suspend),
> + .resume = pm_sleep_ptr(i2c_hid_core_pm_resume),
> + .freeze = pm_sleep_ptr(i2c_hid_core_pm_suspend),
> + .thaw = pm_sleep_ptr(i2c_hid_core_pm_resume),
> + .poweroff = pm_sleep_ptr(i2c_hid_core_pm_suspend),
> + .restore = pm_sleep_ptr(i2c_hid_core_pm_restore),
> };
> EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
> index 2c7b66d5caa0f..1724a435c783a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.h
> +++ b/drivers/hid/i2c-hid/i2c-hid.h
> @@ -27,11 +27,13 @@ static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
> * @power_up: do sequencing to power up the device.
> * @power_down: do sequencing to power down the device.
> * @shutdown_tail: called at the end of shutdown.
> + * @restore_sequence: hibernation restore sequence.
> */
> struct i2chid_ops {
> int (*power_up)(struct i2chid_ops *ops);
> void (*power_down)(struct i2chid_ops *ops);
> void (*shutdown_tail)(struct i2chid_ops *ops);
> + void (*restore_sequence)(struct i2chid_ops *ops);
> };
>
> int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4
2025-09-11 13:49 ` Benjamin Tissoires
@ 2025-09-11 13:58 ` Mario Limonciello
2025-09-11 14:55 ` Benjamin Tissoires
0 siblings, 1 reply; 5+ messages in thread
From: Mario Limonciello @ 2025-09-11 13:58 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: jikos, linux-input
On 9/11/25 8:49 AM, Benjamin Tissoires wrote:
> Hi Mario,
>
> On Sep 09 2025, Mario Limonciello (AMD) wrote:
>> Dell systems utilize an EC-based touchpad emulation when the ACPI
>> touchpad _DSM is not invoked. This emulation acts as a secondary
>> master on the I2C bus, designed for scenarios where the I2C touchpad
>> driver is absent, such as in BIOS menus. Typically, loading the
>> i2c-hid module triggers the _DSM at initialization, disabling the
>> EC-based emulation.
>>
>> However, if the i2c-hid module is missing from the boot kernel
>> used for hibernation snapshot restoration, the _DSM remains
>> uncalled, resulting in dual masters on the I2C bus and
>> subsequent arbitration errors. This issue arises when i2c-hid
>> resides in the rootfs instead of the kernel or initramfs.
>
> In the downstream Red Hat bug it was mentioned that the kernel
> configuration had an impact though. But as I understand it now that I'm
> re-reading it for the 4th time:
> - on stock fedora kernel (i2c-hid in initramfs): works
> - on stock RHEL kernel (i2c-hid in initramfs): bug but config issue
> - on vanilla kernel without i2c-hid in initramfs: bug fixed by that
> patch
> - on vanilla kernel with i2c-hid in initramfs: works
The detail you're missing here for #4 is vanilla kernel *with Fedora
kernel config* not with a defconfig. A defconfig kernel I don't expect
enables all the config options needed for this hardware.
I've reproduced it with my own kernel config as well which is not the
Fedora or RHEL kernel config but rather a narrowed down config of
options that I normally use for testing.
>
> So that patch is needed no matter the config bug we have there and it
> will also fix that config bug. Am I correct?
Yes there are certainly circumstances it is needed if i2c-hid isn't
present in the kernel used to restore the hibernation image.
>
>>
>> To address this, switch from using the SYSTEM_SLEEP_PM_OPS()
>> macro to dedicated callbacks, introducing a specific
>> callback for restoring the S4 image. This callback ensures
>> the _DSM is invoked.
>
> I'm a little bit hesitant to include this patch without proper testing.
> Did you run this through a wider range of laptops than just the
> problematic one?
I have been including it in my (local) development and testing tree ever
since I made it about a month ago so I've thrown it at a random
assortment of hardware I boot that on. I have also been doing S4
testing explicitly because of checking that I didn't break S4 with by S4
at S5 series [1].
That hardware is all biased to be AMD hardware though.
[1]
https://lore.kernel.org/linux-usb/20250909191619.2580169-1-superm1@kernel.org/
>
> Other than that, patch looks good to me, but I would have liked to have
> another opinion on it.
>
> Cheers,
> Benjamin
>
>>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> drivers/hid/i2c-hid/i2c-hid-acpi.c | 8 ++++++++
>> drivers/hid/i2c-hid/i2c-hid-core.c | 28 +++++++++++++++++++++++++++-
>> drivers/hid/i2c-hid/i2c-hid.h | 2 ++
>> 3 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
>> index 1b49243adb16a..abd700a101f46 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
>> @@ -76,6 +76,13 @@ static int i2c_hid_acpi_get_descriptor(struct i2c_hid_acpi *ihid_acpi)
>> return hid_descriptor_address;
>> }
>>
>> +static void i2c_hid_acpi_restore_sequence(struct i2chid_ops *ops)
>> +{
>> + struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
>> +
>> + i2c_hid_acpi_get_descriptor(ihid_acpi);
>> +}
>> +
>> static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops)
>> {
>> struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
>> @@ -96,6 +103,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
>>
>> ihid_acpi->adev = ACPI_COMPANION(dev);
>> ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail;
>> + ihid_acpi->ops.restore_sequence = i2c_hid_acpi_restore_sequence;
>>
>> ret = i2c_hid_acpi_get_descriptor(ihid_acpi);
>> if (ret < 0)
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index d3912e3f2f13a..3257aa87be898 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -961,6 +961,14 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid)
>> ihid->ops->shutdown_tail(ihid->ops);
>> }
>>
>> +static void i2c_hid_core_restore_sequence(struct i2c_hid *ihid)
>> +{
>> + if (!ihid->ops->restore_sequence)
>> + return;
>> +
>> + ihid->ops->restore_sequence(ihid->ops);
>> +}
>> +
>> static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff)
>> {
>> struct i2c_client *client = ihid->client;
>> @@ -1360,8 +1368,26 @@ static int i2c_hid_core_pm_resume(struct device *dev)
>> return i2c_hid_core_resume(ihid);
>> }
>>
>> +static int i2c_hid_core_pm_restore(struct device *dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + struct i2c_hid *ihid = i2c_get_clientdata(client);
>> +
>> + if (ihid->is_panel_follower)
>> + return 0;
>> +
>> + i2c_hid_core_restore_sequence(ihid);
>> +
>> + return i2c_hid_core_resume(ihid);
>> +}
>> +
>> const struct dev_pm_ops i2c_hid_core_pm = {
>> - SYSTEM_SLEEP_PM_OPS(i2c_hid_core_pm_suspend, i2c_hid_core_pm_resume)
>> + .suspend = pm_sleep_ptr(i2c_hid_core_pm_suspend),
>> + .resume = pm_sleep_ptr(i2c_hid_core_pm_resume),
>> + .freeze = pm_sleep_ptr(i2c_hid_core_pm_suspend),
>> + .thaw = pm_sleep_ptr(i2c_hid_core_pm_resume),
>> + .poweroff = pm_sleep_ptr(i2c_hid_core_pm_suspend),
>> + .restore = pm_sleep_ptr(i2c_hid_core_pm_restore),
>> };
>> EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
>> index 2c7b66d5caa0f..1724a435c783a 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.h
>> +++ b/drivers/hid/i2c-hid/i2c-hid.h
>> @@ -27,11 +27,13 @@ static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
>> * @power_up: do sequencing to power up the device.
>> * @power_down: do sequencing to power down the device.
>> * @shutdown_tail: called at the end of shutdown.
>> + * @restore_sequence: hibernation restore sequence.
>> */
>> struct i2chid_ops {
>> int (*power_up)(struct i2chid_ops *ops);
>> void (*power_down)(struct i2chid_ops *ops);
>> void (*shutdown_tail)(struct i2chid_ops *ops);
>> + void (*restore_sequence)(struct i2chid_ops *ops);
>> };
>>
>> int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
>> --
>> 2.43.0
>>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4
2025-09-11 13:58 ` Mario Limonciello
@ 2025-09-11 14:55 ` Benjamin Tissoires
0 siblings, 0 replies; 5+ messages in thread
From: Benjamin Tissoires @ 2025-09-11 14:55 UTC (permalink / raw)
To: Mario Limonciello; +Cc: jikos, linux-input
On Sep 11 2025, Mario Limonciello wrote:
> On 9/11/25 8:49 AM, Benjamin Tissoires wrote:
> > Hi Mario,
> >
> > On Sep 09 2025, Mario Limonciello (AMD) wrote:
> > > Dell systems utilize an EC-based touchpad emulation when the ACPI
> > > touchpad _DSM is not invoked. This emulation acts as a secondary
> > > master on the I2C bus, designed for scenarios where the I2C touchpad
> > > driver is absent, such as in BIOS menus. Typically, loading the
> > > i2c-hid module triggers the _DSM at initialization, disabling the
> > > EC-based emulation.
> > >
> > > However, if the i2c-hid module is missing from the boot kernel
> > > used for hibernation snapshot restoration, the _DSM remains
> > > uncalled, resulting in dual masters on the I2C bus and
> > > subsequent arbitration errors. This issue arises when i2c-hid
> > > resides in the rootfs instead of the kernel or initramfs.
> >
> > In the downstream Red Hat bug it was mentioned that the kernel
> > configuration had an impact though. But as I understand it now that I'm
> > re-reading it for the 4th time:
> > - on stock fedora kernel (i2c-hid in initramfs): works
> > - on stock RHEL kernel (i2c-hid in initramfs): bug but config issue
> > - on vanilla kernel without i2c-hid in initramfs: bug fixed by that
> > patch
> > - on vanilla kernel with i2c-hid in initramfs: works
>
> The detail you're missing here for #4 is vanilla kernel *with Fedora kernel
> config* not with a defconfig. A defconfig kernel I don't expect enables all
> the config options needed for this hardware.
>
> I've reproduced it with my own kernel config as well which is not the Fedora
> or RHEL kernel config but rather a narrowed down config of options that I
> normally use for testing.
>
> >
> > So that patch is needed no matter the config bug we have there and it
> > will also fix that config bug. Am I correct?
>
> Yes there are certainly circumstances it is needed if i2c-hid isn't present
> in the kernel used to restore the hibernation image.
>
> >
> > >
> > > To address this, switch from using the SYSTEM_SLEEP_PM_OPS()
> > > macro to dedicated callbacks, introducing a specific
> > > callback for restoring the S4 image. This callback ensures
> > > the _DSM is invoked.
> >
> > I'm a little bit hesitant to include this patch without proper testing.
> > Did you run this through a wider range of laptops than just the
> > problematic one?
>
> I have been including it in my (local) development and testing tree ever
> since I made it about a month ago so I've thrown it at a random assortment
> of hardware I boot that on. I have also been doing S4 testing explicitly
> because of checking that I didn't break S4 with by S4 at S5 series [1].
>
> That hardware is all biased to be AMD hardware though.
Alright, thanks for the precision.
I'll run on my intel based laptop a few tests then and will include this
upstream.
Cheers,
Benjamin
>
> [1] https://lore.kernel.org/linux-usb/20250909191619.2580169-1-superm1@kernel.org/
> >
> > Other than that, patch looks good to me, but I would have liked to have
> > another opinion on it.
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> > > ---
> > > drivers/hid/i2c-hid/i2c-hid-acpi.c | 8 ++++++++
> > > drivers/hid/i2c-hid/i2c-hid-core.c | 28 +++++++++++++++++++++++++++-
> > > drivers/hid/i2c-hid/i2c-hid.h | 2 ++
> > > 3 files changed, 37 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > > index 1b49243adb16a..abd700a101f46 100644
> > > --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > > +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > > @@ -76,6 +76,13 @@ static int i2c_hid_acpi_get_descriptor(struct i2c_hid_acpi *ihid_acpi)
> > > return hid_descriptor_address;
> > > }
> > > +static void i2c_hid_acpi_restore_sequence(struct i2chid_ops *ops)
> > > +{
> > > + struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
> > > +
> > > + i2c_hid_acpi_get_descriptor(ihid_acpi);
> > > +}
> > > +
> > > static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops)
> > > {
> > > struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
> > > @@ -96,6 +103,7 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
> > > ihid_acpi->adev = ACPI_COMPANION(dev);
> > > ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail;
> > > + ihid_acpi->ops.restore_sequence = i2c_hid_acpi_restore_sequence;
> > > ret = i2c_hid_acpi_get_descriptor(ihid_acpi);
> > > if (ret < 0)
> > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > > index d3912e3f2f13a..3257aa87be898 100644
> > > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > > @@ -961,6 +961,14 @@ static void i2c_hid_core_shutdown_tail(struct i2c_hid *ihid)
> > > ihid->ops->shutdown_tail(ihid->ops);
> > > }
> > > +static void i2c_hid_core_restore_sequence(struct i2c_hid *ihid)
> > > +{
> > > + if (!ihid->ops->restore_sequence)
> > > + return;
> > > +
> > > + ihid->ops->restore_sequence(ihid->ops);
> > > +}
> > > +
> > > static int i2c_hid_core_suspend(struct i2c_hid *ihid, bool force_poweroff)
> > > {
> > > struct i2c_client *client = ihid->client;
> > > @@ -1360,8 +1368,26 @@ static int i2c_hid_core_pm_resume(struct device *dev)
> > > return i2c_hid_core_resume(ihid);
> > > }
> > > +static int i2c_hid_core_pm_restore(struct device *dev)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct i2c_hid *ihid = i2c_get_clientdata(client);
> > > +
> > > + if (ihid->is_panel_follower)
> > > + return 0;
> > > +
> > > + i2c_hid_core_restore_sequence(ihid);
> > > +
> > > + return i2c_hid_core_resume(ihid);
> > > +}
> > > +
> > > const struct dev_pm_ops i2c_hid_core_pm = {
> > > - SYSTEM_SLEEP_PM_OPS(i2c_hid_core_pm_suspend, i2c_hid_core_pm_resume)
> > > + .suspend = pm_sleep_ptr(i2c_hid_core_pm_suspend),
> > > + .resume = pm_sleep_ptr(i2c_hid_core_pm_resume),
> > > + .freeze = pm_sleep_ptr(i2c_hid_core_pm_suspend),
> > > + .thaw = pm_sleep_ptr(i2c_hid_core_pm_resume),
> > > + .poweroff = pm_sleep_ptr(i2c_hid_core_pm_suspend),
> > > + .restore = pm_sleep_ptr(i2c_hid_core_pm_restore),
> > > };
> > > EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
> > > diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
> > > index 2c7b66d5caa0f..1724a435c783a 100644
> > > --- a/drivers/hid/i2c-hid/i2c-hid.h
> > > +++ b/drivers/hid/i2c-hid/i2c-hid.h
> > > @@ -27,11 +27,13 @@ static inline u32 i2c_hid_get_dmi_quirks(const u16 vendor, const u16 product)
> > > * @power_up: do sequencing to power up the device.
> > > * @power_down: do sequencing to power down the device.
> > > * @shutdown_tail: called at the end of shutdown.
> > > + * @restore_sequence: hibernation restore sequence.
> > > */
> > > struct i2chid_ops {
> > > int (*power_up)(struct i2chid_ops *ops);
> > > void (*power_down)(struct i2chid_ops *ops);
> > > void (*shutdown_tail)(struct i2chid_ops *ops);
> > > + void (*restore_sequence)(struct i2chid_ops *ops);
> > > };
> > > int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
> > > --
> > > 2.43.0
> > >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4
2025-09-09 11:00 [PATCH] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4 Mario Limonciello (AMD)
2025-09-11 13:49 ` Benjamin Tissoires
@ 2025-09-16 8:26 ` Benjamin Tissoires
1 sibling, 0 replies; 5+ messages in thread
From: Benjamin Tissoires @ 2025-09-16 8:26 UTC (permalink / raw)
To: mario.limonciello, jikos, Mario Limonciello (AMD); +Cc: linux-input
On Tue, 09 Sep 2025 06:00:06 -0500, Mario Limonciello (AMD) wrote:
> Dell systems utilize an EC-based touchpad emulation when the ACPI
> touchpad _DSM is not invoked. This emulation acts as a secondary
> master on the I2C bus, designed for scenarios where the I2C touchpad
> driver is absent, such as in BIOS menus. Typically, loading the
> i2c-hid module triggers the _DSM at initialization, disabling the
> EC-based emulation.
>
> [...]
Applied to hid/hid.git (for-6.18/i2c-hid), thanks!
[1/1] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4
https://git.kernel.org/hid/hid/c/7d62beb102d6
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-09-16 8:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-09 11:00 [PATCH] HID: i2c-hid: Resolve touchpad issues on Dell systems during S4 Mario Limonciello (AMD)
2025-09-11 13:49 ` Benjamin Tissoires
2025-09-11 13:58 ` Mario Limonciello
2025-09-11 14:55 ` Benjamin Tissoires
2025-09-16 8:26 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox