| Message ID | 2b4c5881-2047-407f-8bb4-623d9bd167c7@www.fastmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | c7aa374e0000daa89201fc29912b60081b269339 |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: multitouch: Set to high latency mode on suspend. | expand |
Hi Blaž, On Sat, Dec 19, 2020 at 5:55 AM Blaž Hrastnik <blaz@mxxn.io> wrote: > > Per Windows Precision Touchpad guidelines: > > > The latency mode feature report is sent by the host to a Windows > > Precision Touchpad to indicate when high latency is desirable for > > power savings and, conversely, when normal latency is desired for > > operation. > > > > For USB-connected Windows Precision Touchpads, this enables the device > > to disambiguate between being suspended for inactivity (runtime IDLE) > > and being suspended because the system is entering S3 or Connected > > Standby. > > The current implementation would set the latency to normal on device initialization, > but we didn't set the device to high latency on suspend. Just a couple of quick questions, do you have any reason to implement this besides just trying to match the specification? Have you checked if Windows is doing the same thing? Cheers, Benjamin > > Signed-off-by: Blaž Hrastnik <blaz@mxxn.io> > --- > drivers/hid/hid-multitouch.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > index d670bcd57..28bac0f39 100644 > --- a/drivers/hid/hid-multitouch.c > +++ b/drivers/hid/hid-multitouch.c > @@ -1746,6 +1746,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) > } > > #ifdef CONFIG_PM > +static int mt_suspend(struct hid_device *hdev, pm_message_t state) > +{ > + /* High latency is desirable for power savings during S3/S0ix */ > + mt_set_modes(hdev, HID_LATENCY_HIGH, true, true); > + return 0; > +} > + > static int mt_reset_resume(struct hid_device *hdev) > { > mt_release_contacts(hdev); > @@ -1761,6 +1768,8 @@ static int mt_resume(struct hid_device *hdev) > > hid_hw_idle(hdev, 0, 0, HID_REQ_SET_IDLE); > > + mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true); > + > return 0; > } > #endif > @@ -2150,6 +2159,7 @@ static struct hid_driver mt_driver = { > .event = mt_event, > .report = mt_report, > #ifdef CONFIG_PM > + .suspend = mt_suspend, > .reset_resume = mt_reset_resume, > .resume = mt_resume, > #endif > -- > 2.29.2 >
> Just a couple of quick questions, do you have any reason to implement > this besides just trying to match the specification? On the Surface Laptop 3 touchpad and the keyboard are provided by a separate embedded controller that also handles various other components. I think setting the latency mode could improve battery efficiency during suspend. The same should be true for touchpads configured as wake-up sources. Blaž On Fri, 15 Jan 2021, at 01:55, Benjamin Tissoires wrote: > Hi Blaž, > > On Sat, Dec 19, 2020 at 5:55 AM Blaž Hrastnik <blaz@mxxn.io> wrote: > > > > Per Windows Precision Touchpad guidelines: > > > > > The latency mode feature report is sent by the host to a Windows > > > Precision Touchpad to indicate when high latency is desirable for > > > power savings and, conversely, when normal latency is desired for > > > operation. > > > > > > For USB-connected Windows Precision Touchpads, this enables the device > > > to disambiguate between being suspended for inactivity (runtime IDLE) > > > and being suspended because the system is entering S3 or Connected > > > Standby. > > > > The current implementation would set the latency to normal on device initialization, > > but we didn't set the device to high latency on suspend. > > Just a couple of quick questions, do you have any reason to implement > this besides just trying to match the specification? > > Have you checked if Windows is doing the same thing? > > Cheers, > Benjamin > > > > > > > Signed-off-by: Blaž Hrastnik <blaz@mxxn.io> > > --- > > drivers/hid/hid-multitouch.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > > index d670bcd57..28bac0f39 100644 > > --- a/drivers/hid/hid-multitouch.c > > +++ b/drivers/hid/hid-multitouch.c > > @@ -1746,6 +1746,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) > > } > > > > #ifdef CONFIG_PM > > +static int mt_suspend(struct hid_device *hdev, pm_message_t state) > > +{ > > + /* High latency is desirable for power savings during S3/S0ix */ > > + mt_set_modes(hdev, HID_LATENCY_HIGH, true, true); > > + return 0; > > +} > > + > > static int mt_reset_resume(struct hid_device *hdev) > > { > > mt_release_contacts(hdev); > > @@ -1761,6 +1768,8 @@ static int mt_resume(struct hid_device *hdev) > > > > hid_hw_idle(hdev, 0, 0, HID_REQ_SET_IDLE); > > > > + mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true); > > + > > return 0; > > } > > #endif > > @@ -2150,6 +2159,7 @@ static struct hid_driver mt_driver = { > > .event = mt_event, > > .report = mt_report, > > #ifdef CONFIG_PM > > + .suspend = mt_suspend, > > .reset_resume = mt_reset_resume, > > .resume = mt_resume, > > #endif > > -- > > 2.29.2 > > > >
On Sat, 19 Dec 2020, Blaž Hrastnik wrote: > Per Windows Precision Touchpad guidelines: > > > The latency mode feature report is sent by the host to a Windows > > Precision Touchpad to indicate when high latency is desirable for > > power savings and, conversely, when normal latency is desired for > > operation. > > > > For USB-connected Windows Precision Touchpads, this enables the device > > to disambiguate between being suspended for inactivity (runtime IDLE) > > and being suspended because the system is entering S3 or Connected > > Standby. > > The current implementation would set the latency to normal on device initialization, > but we didn't set the device to high latency on suspend. Applied, thanks Blaž.
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index d670bcd57..28bac0f39 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -1746,6 +1746,13 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) } #ifdef CONFIG_PM +static int mt_suspend(struct hid_device *hdev, pm_message_t state) +{ + /* High latency is desirable for power savings during S3/S0ix */ + mt_set_modes(hdev, HID_LATENCY_HIGH, true, true); + return 0; +} + static int mt_reset_resume(struct hid_device *hdev) { mt_release_contacts(hdev); @@ -1761,6 +1768,8 @@ static int mt_resume(struct hid_device *hdev) hid_hw_idle(hdev, 0, 0, HID_REQ_SET_IDLE); + mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true); + return 0; } #endif @@ -2150,6 +2159,7 @@ static struct hid_driver mt_driver = { .event = mt_event, .report = mt_report, #ifdef CONFIG_PM + .suspend = mt_suspend, .reset_resume = mt_reset_resume, .resume = mt_resume, #endif