| Message ID | 20190408040721.2824-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: Setting work mode in RT resume on a Synaptics touchpad | expand |
Hi, On Mon, Apr 8, 2019 at 6:07 AM Hui Wang <hui.wang@canonical.com> wrote: > > We disabled the runtime PM for the synaptics touchpad (06cb:7e7e), > then it worked, but after S3, the touchpad didn't work again. > > After more investigation, we found if we don't disable RT PM and only > apply I2C_HID_QUIRK_DELAY_AFTER_SLEEP, we can get the interrupt and > data report from touchpad, but the data report is invalid to the > driver hid-multitouch (this touchpad has HID_GROUP_MULTITOUCH_WIN_8, > so the driver hid-multitouch is loaded), if we rmmod hid-multitouch, > then the hid-generic is the driver and the data report is valid to > this driver, the touchpad work again. > > The data report is valid to hid-generic while is invalid to > hid-multitouch, that is because the hid-multitouch set the specific > mode to touchpad and let it report data in a specific format. > > After we enable runtime PM, the touchpad will be PWR_SLEEP via RT > PM, this will make the working mode lost on this touchpad, then it > will report data in a different format from the hid-multitouch needed. > > Let the driver set the working mode in the runtime resume just like > the system resume does, this can fix this problem. But on this > touchpad, besides the issue of "working mode lost after PWR_SLEEP", it > also has one more issue, it needs to wait for 100 ms between PWR_ON > and setting mode, otherwise the mode can't be set correctly. > > Since both system resume and RT resume will call reset_resume, i put > the related code in a function, no behaviour change for system resume. Thanks for the patch. However, I have the impression we are totally wrong under Linux and i2c-hid. This is yet an other quirk for a driver that should not have any. I definitively not want to maintain an endless list of quirks, but I'd rather mimic what Windows is doing or this is going to never end. I have been told a few times that the Windows driver was much less aggressive than us regarding the POWER command. And from what I remember from the spec, it actually makes no tsense to control runtime PM on the touchpads by sending those POWER commands. I2C is a bus that should not drain any power when not in use. And the touchpad has enough information when to go into low power. So I am going to install Windows on a i2c-hid laptop, and try to see when these commands are sent, and which timing is used (should we delay any command by 100ms after a POWER?). So unless you have such data, which would save me some time, I'll put this patch on hold because this feels really wrong to have to quirk such a device. Cheers, Benjamin > > Fixes: 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad") > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 36 ++++++++++++++++++++++++------ > 1 file changed, 29 insertions(+), 7 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index 4d1f24ee249c..b44d34b3bc96 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -51,6 +51,8 @@ > #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2) > #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3) > #define I2C_HID_QUIRK_BOGUS_IRQ BIT(4) > +#define I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM BIT(5) > +#define I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET BIT(6) > > /* flags */ > #define I2C_HID_STARTED 0 > @@ -185,7 +187,9 @@ static const struct i2c_hid_quirks { > { USB_VENDOR_ID_ELAN, HID_ANY_ID, > I2C_HID_QUIRK_BOGUS_IRQ }, > { USB_VENDOR_ID_SYNAPTICS, I2C_DEVICE_ID_SYNAPTICS_7E7E, > - I2C_HID_QUIRK_NO_RUNTIME_PM }, > + I2C_HID_QUIRK_DELAY_AFTER_SLEEP | > + I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM | > + I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET }, > { 0, 0 } > }; > > @@ -1214,6 +1218,24 @@ static void i2c_hid_shutdown(struct i2c_client *client) > } > > #ifdef CONFIG_PM_SLEEP > +static int i2c_hid_call_reset_resume(struct i2c_hid *ihid) > +{ > + struct hid_device *hid = ihid->hid; > + > + if (hid->driver && hid->driver->reset_resume) { > + /* > + * On some touchpads like synaptics touchpad (06cb:7e7e), after > + * it is powered on, needs to wait for 100 ms, then set the mode > + * data to it, otherwise the data can't be set correctly. > + */ > + if (ihid->quirks & I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET) > + msleep(100); > + return hid->driver->reset_resume(hid); > + } > + > + return 0; > +} > + > static int i2c_hid_suspend(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > @@ -1299,12 +1321,7 @@ static int i2c_hid_resume(struct device *dev) > if (ret) > return ret; > > - if (hid->driver && hid->driver->reset_resume) { > - ret = hid->driver->reset_resume(hid); > - return ret; > - } > - > - return 0; > + return i2c_hid_call_reset_resume(ihid); > } > #endif > > @@ -1321,9 +1338,14 @@ static int i2c_hid_runtime_suspend(struct device *dev) > static int i2c_hid_runtime_resume(struct device *dev) > { > struct i2c_client *client = to_i2c_client(dev); > + struct i2c_hid *ihid = i2c_get_clientdata(client); > > enable_irq(client->irq); > i2c_hid_set_power(client, I2C_HID_PWR_ON); > + > + if (ihid->quirks & I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM) > + return i2c_hid_call_reset_resume(ihid); > + > return 0; > } > #endif > -- > 2.17.1 >
On 2019/4/8 下午11:44, Benjamin Tissoires wrote: > Hi, > > On Mon, Apr 8, 2019 at 6:07 AM Hui Wang <hui.wang@canonical.com> wrote: >> We disabled the runtime PM for the synaptics touchpad (06cb:7e7e), >> then it worked, but after S3, the touchpad didn't work again. >> >> After more investigation, we found if we don't disable RT PM and only >> apply I2C_HID_QUIRK_DELAY_AFTER_SLEEP, we can get the interrupt and >> data report from touchpad, but the data report is invalid to the >> driver hid-multitouch (this touchpad has HID_GROUP_MULTITOUCH_WIN_8, >> so the driver hid-multitouch is loaded), if we rmmod hid-multitouch, >> then the hid-generic is the driver and the data report is valid to >> this driver, the touchpad work again. >> >> The data report is valid to hid-generic while is invalid to >> hid-multitouch, that is because the hid-multitouch set the specific >> mode to touchpad and let it report data in a specific format. >> >> After we enable runtime PM, the touchpad will be PWR_SLEEP via RT >> PM, this will make the working mode lost on this touchpad, then it >> will report data in a different format from the hid-multitouch needed. >> >> Let the driver set the working mode in the runtime resume just like >> the system resume does, this can fix this problem. But on this >> touchpad, besides the issue of "working mode lost after PWR_SLEEP", it >> also has one more issue, it needs to wait for 100 ms between PWR_ON >> and setting mode, otherwise the mode can't be set correctly. >> >> Since both system resume and RT resume will call reset_resume, i put >> the related code in a function, no behaviour change for system resume. Thanks for your comment, today I discussed with Kai-Heng Feng who is the author of commit 77ae0d8e401f ("HID: i2c-hid: Disable runtime PM on Goodix touchpad"), it looks like I applied the quirk to the wrong vid:pid, the synaptics touchpad (06cb:7e7e) has no any problem with the existing i2c-hid driver, and we should revert 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad"). It looks like Hai-Heng and I worked on the same hardware touchpad, but he got the id (27c6:01f0) while I got the id (06cb:7e7e) This patch should be applied to Goodix touchpad (27c6:01f0) if the touchpad on Kai-Heng's machine doesn't work after s3. I need to verify if that touchpad still work or not after s3, but I guess it will not work since in the system resume it set the touchpad to be I2C_HID_PWR_ON, then immediately it set the working mode via hid->driver->reset_resume(hid), there is no delay between POW_ON and setting work mode. At least on my machine, if there is no delay, the working mode can't be set successfully and hid-multitouch can't parse the data report. I set the delay to 100ms based on my testing result. On my machine, I originally set the delay to 10/20/30/40ms but the touchpad can't report the data that hid-multitouch needed, After I set the delay to 50ms, the driver works occasionally. Then to be safe, I set the delay to 100ms in the patch. And this afternoon, I found this comment from Goodix " Our IC needs about 60 ms to resume from sleep state, so we hope system can extend the command delay time to avoid potential risk." > Thanks for the patch. > > However, I have the impression we are totally wrong under Linux and > i2c-hid. This is yet an other quirk for a driver that should not have > any. > I definitively not want to maintain an endless list of quirks, but I'd > rather mimic what Windows is doing or this is going to never end. > > I have been told a few times that the Windows driver was much less > aggressive than us regarding the POWER command. And from what I > remember from the spec, it actually makes no tsense to control runtime > PM on the touchpads by sending those POWER commands. I2C is a bus that > should not drain any power when not in use. And the touchpad has > enough information when to go into low power. > > So I am going to install Windows on a i2c-hid laptop, and try to see > when these commands are sent, and which timing is used (should we > delay any command by 100ms after a POWER?). I have two different laptops with the same touchpad hardware, one is installed Linux, the touchpad can't work unless we apply this patch or disable runtime in the i2c-hid-core.c, the other is installed windows10, the touchapd can't work too after installation, after a upgrade online, the touchpad can work. I am willing to test commands and timing under windows, but I don't know how to do that, is there a guide? > So unless you have such data, which would save me some time, I'll put > this patch on hold because this feels really wrong to have to quirk > such a device. After I confirm that the touchpad Goodix (27c6:01f0) doesn't work after S3, I will send a revert patch for 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad") and a new patch to improve 77ae0d8e401f ("HID: i2c-hid: Disable runtime PM on Goodix touchpad"). Thanks, Hui. > Cheers, > Benjamin > >> Fixes: 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad") >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> drivers/hid/i2c-hid/i2c-hid-core.c | 36 ++++++++++++++++++++++++------ >> 1 file changed, 29 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c >> index 4d1f24ee249c..b44d34b3bc96 100644 >> --- a/drivers/hid/i2c-hid/i2c-hid-core.c >> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c >> @@ -51,6 +51,8 @@ >> #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2) >> #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3) >> #define I2C_HID_QUIRK_BOGUS_IRQ BIT(4) >> +#define I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM BIT(5) >> +#define I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET BIT(6) >> >> /* flags */ >> #define I2C_HID_STARTED 0 >> @@ -185,7 +187,9 @@ static const struct i2c_hid_quirks { >> { USB_VENDOR_ID_ELAN, HID_ANY_ID, >> I2C_HID_QUIRK_BOGUS_IRQ }, >> { USB_VENDOR_ID_SYNAPTICS, I2C_DEVICE_ID_SYNAPTICS_7E7E, >> - I2C_HID_QUIRK_NO_RUNTIME_PM }, >> + I2C_HID_QUIRK_DELAY_AFTER_SLEEP | >> + I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM | >> + I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET }, >> { 0, 0 } >> }; >> >> @@ -1214,6 +1218,24 @@ static void i2c_hid_shutdown(struct i2c_client *client) >> } >> >> #ifdef CONFIG_PM_SLEEP >> +static int i2c_hid_call_reset_resume(struct i2c_hid *ihid) >> +{ >> + struct hid_device *hid = ihid->hid; >> + >> + if (hid->driver && hid->driver->reset_resume) { >> + /* >> + * On some touchpads like synaptics touchpad (06cb:7e7e), after >> + * it is powered on, needs to wait for 100 ms, then set the mode >> + * data to it, otherwise the data can't be set correctly. >> + */ >> + if (ihid->quirks & I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET) >> + msleep(100); >> + return hid->driver->reset_resume(hid); >> + } >> + >> + return 0; >> +} >> + >> static int i2c_hid_suspend(struct device *dev) >> { >> struct i2c_client *client = to_i2c_client(dev); >> @@ -1299,12 +1321,7 @@ static int i2c_hid_resume(struct device *dev) >> if (ret) >> return ret; >> >> - if (hid->driver && hid->driver->reset_resume) { >> - ret = hid->driver->reset_resume(hid); >> - return ret; >> - } >> - >> - return 0; >> + return i2c_hid_call_reset_resume(ihid); >> } >> #endif >> >> @@ -1321,9 +1338,14 @@ static int i2c_hid_runtime_suspend(struct device *dev) >> static int i2c_hid_runtime_resume(struct device *dev) >> { >> struct i2c_client *client = to_i2c_client(dev); >> + struct i2c_hid *ihid = i2c_get_clientdata(client); >> >> enable_irq(client->irq); >> i2c_hid_set_power(client, I2C_HID_PWR_ON); >> + >> + if (ihid->quirks & I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM) >> + return i2c_hid_call_reset_resume(ihid); >> + >> return 0; >> } >> #endif >> -- >> 2.17.1 >>
On Tue, Apr 9, 2019 at 3:32 PM hwang4 <hui.wang@canonical.com> wrote: > > > On 2019/4/8 下午11:44, Benjamin Tissoires wrote: > > Hi, > > > > On Mon, Apr 8, 2019 at 6:07 AM Hui Wang <hui.wang@canonical.com> wrote: > >> We disabled the runtime PM for the synaptics touchpad (06cb:7e7e), > >> then it worked, but after S3, the touchpad didn't work again. > >> > >> After more investigation, we found if we don't disable RT PM and only > >> apply I2C_HID_QUIRK_DELAY_AFTER_SLEEP, we can get the interrupt and > >> data report from touchpad, but the data report is invalid to the > >> driver hid-multitouch (this touchpad has HID_GROUP_MULTITOUCH_WIN_8, > >> so the driver hid-multitouch is loaded), if we rmmod hid-multitouch, > >> then the hid-generic is the driver and the data report is valid to > >> this driver, the touchpad work again. > >> > >> The data report is valid to hid-generic while is invalid to > >> hid-multitouch, that is because the hid-multitouch set the specific > >> mode to touchpad and let it report data in a specific format. > >> > >> After we enable runtime PM, the touchpad will be PWR_SLEEP via RT > >> PM, this will make the working mode lost on this touchpad, then it > >> will report data in a different format from the hid-multitouch needed. > >> > >> Let the driver set the working mode in the runtime resume just like > >> the system resume does, this can fix this problem. But on this > >> touchpad, besides the issue of "working mode lost after PWR_SLEEP", it > >> also has one more issue, it needs to wait for 100 ms between PWR_ON > >> and setting mode, otherwise the mode can't be set correctly. > >> > >> Since both system resume and RT resume will call reset_resume, i put > >> the related code in a function, no behaviour change for system resume. > > Thanks for your comment, today I discussed with Kai-Heng Feng who is the > author of commit 77ae0d8e401f ("HID: i2c-hid: Disable runtime PM on > Goodix touchpad"), it looks like I applied the quirk to the wrong > vid:pid, the synaptics touchpad (06cb:7e7e) has no any problem with the > existing i2c-hid driver, and we should revert 74e7c6c877f6 ("HID: > i2c-hid: Disable runtime PM on Synaptics touchpad"). It looks like > Hai-Heng and I worked on the same hardware touchpad, but he got the id > (27c6:01f0) while I got the id (06cb:7e7e) Not sure I followed everything. Is the following correct? - Synaptics 06cb:7e7e is working fine without the runtime PM patch (74e7c6c877f6) and has no issues with suspend/resume - Goodix 27c6:01f0 is the one having trouble with runtime PM *and* suspend resume > > This patch should be applied to Goodix touchpad (27c6:01f0) if the > touchpad on Kai-Heng's machine doesn't work after s3. I need to verify > if that touchpad still work or not after s3, but I guess it will not > work since in the system resume it set the touchpad to be > I2C_HID_PWR_ON, then immediately it set the working mode via > hid->driver->reset_resume(hid), there is no delay between POW_ON and > setting work mode. At least on my machine, if there is no delay, the > working mode can't be set successfully and hid-multitouch can't parse > the data report. > > I set the delay to 100ms based on my testing result. On my machine, I > originally set the delay to 10/20/30/40ms but the touchpad can't report > the data that hid-multitouch needed, After I set the delay to 50ms, the > driver works occasionally. Then to be safe, I set the delay to 100ms in > the patch. And this afternoon, I found this comment from Goodix " Our IC > needs about 60 ms to resume from sleep state, so we hope system can > extend the command delay time to avoid potential risk." So if I understand correctly, the Goodix touchpad needs: - to wait 60ms after a SET_POWER on - to switch back to multitouch mode after a SET_POWER sleep and back on > > > Thanks for the patch. > > > > However, I have the impression we are totally wrong under Linux and > > i2c-hid. This is yet an other quirk for a driver that should not have > > any. > > I definitively not want to maintain an endless list of quirks, but I'd > > rather mimic what Windows is doing or this is going to never end. > > > > I have been told a few times that the Windows driver was much less > > aggressive than us regarding the POWER command. And from what I > > remember from the spec, it actually makes no tsense to control runtime > > PM on the touchpads by sending those POWER commands. I2C is a bus that > > should not drain any power when not in use. And the touchpad has > > enough information when to go into low power. > > > > So I am going to install Windows on a i2c-hid laptop, and try to see > > when these commands are sent, and which timing is used (should we > > delay any command by 100ms after a POWER?). > I have two different laptops with the same touchpad hardware, one is Are those 2 Synaptics or Goodix? > installed Linux, the touchpad can't work unless we apply this patch or > disable runtime in the i2c-hid-core.c, the other is installed windows10, > the touchapd can't work too after installation, after a upgrade online, What do you mean by "after a upgrade online, the touchpad can work"? > the touchpad can work. I am willing to test commands and timing under > windows, but I don't know how to do that, is there a guide? I am using https://github.com/bentiss/SimplePeripheralBusProbe It's quite complex to set up, but it's better than having to crack open the laptop and put physical probes in it. So on my side, I have been checking on a Dell XPS 13 how Windows handled the touchpad. In term of features retrievals, we are similar to what Windows does. There is a significant difference in how many times we switch the device on and off during probe. Windows basically switches it on once, and done. Under Linux we tend to switch it off and on quite a lot, and this is what I must have been told. This has a tendency to confuse some hardware, so we better fix that. In terms of timing, I do not see any special sleep after a wake up. So I guess this must be a goodix limitation. Last, on S3 suspend of the touchpad, I see: set feature latency low SET_POWER sleep then on resume: SET_POWER on set feature latency normal set feature latency normal set feature button on, surface on I have an almost similar sequence on runtime pm (screen going black): suspend: SET_POWER sleep then on resume: SET_POWER on set feature latency normal set feature latency normal set feature button on, surface on The only difference is that when the system is in S3, the touchpad is set in the low latency mode. > > So unless you have such data, which would save me some time, I'll put > > this patch on hold because this feels really wrong to have to quirk > > such a device. > > After I confirm that the touchpad Goodix (27c6:01f0) doesn't work after > S3, I will send a revert patch for 74e7c6c877f6 ("HID: i2c-hid: Disable > runtime PM on Synaptics touchpad") and a new patch to improve > 77ae0d8e401f ("HID: i2c-hid: Disable runtime PM on Goodix touchpad"). > Thanks for that. Cheers, Benjamin
On 2019/4/9 下午11:42, Benjamin Tissoires wrote: > On Tue, Apr 9, 2019 at 3:32 PM hwang4 <hui.wang@canonical.com> wrote: >> >> On 2019/4/8 下午11:44, Benjamin Tissoires wrote: >>> Hi, >>> >>> On Mon, Apr 8, 2019 at 6:07 AM Hui Wang <hui.wang@canonical.com> wrote: >>>> We disabled the runtime PM for the synaptics touchpad (06cb:7e7e), >>>> then it worked, but after S3, the touchpad didn't work again. >>>> >>>> After more investigation, we found if we don't disable RT PM and only >>>> apply I2C_HID_QUIRK_DELAY_AFTER_SLEEP, we can get the interrupt and >>>> data report from touchpad, but the data report is invalid to the >>>> driver hid-multitouch (this touchpad has HID_GROUP_MULTITOUCH_WIN_8, >>>> so the driver hid-multitouch is loaded), if we rmmod hid-multitouch, >>>> then the hid-generic is the driver and the data report is valid to >>>> this driver, the touchpad work again. >>>> >>>> The data report is valid to hid-generic while is invalid to >>>> hid-multitouch, that is because the hid-multitouch set the specific >>>> mode to touchpad and let it report data in a specific format. >>>> >>>> After we enable runtime PM, the touchpad will be PWR_SLEEP via RT >>>> PM, this will make the working mode lost on this touchpad, then it >>>> will report data in a different format from the hid-multitouch needed. >>>> >>>> Let the driver set the working mode in the runtime resume just like >>>> the system resume does, this can fix this problem. But on this >>>> touchpad, besides the issue of "working mode lost after PWR_SLEEP", it >>>> also has one more issue, it needs to wait for 100 ms between PWR_ON >>>> and setting mode, otherwise the mode can't be set correctly. >>>> >>>> Since both system resume and RT resume will call reset_resume, i put >>>> the related code in a function, no behaviour change for system resume. >> Thanks for your comment, today I discussed with Kai-Heng Feng who is the >> author of commit 77ae0d8e401f ("HID: i2c-hid: Disable runtime PM on >> Goodix touchpad"), it looks like I applied the quirk to the wrong >> vid:pid, the synaptics touchpad (06cb:7e7e) has no any problem with the >> existing i2c-hid driver, and we should revert 74e7c6c877f6 ("HID: >> i2c-hid: Disable runtime PM on Synaptics touchpad"). It looks like >> Hai-Heng and I worked on the same hardware touchpad, but he got the id >> (27c6:01f0) while I got the id (06cb:7e7e) > Not sure I followed everything. Is the following correct? > - Synaptics 06cb:7e7e is working fine without the runtime PM patch > (74e7c6c877f6) and has no issues with suspend/resume Correct. > - Goodix 27c6:01f0 is the one having trouble with runtime PM *and* > suspend resume It has the trouble with runtime PM, and I thought it also has the trouble with suspend and resume, but today I tested the touchpad (27c6:01f0), it has no problem with suspend and resume. So it only has the trouble with runtime PM. And today I got the whole story, The touchpad (06cb:7e7e) on my two laptops are engineering samples, in fact they are not synaptics touchpads, we can call them Gen-1 (generation one) samples, they have trouble with runtime PM and suspend/resume (" Our IC needs about 60 ms to resume from sleep state, so we hope system can extend the command delay time to avoid potential risk."). The touchpad (27c6:01f0) is gen-2 sample, it reports different vid:pid from gen-1, and for the gen-2 sample, it has no 60ms delay requirement, so gen-2 only needs NO_RUNTIME_PM quirk and since gen-1 is not real synaptics touchpad and gen-1 is replaced by gen-2 already, we need to revert the commit 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad") >> This patch should be applied to Goodix touchpad (27c6:01f0) if the >> touchpad on Kai-Heng's machine doesn't work after s3. I need to verify >> if that touchpad still work or not after s3, but I guess it will not >> work since in the system resume it set the touchpad to be >> I2C_HID_PWR_ON, then immediately it set the working mode via >> hid->driver->reset_resume(hid), there is no delay between POW_ON and >> setting work mode. At least on my machine, if there is no delay, the >> working mode can't be set successfully and hid-multitouch can't parse >> the data report. >> >> I set the delay to 100ms based on my testing result. On my machine, I >> originally set the delay to 10/20/30/40ms but the touchpad can't report >> the data that hid-multitouch needed, After I set the delay to 50ms, the >> driver works occasionally. Then to be safe, I set the delay to 100ms in >> the patch. And this afternoon, I found this comment from Goodix " Our IC >> needs about 60 ms to resume from sleep state, so we hope system can >> extend the command delay time to avoid potential risk." > So if I understand correctly, the Goodix touchpad needs: > - to wait 60ms after a SET_POWER on No need for gen-2. I tested it today. > - to switch back to multitouch mode after a SET_POWER sleep and back on Needed for gen-2, but disable RUNTIME_PM can workaround it. >>> Thanks for the patch. >>> >>> However, I have the impression we are totally wrong under Linux and >>> i2c-hid. This is yet an other quirk for a driver that should not have >>> any. >>> I definitively not want to maintain an endless list of quirks, but I'd >>> rather mimic what Windows is doing or this is going to never end. >>> >>> I have been told a few times that the Windows driver was much less >>> aggressive than us regarding the POWER command. And from what I >>> remember from the spec, it actually makes no tsense to control runtime >>> PM on the touchpads by sending those POWER commands. I2C is a bus that >>> should not drain any power when not in use. And the touchpad has >>> enough information when to go into low power. >>> >>> So I am going to install Windows on a i2c-hid laptop, and try to see >>> when these commands are sent, and which timing is used (should we >>> delay any command by 100ms after a POWER?). >> I have two different laptops with the same touchpad hardware, one is > Are those 2 Synaptics or Goodix? All of them are Gen-1 samples, they are Goodix samples but report Synaptics id. > >> installed Linux, the touchpad can't work unless we apply this patch or >> disable runtime in the i2c-hid-core.c, the other is installed windows10, >> the touchapd can't work too after installation, after a upgrade online, > What do you mean by "after a upgrade online, the touchpad can work"? Windows10 upgrade, it will search and install the drivers for hardware. After installing the touchpad driver from the internet, the touchpad can work. > >> the touchpad can work. I am willing to test commands and timing under >> windows, but I don't know how to do that, is there a guide? > I am using https://github.com/bentiss/SimplePeripheralBusProbe > It's quite complex to set up, but it's better than having to crack > open the laptop and put physical probes in it. OK, I will try to setup it and catch the commands on my gen-1 touchpad. > > So on my side, I have been checking on a Dell XPS 13 how Windows > handled the touchpad. > In term of features retrievals, we are similar to what Windows does. > > There is a significant difference in how many times we switch the > device on and off during probe. > Windows basically switches it on once, and done. > Under Linux we tend to switch it off and on quite a lot, and this is > what I must have been told. This has a tendency to confuse some > hardware, so we better fix that. Yes, I also observed many times of SLEEP and ON under Linux. > > In terms of timing, I do not see any special sleep after a wake up. So > I guess this must be a goodix limitation. > > Last, on S3 suspend of the touchpad, I see: > set feature latency low > SET_POWER sleep > > then on resume: > SET_POWER on > set feature latency normal > set feature latency normal > set feature button on, surface on > > I have an almost similar sequence on runtime pm (screen going black): > suspend: > SET_POWER sleep > > then on resume: > SET_POWER on > set feature latency normal > set feature latency normal > set feature button on, surface on > > The only difference is that when the system is in S3, the touchpad is > set in the low latency mode. Are they got from Linux? if "set feature xxxx" are from hid->driver->reset_resume(), the existing runtime resume in linux doesn't call "set feature xxx" at all, in current i2c-hid-core.c of linux, the runtime resume is different from system resume, it doesn't call hid->driver->reset_resume(). > >>> So unless you have such data, which would save me some time, I'll put >>> this patch on hold because this feels really wrong to have to quirk >>> such a device. >> After I confirm that the touchpad Goodix (27c6:01f0) doesn't work after >> S3, I will send a revert patch for 74e7c6c877f6 ("HID: i2c-hid: Disable >> runtime PM on Synaptics touchpad") and a new patch to improve >> 77ae0d8e401f ("HID: i2c-hid: Disable runtime PM on Goodix touchpad"). >> > Thanks for that. > > Cheers, > Benjamin >
On Wed, Apr 10, 2019 at 1:43 PM hwang4 <hui.wang@canonical.com> wrote: > > > On 2019/4/9 下午11:42, Benjamin Tissoires wrote: > > On Tue, Apr 9, 2019 at 3:32 PM hwang4 <hui.wang@canonical.com> wrote: > >> > >> On 2019/4/8 下午11:44, Benjamin Tissoires wrote: > >>> Hi, > >>> > >>> On Mon, Apr 8, 2019 at 6:07 AM Hui Wang <hui.wang@canonical.com> wrote: > >>>> We disabled the runtime PM for the synaptics touchpad (06cb:7e7e), > >>>> then it worked, but after S3, the touchpad didn't work again. > >>>> > >>>> After more investigation, we found if we don't disable RT PM and only > >>>> apply I2C_HID_QUIRK_DELAY_AFTER_SLEEP, we can get the interrupt and > >>>> data report from touchpad, but the data report is invalid to the > >>>> driver hid-multitouch (this touchpad has HID_GROUP_MULTITOUCH_WIN_8, > >>>> so the driver hid-multitouch is loaded), if we rmmod hid-multitouch, > >>>> then the hid-generic is the driver and the data report is valid to > >>>> this driver, the touchpad work again. > >>>> > >>>> The data report is valid to hid-generic while is invalid to > >>>> hid-multitouch, that is because the hid-multitouch set the specific > >>>> mode to touchpad and let it report data in a specific format. > >>>> > >>>> After we enable runtime PM, the touchpad will be PWR_SLEEP via RT > >>>> PM, this will make the working mode lost on this touchpad, then it > >>>> will report data in a different format from the hid-multitouch needed. > >>>> > >>>> Let the driver set the working mode in the runtime resume just like > >>>> the system resume does, this can fix this problem. But on this > >>>> touchpad, besides the issue of "working mode lost after PWR_SLEEP", it > >>>> also has one more issue, it needs to wait for 100 ms between PWR_ON > >>>> and setting mode, otherwise the mode can't be set correctly. > >>>> > >>>> Since both system resume and RT resume will call reset_resume, i put > >>>> the related code in a function, no behaviour change for system resume. > >> Thanks for your comment, today I discussed with Kai-Heng Feng who is the > >> author of commit 77ae0d8e401f ("HID: i2c-hid: Disable runtime PM on > >> Goodix touchpad"), it looks like I applied the quirk to the wrong > >> vid:pid, the synaptics touchpad (06cb:7e7e) has no any problem with the > >> existing i2c-hid driver, and we should revert 74e7c6c877f6 ("HID: > >> i2c-hid: Disable runtime PM on Synaptics touchpad"). It looks like > >> Hai-Heng and I worked on the same hardware touchpad, but he got the id > >> (27c6:01f0) while I got the id (06cb:7e7e) > > Not sure I followed everything. Is the following correct? > > - Synaptics 06cb:7e7e is working fine without the runtime PM patch > > (74e7c6c877f6) and has no issues with suspend/resume > Correct. > > - Goodix 27c6:01f0 is the one having trouble with runtime PM *and* > > suspend resume > > It has the trouble with runtime PM, and I thought it also has the > trouble with suspend and resume, but today I tested the touchpad > (27c6:01f0), it has no problem with suspend and resume. So it only has > the trouble with runtime PM. > > And today I got the whole story, The touchpad (06cb:7e7e) on my two > laptops are engineering samples, in fact they are not synaptics > touchpads, we can call them Gen-1 (generation one) samples, they have > trouble with runtime PM and suspend/resume (" Our IC needs about 60 ms > to resume from sleep state, so we hope system can extend the command > delay time to avoid potential risk."). The touchpad (27c6:01f0) is > gen-2 sample, it reports different vid:pid from gen-1, and for the > gen-2 sample, it has no 60ms delay requirement, so gen-2 only needs > NO_RUNTIME_PM quirk and since gen-1 is not real synaptics touchpad and > gen-1 is replaced by gen-2 already, we need to revert the commit > 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad") > OK, great. Thanks for the clarification. So if gen-1 were only engineering samples, do we need to fix them at all in the kernel? > > >> This patch should be applied to Goodix touchpad (27c6:01f0) if the > >> touchpad on Kai-Heng's machine doesn't work after s3. I need to verify > >> if that touchpad still work or not after s3, but I guess it will not > >> work since in the system resume it set the touchpad to be > >> I2C_HID_PWR_ON, then immediately it set the working mode via > >> hid->driver->reset_resume(hid), there is no delay between POW_ON and > >> setting work mode. At least on my machine, if there is no delay, the > >> working mode can't be set successfully and hid-multitouch can't parse > >> the data report. > >> > >> I set the delay to 100ms based on my testing result. On my machine, I > >> originally set the delay to 10/20/30/40ms but the touchpad can't report > >> the data that hid-multitouch needed, After I set the delay to 50ms, the > >> driver works occasionally. Then to be safe, I set the delay to 100ms in > >> the patch. And this afternoon, I found this comment from Goodix " Our IC > >> needs about 60 ms to resume from sleep state, so we hope system can > >> extend the command delay time to avoid potential risk." > > So if I understand correctly, the Goodix touchpad needs: > > - to wait 60ms after a SET_POWER on > No need for gen-2. I tested it today. > > - to switch back to multitouch mode after a SET_POWER sleep and back on > Needed for gen-2, but disable RUNTIME_PM can workaround it. > >>> Thanks for the patch. > >>> > >>> However, I have the impression we are totally wrong under Linux and > >>> i2c-hid. This is yet an other quirk for a driver that should not have > >>> any. > >>> I definitively not want to maintain an endless list of quirks, but I'd > >>> rather mimic what Windows is doing or this is going to never end. > >>> > >>> I have been told a few times that the Windows driver was much less > >>> aggressive than us regarding the POWER command. And from what I > >>> remember from the spec, it actually makes no tsense to control runtime > >>> PM on the touchpads by sending those POWER commands. I2C is a bus that > >>> should not drain any power when not in use. And the touchpad has > >>> enough information when to go into low power. > >>> > >>> So I am going to install Windows on a i2c-hid laptop, and try to see > >>> when these commands are sent, and which timing is used (should we > >>> delay any command by 100ms after a POWER?). > >> I have two different laptops with the same touchpad hardware, one is > > Are those 2 Synaptics or Goodix? > All of them are Gen-1 samples, they are Goodix samples but report > Synaptics id. > > > >> installed Linux, the touchpad can't work unless we apply this patch or > >> disable runtime in the i2c-hid-core.c, the other is installed windows10, > >> the touchapd can't work too after installation, after a upgrade online, > > What do you mean by "after a upgrade online, the touchpad can work"? > Windows10 upgrade, it will search and install the drivers for hardware. > After installing the touchpad driver from the internet, the touchpad can > work. > > > >> the touchpad can work. I am willing to test commands and timing under > >> windows, but I don't know how to do that, is there a guide? > > I am using https://github.com/bentiss/SimplePeripheralBusProbe > > It's quite complex to set up, but it's better than having to crack > > open the laptop and put physical probes in it. > OK, I will try to setup it and catch the commands on my gen-1 touchpad. Thanks. It could be interesting to see how Windows behave with it. Feel free to ping me if you have trouble recompiling your DSDT. > > > > So on my side, I have been checking on a Dell XPS 13 how Windows > > handled the touchpad. > > In term of features retrievals, we are similar to what Windows does. > > > > There is a significant difference in how many times we switch the > > device on and off during probe. > > Windows basically switches it on once, and done. > > Under Linux we tend to switch it off and on quite a lot, and this is > > what I must have been told. This has a tendency to confuse some > > hardware, so we better fix that. > Yes, I also observed many times of SLEEP and ON under Linux. So, for that, I am thinking that maybe we should delay the last `pm_runtime_put()` we do in `i2c_hid_probe()` by, for instance 5 or 10 secs. So for 5 seconds, we keep the device powered on, which should give time to udev and the session to open the node, get its information, close it and the session to get over it. It shouldn't be hard to write, and might be save us some headaches. Also, once this is in, I think we can consider the use case of opening/closing the device repeatedly as niche case and probably force I2C_HID_QUIRK_DELAY_AFTER_SLEEP on all devices. > > > > In terms of timing, I do not see any special sleep after a wake up. So > > I guess this must be a goodix limitation. > > > > Last, on S3 suspend of the touchpad, I see: > > set feature latency low > > SET_POWER sleep > > > > then on resume: > > SET_POWER on > > set feature latency normal > > set feature latency normal > > set feature button on, surface on > > > > I have an almost similar sequence on runtime pm (screen going black): > > suspend: > > SET_POWER sleep > > > > then on resume: > > SET_POWER on > > set feature latency normal > > set feature latency normal > > set feature button on, surface on > > > > The only difference is that when the system is in S3, the touchpad is > > set in the low latency mode. > Are they got from Linux? if "set feature xxxx" are from These traces were taken from the Windows XPS I instrumented. > hid->driver->reset_resume(), the existing runtime resume in linux > doesn't call "set feature xxx" at all, in current i2c-hid-core.c of > linux, the runtime resume is different from system resume, it doesn't > call hid->driver->reset_resume(). Yep. I am still pondering that. It feels weird to force the call of runtime PM from i2c-hid. Can't we set up the runtime PM calls from within hid-multitouch directly? Having the transport level calling those manually seems weird. Cheers, Benjamin
On 2019/4/10 下午9:03, Benjamin Tissoires wrote: > On Wed, Apr 10, 2019 at 1:43 PM hwang4<hui.wang@canonical.com> wrote: >> On 2019/4/9 下午11:42, Benjamin Tissoires wrote: >>> On Tue, Apr 9, 2019 at 3:32 PM hwang4<hui.wang@canonical.com> wrote: >>>> On 2019/4/8 下午11:44, Benjamin Tissoires wrote: >>>>> Hi, >>>>> >>>>> On Mon, Apr 8, 2019 at 6:07 AM Hui Wang<hui.wang@canonical.com> wrote: >>>>>> We disabled the runtime PM for the synaptics touchpad (06cb:7e7e), >>>>>> then it worked, but after S3, the touchpad didn't work again. >>>>>> >>>>>> >>>>>> gen-1 is replaced by gen-2 already, we need to revert the commit >>>>>> 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad") >>>>>> > OK, great. Thanks for the clarification. > > So if gen-1 were only engineering samples, do we need to fix them at > all in the kernel? > No need to fix gen-1 in the kernel, I will send a revert patch for 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad"). >>>> This patch should be applied to Goodix touchpad (27c6:01f0) if the >>>> touchpad on Kai-Heng's machine doesn't work after s3. I need to verify >>>> if that touchpad still work or not after s3, but I guess it will not >>>> work since in the system resume it set the touchpad to be >>>> I2C_HID_PWR_ON, then immediately it set the working mode via >>>> hid->driver->reset_resume(hid), there is no delay between POW_ON and >>>> setting work mode. At least on my machine, if there is no delay, the >>>> working mode can't be set successfully and hid-multitouch can't parse >>>> the data report. >>>> >>>> I set the delay to 100ms based on my testing result. On my machine, I >>>> originally set the delay to 10/20/30/40ms but the touchpad can't report >>>> the data that hid-multitouch needed, After I set the delay to 50ms, the >>>> driver works occasionally. Then to be safe, I set the delay to 100ms in >>>> the patch. And this afternoon, I found this comment from Goodix " Our IC >>>> needs about 60 ms to resume from sleep state, so we hope system can >>>> extend the command delay time to avoid potential risk." >>> So if I understand correctly, the Goodix touchpad needs: >>> - to wait 60ms after a SET_POWER on >> No need for gen-2. I tested it today. >>> - to switch back to multitouch mode after a SET_POWER sleep and back on >> Needed for gen-2, but disable RUNTIME_PM can workaround it. >>>>> Thanks for the patch. >>>>> >>>>> However, I have the impression we are totally wrong under Linux and >>>>> i2c-hid. This is yet an other quirk for a driver that should not have >>>>> any. >>>>> I definitively not want to maintain an endless list of quirks, but I'd >>>>> rather mimic what Windows is doing or this is going to never end. >>>>> >>>>> I have been told a few times that the Windows driver was much less >>>>> aggressive than us regarding the POWER command. And from what I >>>>> remember from the spec, it actually makes no tsense to control runtime >>>>> PM on the touchpads by sending those POWER commands. I2C is a bus that >>>>> should not drain any power when not in use. And the touchpad has >>>>> enough information when to go into low power. >>>>> >>>>> So I am going to install Windows on a i2c-hid laptop, and try to see >>>>> when these commands are sent, and which timing is used (should we >>>>> delay any command by 100ms after a POWER?). >>>> I have two different laptops with the same touchpad hardware, one is >>> Are those 2 Synaptics or Goodix? >> All of them are Gen-1 samples, they are Goodix samples but report >> Synaptics id. >>>> installed Linux, the touchpad can't work unless we apply this patch or >>>> disable runtime in the i2c-hid-core.c, the other is installed windows10, >>>> the touchapd can't work too after installation, after a upgrade online, >>> What do you mean by "after a upgrade online, the touchpad can work"? >> Windows10 upgrade, it will search and install the drivers for hardware. >> After installing the touchpad driver from the internet, the touchpad can >> work. >>>> the touchpad can work. I am willing to test commands and timing under >>>> windows, but I don't know how to do that, is there a guide? >>> I am usinghttps://github.com/bentiss/SimplePeripheralBusProbe >>> It's quite complex to set up, but it's better than having to crack >>> open the laptop and put physical probes in it. >> OK, I will try to setup it and catch the commands on my gen-1 touchpad. > Thanks. It could be interesting to see how Windows behave with it. > Feel free to ping me if you have trouble recompiling your DSDT. > >>> So on my side, I have been checking on a Dell XPS 13 how Windows >>> handled the touchpad. >>> In term of features retrievals, we are similar to what Windows does. >>> >>> There is a significant difference in how many times we switch the >>> device on and off during probe. >>> Windows basically switches it on once, and done. >>> Under Linux we tend to switch it off and on quite a lot, and this is >>> what I must have been told. This has a tendency to confuse some >>> hardware, so we better fix that. >> Yes, I also observed many times of SLEEP and ON under Linux. > So, for that, I am thinking that maybe we should delay the last > `pm_runtime_put()` we do in `i2c_hid_probe()` by, for instance 5 or 10 > secs. So for 5 seconds, we keep the device powered on, which should > give time to udev and the session to open the node, get its > information, close it and the session to get over it. > > It shouldn't be hard to write, and might be save us some headaches. If this design is implemented, so far we can remove NO_RUNTIME_PM_QUIRKS since Xwindow will open the hid device within 5 secs, then the runtime_suspend will not be called outside the system_suspend. And if we add below code in the runtime_resume(), we can remove NO_RUNTIME_PM_QUIRKS too. So far it looks like all touchpads which applied this quirk will lose working mode after PWR_SLEEP, if we restore working mode in the runtime_resume, they don't need to apply that quirk anymore. if (hid->driver && hid->driver->reset_resume) { ret = hid->driver->reset_resume(hid); return ret; } > > Also, once this is in, I think we can consider the use case of > opening/closing the device repeatedly as niche case and probably force > I2C_HID_QUIRK_DELAY_AFTER_SLEEP on all devices. Good idea. I think it is safe to apply this quirk to all touchpads. > >>> In terms of timing, I do not see any special sleep after a wake up. So >>> I guess this must be a goodix limitation. >>> >>> Last, on S3 suspend of the touchpad, I see: >>> set feature latency low >>> SET_POWER sleep >>> >>> then on resume: >>> SET_POWER on >>> set feature latency normal >>> set feature latency normal >>> set feature button on, surface on >>> >>> I have an almost similar sequence on runtime pm (screen going black): >>> suspend: >>> SET_POWER sleep >>> >>> then on resume: >>> SET_POWER on >>> set feature latency normal >>> set feature latency normal >>> set feature button on, surface on >>> >>> The only difference is that when the system is in S3, the touchpad is >>> set in the low latency mode. >> Are they got from Linux? if "set feature xxxx" are from > These traces were taken from the Windows XPS I instrumented. > >> hid->driver->reset_resume(), the existing runtime resume in linux >> doesn't call "set feature xxx" at all, in current i2c-hid-core.c of >> linux, the runtime resume is different from system resume, it doesn't >> call hid->driver->reset_resume(). > Yep. I am still pondering that. It feels weird to force the call of > runtime PM from i2c-hid. > Can't we set up the runtime PM calls from within hid-multitouch directly? > Having the transport level calling those manually seems weird. Yes, to do so, need a big change. > Cheers, > Benjamin >
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 4d1f24ee249c..b44d34b3bc96 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -51,6 +51,8 @@ #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2) #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3) #define I2C_HID_QUIRK_BOGUS_IRQ BIT(4) +#define I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM BIT(5) +#define I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET BIT(6) /* flags */ #define I2C_HID_STARTED 0 @@ -185,7 +187,9 @@ static const struct i2c_hid_quirks { { USB_VENDOR_ID_ELAN, HID_ANY_ID, I2C_HID_QUIRK_BOGUS_IRQ }, { USB_VENDOR_ID_SYNAPTICS, I2C_DEVICE_ID_SYNAPTICS_7E7E, - I2C_HID_QUIRK_NO_RUNTIME_PM }, + I2C_HID_QUIRK_DELAY_AFTER_SLEEP | + I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM | + I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET }, { 0, 0 } }; @@ -1214,6 +1218,24 @@ static void i2c_hid_shutdown(struct i2c_client *client) } #ifdef CONFIG_PM_SLEEP +static int i2c_hid_call_reset_resume(struct i2c_hid *ihid) +{ + struct hid_device *hid = ihid->hid; + + if (hid->driver && hid->driver->reset_resume) { + /* + * On some touchpads like synaptics touchpad (06cb:7e7e), after + * it is powered on, needs to wait for 100 ms, then set the mode + * data to it, otherwise the data can't be set correctly. + */ + if (ihid->quirks & I2C_HID_QUIRK_WAIT_PWRON_AND_MODE_SET) + msleep(100); + return hid->driver->reset_resume(hid); + } + + return 0; +} + static int i2c_hid_suspend(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); @@ -1299,12 +1321,7 @@ static int i2c_hid_resume(struct device *dev) if (ret) return ret; - if (hid->driver && hid->driver->reset_resume) { - ret = hid->driver->reset_resume(hid); - return ret; - } - - return 0; + return i2c_hid_call_reset_resume(ihid); } #endif @@ -1321,9 +1338,14 @@ static int i2c_hid_runtime_suspend(struct device *dev) static int i2c_hid_runtime_resume(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); + struct i2c_hid *ihid = i2c_get_clientdata(client); enable_irq(client->irq); i2c_hid_set_power(client, I2C_HID_PWR_ON); + + if (ihid->quirks & I2C_HID_QUIRK_SET_MODES_IN_RUNTIME_PM) + return i2c_hid_call_reset_resume(ihid); + return 0; } #endif
We disabled the runtime PM for the synaptics touchpad (06cb:7e7e), then it worked, but after S3, the touchpad didn't work again. After more investigation, we found if we don't disable RT PM and only apply I2C_HID_QUIRK_DELAY_AFTER_SLEEP, we can get the interrupt and data report from touchpad, but the data report is invalid to the driver hid-multitouch (this touchpad has HID_GROUP_MULTITOUCH_WIN_8, so the driver hid-multitouch is loaded), if we rmmod hid-multitouch, then the hid-generic is the driver and the data report is valid to this driver, the touchpad work again. The data report is valid to hid-generic while is invalid to hid-multitouch, that is because the hid-multitouch set the specific mode to touchpad and let it report data in a specific format. After we enable runtime PM, the touchpad will be PWR_SLEEP via RT PM, this will make the working mode lost on this touchpad, then it will report data in a different format from the hid-multitouch needed. Let the driver set the working mode in the runtime resume just like the system resume does, this can fix this problem. But on this touchpad, besides the issue of "working mode lost after PWR_SLEEP", it also has one more issue, it needs to wait for 100 ms between PWR_ON and setting mode, otherwise the mode can't be set correctly. Since both system resume and RT resume will call reset_resume, i put the related code in a function, no behaviour change for system resume. Fixes: 74e7c6c877f6 ("HID: i2c-hid: Disable runtime PM on Synaptics touchpad") Signed-off-by: Hui Wang <hui.wang@canonical.com> --- drivers/hid/i2c-hid/i2c-hid-core.c | 36 ++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 7 deletions(-)