| Message ID | 20191020214718.150906-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Benjamin Tissoires |
| Headers | show |
| Series | HID: i2c-hid: Send power-on command after reset | expand |
Hi Hans, > On Oct 21, 2019, at 5:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: > > Before commit 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power > management"), any i2c-hid touchscreens would typically be runtime-suspended > between the driver loading and Xorg or a Wayland compositor opening it, > causing it to be resumed again. This means that before this change, > we would call i2c_hid_set_power(OFF), i2c_hid_set_power(ON) before the > graphical session would start listening to the touchscreen. > > It turns out that at least some SIS touchscreens, such as the one found > on the Asus T100HA, need a power-on command after reset, otherwise they > will not send any events. As You-Sheng pointed out before, device may need a 60ms delay between ON and RESET command. Does adding the delay help? Kai-Heng > > Fixes: 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index d9c55e30f986..04c088131e04 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -447,8 +447,12 @@ static int i2c_hid_hwreset(struct i2c_client *client) > if (ret) { > dev_err(&client->dev, "failed to reset device.\n"); > i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); > + goto out_unlock; > } > > + /* At least some SIS devices need this after reset */ > + ret = i2c_hid_set_power(client, I2C_HID_PWR_ON); > + > out_unlock: > mutex_unlock(&ihid->reset_lock); > return ret; > -- > 2.23.0 >
Hi, On 21-10-2019 09:17, Kai Heng Feng wrote: > Hi Hans, > >> On Oct 21, 2019, at 5:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Before commit 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power >> management"), any i2c-hid touchscreens would typically be runtime-suspended >> between the driver loading and Xorg or a Wayland compositor opening it, >> causing it to be resumed again. This means that before this change, >> we would call i2c_hid_set_power(OFF), i2c_hid_set_power(ON) before the >> graphical session would start listening to the touchscreen. >> >> It turns out that at least some SIS touchscreens, such as the one found >> on the Asus T100HA, need a power-on command after reset, otherwise they >> will not send any events. > > As You-Sheng pointed out before, device may need a 60ms delay between ON and RESET command. > Does adding the delay help? I just tried increasing the existing usleep between ON and RESET to: usleep_range(60000, 70000); And this does not help. Note that before we had quirks for devices with a SIS screen needed, where we avoided the reset on resume and instead did just an i2c_hid_set_power(client, I2C_HID_PWR_ON) for these. Which likely was to work around the same problem, these devices simply need a i2c_hid_set_power(client, I2C_HID_PWR_ON) after rest to function. Assuming other devices do come up in the "ON" state after reset then this will be a no-op for them and this should thus not impact their operation. Also I just noticed that 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") has been added to 5.4-rc# as a fix, so we really need to get this in place to to not avoid regressing devices with a SIS touchscreen actually quite a few devices including some quite popular ones uses a SIS touchscreen, here is the list of devices I know about: Asus T100HA Asus T200TA Peaq 10.1" C1010 2-in-1 Toshiba Click Mini L9W-B Regards, Hans >> Fixes: 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") >> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/hid/i2c-hid/i2c-hid-core.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c >> index d9c55e30f986..04c088131e04 100644 >> --- a/drivers/hid/i2c-hid/i2c-hid-core.c >> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c >> @@ -447,8 +447,12 @@ static int i2c_hid_hwreset(struct i2c_client *client) >> if (ret) { >> dev_err(&client->dev, "failed to reset device.\n"); >> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); >> + goto out_unlock; >> } >> >> + /* At least some SIS devices need this after reset */ >> + ret = i2c_hid_set_power(client, I2C_HID_PWR_ON); >> + >> out_unlock: >> mutex_unlock(&ihid->reset_lock); >> return ret; >> -- >> 2.23.0 >> >
Hi Hans, > On Oct 30, 2019, at 23:11, Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 21-10-2019 09:17, Kai Heng Feng wrote: >> Hi Hans, >>> On Oct 21, 2019, at 5:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Before commit 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power >>> management"), any i2c-hid touchscreens would typically be runtime-suspended >>> between the driver loading and Xorg or a Wayland compositor opening it, >>> causing it to be resumed again. This means that before this change, >>> we would call i2c_hid_set_power(OFF), i2c_hid_set_power(ON) before the >>> graphical session would start listening to the touchscreen. >>> >>> It turns out that at least some SIS touchscreens, such as the one found >>> on the Asus T100HA, need a power-on command after reset, otherwise they >>> will not send any events. >> As You-Sheng pointed out before, device may need a 60ms delay between ON and RESET command. >> Does adding the delay help? > > I just tried increasing the existing usleep between ON and RESET to: > > usleep_range(60000, 70000); > > And this does not help. Note that before we had quirks for devices with a SIS > screen needed, where we avoided the reset on resume and instead did just an > i2c_hid_set_power(client, I2C_HID_PWR_ON) for these. > > Which likely was to work around the same problem, these devices simply need a > i2c_hid_set_power(client, I2C_HID_PWR_ON) after rest to function. > > Assuming other devices do come up in the "ON" state after reset then this > will be a no-op for them and this should thus not impact their operation. > > Also I just noticed that 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") > has been added to 5.4-rc# as a fix, so we really need to get this in place > to to not avoid regressing devices with a SIS touchscreen actually quite a > few devices including some quite popular ones uses a SIS touchscreen, here > is the list of devices I know about: I agree we should use this workaround since increasing delay doesn't work. Kai-Heng > > Asus T100HA > Asus T200TA > Peaq 10.1" C1010 2-in-1 > Toshiba Click Mini L9W-B > > Regards, > > Hans > > > >>> Fixes: 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") >>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> drivers/hid/i2c-hid/i2c-hid-core.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c >>> index d9c55e30f986..04c088131e04 100644 >>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c >>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c >>> @@ -447,8 +447,12 @@ static int i2c_hid_hwreset(struct i2c_client *client) >>> if (ret) { >>> dev_err(&client->dev, "failed to reset device.\n"); >>> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); >>> + goto out_unlock; >>> } >>> >>> + /* At least some SIS devices need this after reset */ >>> + ret = i2c_hid_set_power(client, I2C_HID_PWR_ON); >>> + >>> out_unlock: >>> mutex_unlock(&ihid->reset_lock); >>> return ret; >>> -- >>> 2.23.0
Hi Hans, > On Oct 31, 2019, at 01:39, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Hi Hans, > >> On Oct 30, 2019, at 23:11, Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 21-10-2019 09:17, Kai Heng Feng wrote: >>> Hi Hans, >>>> On Oct 21, 2019, at 5:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Before commit 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power >>>> management"), any i2c-hid touchscreens would typically be runtime-suspended >>>> between the driver loading and Xorg or a Wayland compositor opening it, >>>> causing it to be resumed again. This means that before this change, >>>> we would call i2c_hid_set_power(OFF), i2c_hid_set_power(ON) before the >>>> graphical session would start listening to the touchscreen. >>>> >>>> It turns out that at least some SIS touchscreens, such as the one found >>>> on the Asus T100HA, need a power-on command after reset, otherwise they >>>> will not send any events. >>> As You-Sheng pointed out before, device may need a 60ms delay between ON and RESET command. >>> Does adding the delay help? >> >> I just tried increasing the existing usleep between ON and RESET to: >> >> usleep_range(60000, 70000); >> >> And this does not help. Note that before we had quirks for devices with a SIS >> screen needed, where we avoided the reset on resume and instead did just an >> i2c_hid_set_power(client, I2C_HID_PWR_ON) for these. >> >> Which likely was to work around the same problem, these devices simply need a >> i2c_hid_set_power(client, I2C_HID_PWR_ON) after rest to function. >> >> Assuming other devices do come up in the "ON" state after reset then this >> will be a no-op for them and this should thus not impact their operation. >> >> Also I just noticed that 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") >> has been added to 5.4-rc# as a fix, so we really need to get this in place >> to to not avoid regressing devices with a SIS touchscreen actually quite a >> few devices including some quite popular ones uses a SIS touchscreen, here >> is the list of devices I know about: > > I agree we should use this workaround since increasing delay doesn't work. I just checked the spec again, seems like ON before RESET in probe is unnecessary. Does removing the ON command in probe help? > > Kai-Heng > >> >> Asus T100HA >> Asus T200TA >> Peaq 10.1" C1010 2-in-1 >> Toshiba Click Mini L9W-B >> >> Regards, >> >> Hans >> >> >> >>>> Fixes: 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") >>>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/hid/i2c-hid/i2c-hid-core.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c >>>> index d9c55e30f986..04c088131e04 100644 >>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c >>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c >>>> @@ -447,8 +447,12 @@ static int i2c_hid_hwreset(struct i2c_client *client) >>>> if (ret) { >>>> dev_err(&client->dev, "failed to reset device.\n"); >>>> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); >>>> + goto out_unlock; >>>> } >>>> >>>> + /* At least some SIS devices need this after reset */ >>>> + ret = i2c_hid_set_power(client, I2C_HID_PWR_ON); >>>> + >>>> out_unlock: >>>> mutex_unlock(&ihid->reset_lock); >>>> return ret; >>>> -- >>>> 2.23.0
Hi, On 31-10-2019 07:23, Kai-Heng Feng wrote: > Hi Hans, > >> On Oct 31, 2019, at 01:39, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: >> >> Hi Hans, >> >>> On Oct 30, 2019, at 23:11, Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> Hi, >>> >>> On 21-10-2019 09:17, Kai Heng Feng wrote: >>>> Hi Hans, >>>>> On Oct 21, 2019, at 5:47 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> >>>>> Before commit 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power >>>>> management"), any i2c-hid touchscreens would typically be runtime-suspended >>>>> between the driver loading and Xorg or a Wayland compositor opening it, >>>>> causing it to be resumed again. This means that before this change, >>>>> we would call i2c_hid_set_power(OFF), i2c_hid_set_power(ON) before the >>>>> graphical session would start listening to the touchscreen. >>>>> >>>>> It turns out that at least some SIS touchscreens, such as the one found >>>>> on the Asus T100HA, need a power-on command after reset, otherwise they >>>>> will not send any events. >>>> As You-Sheng pointed out before, device may need a 60ms delay between ON and RESET command. >>>> Does adding the delay help? >>> >>> I just tried increasing the existing usleep between ON and RESET to: >>> >>> usleep_range(60000, 70000); >>> >>> And this does not help. Note that before we had quirks for devices with a SIS >>> screen needed, where we avoided the reset on resume and instead did just an >>> i2c_hid_set_power(client, I2C_HID_PWR_ON) for these. >>> >>> Which likely was to work around the same problem, these devices simply need a >>> i2c_hid_set_power(client, I2C_HID_PWR_ON) after rest to function. >>> >>> Assuming other devices do come up in the "ON" state after reset then this >>> will be a no-op for them and this should thus not impact their operation. >>> >>> Also I just noticed that 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") >>> has been added to 5.4-rc# as a fix, so we really need to get this in place >>> to to not avoid regressing devices with a SIS touchscreen actually quite a >>> few devices including some quite popular ones uses a SIS touchscreen, here >>> is the list of devices I know about: >> >> I agree we should use this workaround since increasing delay doesn't work. > > I just checked the spec again, seems like ON before RESET in probe is unnecessary. > Does removing the ON command in probe help? I just tried, removing the ON command done before the RESET does not help. Regards, Hans >>>>> Fixes: 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") >>>>> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> --- >>>>> drivers/hid/i2c-hid/i2c-hid-core.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c >>>>> index d9c55e30f986..04c088131e04 100644 >>>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c >>>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c >>>>> @@ -447,8 +447,12 @@ static int i2c_hid_hwreset(struct i2c_client *client) >>>>> if (ret) { >>>>> dev_err(&client->dev, "failed to reset device.\n"); >>>>> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); >>>>> + goto out_unlock; >>>>> } >>>>> >>>>> + /* At least some SIS devices need this after reset */ >>>>> + ret = i2c_hid_set_power(client, I2C_HID_PWR_ON); >>>>> + >>>>> out_unlock: >>>>> mutex_unlock(&ihid->reset_lock); >>>>> return ret; >>>>> -- >>>>> 2.23.0 >
On Sun, 20 Oct 2019, Hans de Goede wrote: > Before commit 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power > management"), any i2c-hid touchscreens would typically be runtime-suspended > between the driver loading and Xorg or a Wayland compositor opening it, > causing it to be resumed again. This means that before this change, > we would call i2c_hid_set_power(OFF), i2c_hid_set_power(ON) before the > graphical session would start listening to the touchscreen. > > It turns out that at least some SIS touchscreens, such as the one found > on the Asus T100HA, need a power-on command after reset, otherwise they > will not send any events. > > Fixes: 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Thanks a lot for debugging this. I've queued this in for-5.4/upstream-fixes and will make sure it hits Linus' tree before 5.4 final.
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index d9c55e30f986..04c088131e04 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -447,8 +447,12 @@ static int i2c_hid_hwreset(struct i2c_client *client) if (ret) { dev_err(&client->dev, "failed to reset device.\n"); i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); + goto out_unlock; } + /* At least some SIS devices need this after reset */ + ret = i2c_hid_set_power(client, I2C_HID_PWR_ON); + out_unlock: mutex_unlock(&ihid->reset_lock); return ret;
Before commit 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management"), any i2c-hid touchscreens would typically be runtime-suspended between the driver loading and Xorg or a Wayland compositor opening it, causing it to be resumed again. This means that before this change, we would call i2c_hid_set_power(OFF), i2c_hid_set_power(ON) before the graphical session would start listening to the touchscreen. It turns out that at least some SIS touchscreens, such as the one found on the Asus T100HA, need a power-on command after reset, otherwise they will not send any events. Fixes: 67b18dfb8cfc ("HID: i2c-hid: Remove runtime power management") Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/hid/i2c-hid/i2c-hid-core.c | 4 ++++ 1 file changed, 4 insertions(+)