[PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Kai-Heng Feng]

A Goodix touchpad doesn't work. Touching the touchpad can trigger IRQ
but there's no input event from HID subsystem.

Turns out it reports some invalid data:
[   22.136630] i2c_hid i2c-DELL091F:00: input: 0b 00 01 00 00 00 00 00 00 00 00

After some trial and error, it's another device that doesn't work well
with ON/SLEEP commands. Disable runtime PM to fix the issue.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/hid/hid-ids.h         | 3 +++
 drivers/hid/i2c-hid/i2c-hid.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 3171db744c1c..6a8cb0679961 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -442,6 +442,9 @@
 #define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A 0x010a
 #define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100 0xe100
 
+#define I2C_VENDOR_ID_GOODIX		0x27c6
+#define I2C_DEVICE_ID_GOODIX_01F0	0x01f0
+
 #define USB_VENDOR_ID_GOODTOUCH		0x1aad
 #define USB_DEVICE_ID_GOODTOUCH_000f	0x000f
 
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 103916c6026c..b235d2b38f39 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -178,6 +178,8 @@ static const struct i2c_hid_quirks {
 		I2C_HID_QUIRK_RESEND_REPORT_DESCR },
 	{ USB_VENDOR_ID_LG, I2C_DEVICE_ID_LG_8001,
 		I2C_HID_QUIRK_NO_RUNTIME_PM },
+	{ I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
+		I2C_HID_QUIRK_NO_RUNTIME_PM },
 	{ 0, 0 }
 };
 
-- 
2.17.1

Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Jiri Kosina]

On Mon, 14 Jan 2019, Kai-Heng Feng wrote:

> A Goodix touchpad doesn't work. Touching the touchpad can trigger IRQ
> but there's no input event from HID subsystem.
> 
> Turns out it reports some invalid data:
> [   22.136630] i2c_hid i2c-DELL091F:00: input: 0b 00 01 00 00 00 00 00 00 00 00
> 
> After some trial and error, it's another device that doesn't work well
> with ON/SLEEP commands. Disable runtime PM to fix the issue.

Thanks, I've now applied the patch to for-5.0/upstream-fixes. I am 
wondering though we are we seeing these at all - do other OSes not do the 
runtime PM on i2c at all?

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Kai Heng Feng]

> On Jan 14, 2019, at 19:18, Jiri Kosina <jikos@kernel.org> wrote:
> 
> On Mon, 14 Jan 2019, Kai-Heng Feng wrote:
> 
>> A Goodix touchpad doesn't work. Touching the touchpad can trigger IRQ
>> but there's no input event from HID subsystem.
>> 
>> Turns out it reports some invalid data:
>> [   22.136630] i2c_hid i2c-DELL091F:00: input: 0b 00 01 00 00 00 00 00 00 00 00
>> 
>> After some trial and error, it's another device that doesn't work well
>> with ON/SLEEP commands. Disable runtime PM to fix the issue.
> 
> Thanks, I've now applied the patch to for-5.0/upstream-fixes. I am 
> wondering though we are we seeing these at all - do other OSes not do the 
> runtime PM on i2c at all?

According to the vendor, Windows does use ON/SLEEP, but infrequently.

We can use pm_runtime_use_autosuspend() to reduce the frequency of
ON/SLEEP commands, but it’s just papering over the touchpad firmware
bug.

Kai-Heng

> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Kai-Heng Feng]

> On Jan 16, 2019, at 16:21, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
> 
> 
> 
>> On Jan 14, 2019, at 19:18, Jiri Kosina <jikos@kernel.org> wrote:
>> 
>> On Mon, 14 Jan 2019, Kai-Heng Feng wrote:
>> 
>>> A Goodix touchpad doesn't work. Touching the touchpad can trigger IRQ
>>> but there's no input event from HID subsystem.
>>> 
>>> Turns out it reports some invalid data:
>>> [   22.136630] i2c_hid i2c-DELL091F:00: input: 0b 00 01 00 00 00 00 00 00 00 00
>>> 
>>> After some trial and error, it's another device that doesn't work well
>>> with ON/SLEEP commands. Disable runtime PM to fix the issue.
>> 
>> Thanks, I've now applied the patch to for-5.0/upstream-fixes. I am 
>> wondering though we are we seeing these at all - do other OSes not do the 
>> runtime PM on i2c at all?
> 
> According to the vendor, Windows does use ON/SLEEP, but infrequently.
> 
> We can use pm_runtime_use_autosuspend() to reduce the frequency of
> ON/SLEEP commands, but it’s just papering over the touchpad firmware
> bug.

