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 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@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
>
