Re: [PATCH v3] HID: i2c-hid: Enable touchpad wakeup from Suspend-to-Idle

[Kai-Heng Feng]

Hi,

> On Jun 18, 2020, at 23:28, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> 
> Hi,
> 
> On 6/18/20 4:55 PM, Kai-Heng Feng wrote:
>> Many laptops can be woken up from Suspend-to-Idle by touchpad. This is
>> also the default behavior on other OSes.
>> So let's enable the wakeup support if the system defaults to
>> Suspend-to-Idle.
> 
> I have been debugging a spurious wakeup issue on an Asus T101HA,
> where the system would not stay suspended when the lid was closed.
> 
> The issue turns out to be that the touchpad is generating touch
> events when the lid/display gets close to the touchpad. In this case
> wakeup is already enabled by default because it is an USB device.

Sounds like a mechanical/hardware issue to me.
I've seen some old laptops have the same issue.

Swollen battery can push up the touchpad, makes it contact to touchscreen, and wakes up the system.

> 
> So I do not believe that this is a good idea, most current devices
> with a HID multi-touch touchpad use i2c-hid and also use S2idle,
> so this will basically enable wakeup by touchpad on almost all
> current devices.

However, it's really handy to wake up the system from touchpad.

> 
> There will likely be other devices with the same issue as the T101HA,
> but currently we are not seeing this issue because by default i2c-hid
> devices do not have wakeup enabled. This change will thus likely cause
> new spurious wakeup issues on more devices. So this seems like a
> bad idea.

But only under lid is closed?

I wonder if it's okay to handle the case in s2idle_loop() or in userspace?
Lid close -> Wakeup event from touchpad -> Found the lid is closed 
-> Turn off touchpad wakeup -> continue suspend.

> 
> Also your commit message mentions touchpads, but the change
> will also enable wakeup on most touchscreens out there, meaning
> that just picking up a device in tablet mode and accidentally
> touching the screen will wake it up.

I tried touch and i2c-hid touchscreen and it doesn't wake up the system.
However we should still handle the two different cases, probably differentiate touchpad and touchscreen in hid-multitouch.

> 
> Also hid multi-touch devices have 3 modes, see the diagrams
> in Microsoft hw design guides for win8/10 touch devices:
> 1. Reporting events with low latency (high power mode)
> 2. Reporting events with high latency (lower power mode)
> 3. Not reporting events (lowest power mode)
> 
> I actually still need to write some patches for hid-multitouch.c
> to set the mode to 2 or 3 on suspend depending on the device_may_wakeup
> setting of the parent. Once that patch is written, it should
> put most i2c-hid mt devices in mode 3, hopefully also helping
> with Linux' relative high power consumption when a device is
> suspended. With your change instead my to-be-written patch
> would put the device in mode 2, which would still be an
> improvement but less so.

IIRC, touchpad and touchscreen connect to different parents on all laptops I worked on.
So I think it's possible to enable mode 2 for touchpad, and mode 3 for touchscreen.

Touchpad wake is really handy, let's figure out how to enable it while covering all potential regression risks.

Kai-Heng

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
>> ---
>> v3:
>>  - Use device_init_wakeup().
>>  - Wording change.
>> v2:
>>  - Fix compile error when ACPI is not enabled.
>>  drivers/hid/i2c-hid/i2c-hid-core.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index 294c84e136d7..dae1d072daf6 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -931,6 +931,12 @@ static void i2c_hid_acpi_fix_up_power(struct device *dev)
>>  		acpi_device_fix_up_power(adev);
>>  }
>>  +static void i2c_hid_acpi_enable_wakeup(struct device *dev)
>> +{
>> +	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
>> +		device_init_wakeup(dev, true);
>> +}
>> +
>>  static const struct acpi_device_id i2c_hid_acpi_match[] = {
>>  	{"ACPI0C50", 0 },
>>  	{"PNP0C50", 0 },
>> @@ -945,6 +951,8 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
>>  }
>>    static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {}
>> +
>> +static inline void i2c_hid_acpi_enable_wakeup(struct device *dev) {}
>>  #endif
>>    #ifdef CONFIG_OF
>> @@ -1072,6 +1080,8 @@ static int i2c_hid_probe(struct i2c_client *client,
>>    	i2c_hid_acpi_fix_up_power(&client->dev);
>>  +	i2c_hid_acpi_enable_wakeup(&client->dev);
>> +
>>  	device_enable_async_suspend(&client->dev);
>>    	/* Make sure there is something at this address */
>