Goodix says the firmware needs at least 60ms to fully respond ON and
SLEEP command.

I’ll see if an 200ms autosuspend can solve all Goodix, LG and Raydium
touchpanels.

Kai-Heng

> 
> Kai-Heng
> 
>> 
>> -- 
>> Jiri Kosina
>> SUSE Labs

Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Benjamin Tissoires]

On Thu, Jan 17, 2019 at 6:02 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
>
>
> > On Jan 16, 2019, at 16:21, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
> >
> >
> >
> >> On Jan 14, 2019, at 19:18, Jiri Kosina <jikos@kernel.org> wrote:
> >>
> >> On Mon, 14 Jan 2019, Kai-Heng Feng wrote:
> >>
> >>> A Goodix touchpad doesn't work. Touching the touchpad can trigger IRQ
> >>> but there's no input event from HID subsystem.
> >>>
> >>> Turns out it reports some invalid data:
> >>> [   22.136630] i2c_hid i2c-DELL091F:00: input: 0b 00 01 00 00 00 00 00 00 00 00
> >>>
> >>> After some trial and error, it's another device that doesn't work well
> >>> with ON/SLEEP commands. Disable runtime PM to fix the issue.
> >>
> >> Thanks, I've now applied the patch to for-5.0/upstream-fixes. I am
> >> wondering though we are we seeing these at all - do other OSes not do the
> >> runtime PM on i2c at all?
> >
> > According to the vendor, Windows does use ON/SLEEP, but infrequently.
> >
> > We can use pm_runtime_use_autosuspend() to reduce the frequency of
> > ON/SLEEP commands, but it’s just papering over the touchpad firmware
> > bug.
>
> Goodix says the firmware needs at least 60ms to fully respond ON and
> SLEEP command.

I was about to say that this is not conformant to the specification.
But looking at 7.2.8 SET_POWER of
https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)

They say:
"The DEVICE must ensure that it transitions to the HOST specified
Power State in under 1 second. "
But in the RESPONSE paragraph: "The DEVICE shall not respond back
after receiving the command. The DEVICE is mandated to enter that
power state imminently."

My interpretation was always that the device has to be in a ready
state for new commands "immediately".
However, the first sentence may suggest that the driver would block
any command to the device before the 1 second timestamp.

>
> I’ll see if an 200ms autosuspend can solve all Goodix, LG and Raydium
> touchpanels.

We already have a I2C_HID_QUIRK_DELAY_AFTER_SLEEP quirk that delay
operations after sleep by 20ms. Maybe we can keep the runtime PM but
use the same timers to not confuse the hardware.

Cheers,
Benjamin

Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Kai-Heng Feng]

> On Jan 17, 2019, at 16:06, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> 
> On Thu, Jan 17, 2019 at 6:02 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
[snipped]
>> Goodix says the firmware needs at least 60ms to fully respond ON and
>> SLEEP command.
> 
> I was about to say that this is not conformant to the specification.
> But looking at 7.2.8 SET_POWER of
> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
> 
> They say:
> "The DEVICE must ensure that it transitions to the HOST specified
> Power State in under 1 second. "
> But in the RESPONSE paragraph: "The DEVICE shall not respond back
> after receiving the command. The DEVICE is mandated to enter that
> power state imminently."
> 
> My interpretation was always that the device has to be in a ready
> state for new commands "immediately".
> However, the first sentence may suggest that the driver would block
> any command to the device before the 1 second timestamp.
> 
>> 
>> I’ll see if an 200ms autosuspend can solve all Goodix, LG and Raydium
>> touchpanels.
> 
> We already have a I2C_HID_QUIRK_DELAY_AFTER_SLEEP quirk that delay
> operations after sleep by 20ms. Maybe we can keep the runtime PM but
> use the same timers to not confuse the hardware.

Yes, but wouldn’t use pm_*_autosuspend() helpers can both solve the issue
and make the code more cleaner?

Kai-Heng

> 
> Cheers,
> Benjamin

Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Benjamin Tissoires]

