| Message ID | 20190114084734.3362-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: Disable runtime PM on Goodix touchpad | expand |
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?
> 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 >
> 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
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
> 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
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
> 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
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 } };
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(+)