On 2019/4/8 下午11:44, Benjamin Tissoires wrote:
Hi,
On Mon, Apr 8, 2019 at 6:07 AM Hui Wang <hui.wang@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
---
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