On Thu, Jan 17, 2019 at 12:41 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
>
>
> > On Jan 17, 2019, at 16:06, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >
> > On Thu, Jan 17, 2019 at 6:02 AM Kai-Heng Feng
> > <kai.heng.feng@canonical.com> wrote:
> [snipped]
> >> Goodix says the firmware needs at least 60ms to fully respond ON and
> >> SLEEP command.
> >
> > I was about to say that this is not conformant to the specification.
> > But looking at 7.2.8 SET_POWER of
> > https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
> >
> > They say:
> > "The DEVICE must ensure that it transitions to the HOST specified
> > Power State in under 1 second. "
> > But in the RESPONSE paragraph: "The DEVICE shall not respond back
> > after receiving the command. The DEVICE is mandated to enter that
> > power state imminently."
> >
> > My interpretation was always that the device has to be in a ready
> > state for new commands "immediately".
> > However, the first sentence may suggest that the driver would block
> > any command to the device before the 1 second timestamp.
> >
> >>
> >> I’ll see if an 200ms autosuspend can solve all Goodix, LG and Raydium
> >> touchpanels.
> >
> > We already have a I2C_HID_QUIRK_DELAY_AFTER_SLEEP quirk that delay
> > operations after sleep by 20ms. Maybe we can keep the runtime PM but
> > use the same timers to not confuse the hardware.
>
> Yes, but wouldn’t use pm_*_autosuspend() helpers can both solve the issue
> and make the code more cleaner?

You are probably correct :)

I am not too familiar with the details of the runtime PM API, but if
you can make this a barrier to prevent the host to call too many
suspends/resumes in a row, that would be nice.
And we might be able to ditch I2C_HID_QUIRK_DELAY_AFTER_SLEEP and
I2C_HID_QUIRK_NO_RUNTIME_PM.

Adding the involved people in the thread.

Cheers,
Benjamin

Re: [PATCH] HID: i2c-hid: Disable runtime PM on Goodix touchpad

[Kai-Heng Feng]

> On Jan 17, 2019, at 20:35, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> 
> On Thu, Jan 17, 2019 at 12:41 PM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> 
>> 
>> 
>>> On Jan 17, 2019, at 16:06, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>>> 
>>> On Thu, Jan 17, 2019 at 6:02 AM Kai-Heng Feng
>>> <kai.heng.feng@canonical.com> wrote:
>> [snipped]
>>>> Goodix says the firmware needs at least 60ms to fully respond ON and
>>>> SLEEP command.
>>> 
>>> I was about to say that this is not conformant to the specification.
>>> But looking at 7.2.8 SET_POWER of
>>> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn642101(v=vs.85)
>>> 
>>> They say:
>>> "The DEVICE must ensure that it transitions to the HOST specified
>>> Power State in under 1 second. "
>>> But in the RESPONSE paragraph: "The DEVICE shall not respond back
>>> after receiving the command. The DEVICE is mandated to enter that
>>> power state imminently."
>>> 
>>> My interpretation was always that the device has to be in a ready
>>> state for new commands "immediately".
>>> However, the first sentence may suggest that the driver would block
>>> any command to the device before the 1 second timestamp.
>>> 
>>>> 
>>>> I’ll see if an 200ms autosuspend can solve all Goodix, LG and Raydium
>>>> touchpanels.
>>> 
>>> We already have a I2C_HID_QUIRK_DELAY_AFTER_SLEEP quirk that delay
>>> operations after sleep by 20ms. Maybe we can keep the runtime PM but
>>> use the same timers to not confuse the hardware.
>> 
>> Yes, but wouldn’t use pm_*_autosuspend() helpers can both solve the issue
>> and make the code more cleaner?
> 
> You are probably correct :)
> 
> I am not too familiar with the details of the runtime PM API, but if
> you can make this a barrier to prevent the host to call too many
> suspends/resumes in a row, that would be nice.
> And we might be able to ditch I2C_HID_QUIRK_DELAY_AFTER_SLEEP and
> I2C_HID_QUIRK_NO_RUNTIME_PM.

Ok, using autosuspend helpers doesn’t really solve the issue.

Although it can cover the case like fast ON/SLEEP triggered by
quick transition of display manager -> desktop session, but once
it gets runtime suspended, we can never sure when it’ll get 
runtime resumed again. So a mleep() between each powering
commands is still needed.

So I think we should stick to I2C_HID_QUIRK_NO_RUNTIME_PM
for now.

Kai-Heng

> 
> Adding the involved people in the thread.
> 
> Cheers,
> Benjamin