Hi,
On 12-09-18 14:55, Anisse Astier wrote:
Hi Hans,
Thanks for taking the time to review this patch.
On Wed, Sep 12, 2018 at 02:10:18PM +0200, Hans de Goede wrote:
Hello Anisse,
On 12-09-18 12:22, Anisse Astier wrote:
This hantick HTIX5288 touchpad can quickly fall in a wrong state if
there are too many open/close operations. This will either make it stop
reporting any input, or will shift all the input reads by a few bytes,
making it impossible to decode.
Here, we synchronously block the the runtime suspend by 2.5 seconds;
this was found experimentally, and sleeping less will not result in a
non-working touchpad.
This quirk will also result in a non-responsive touchpad for 2.5s if
userspace closes, then opens a device quickly, but this was deemed an
appropriate compromise versus a non-working touchpad.
This fast repetition of sleep/wakeup is also more likely to happen when
using runtime PM, which is why the quirk is done there, and not for all
power downs, which would include suspend or module removal.
Signed-off-by: Anisse Astier <anisse@xxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
---
This patch feels a bit hack-ish, but I don't really know how to debug this
issue further.
I'm looking forward to your feedback.
Interesting, what laptop are you seeing this on, we've had lots of
This an OEM's Gemini Lake laptop with a Celeron N4000, I don't think it
has shipped with a recognizable name. DMI information is basically
"Default string" and the infamous "To Be Filled By O.E.M".
Ok.
bug reports about this touchpad, mainly see:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244
But we've been unable to get to the bottom of this, so I'm wondering
if you are using one of the machines mentioned in that bug report and
thus if you are seeing the same problem.
I don't think your msleep(2500) is a good solution though, I think
we just need to disable runtime pm all together on these devices,
That's actually the first thing I did, I just removed the
SET_RUNTIME_PM_OPS ; but I didn't want to go nuclear and disable it
entirely. It might be the best option though, I don't think this can
really have a big impact on battery life.
No, when running a graphical user session the touchpad evdev
device node will be open all the time, so runtime pm will not
do much. And I expect these laptops to pretty much always run
a graphical user session.
can you try changing the
pm_runtime_put(&client->dev);
return 0;
Lines at the end of i2c_hid_probe() to:
if (!(ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM))
pm_runtime_put(&client->dev);
And in i2c_hid_remove() change:
struct hid_device *hid;
pm_runtime_get_sync(&client->dev);
pm_runtime_disable(&client->dev);
to:
struct hid_device *hid;
if (!(ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM))
pm_runtime_get_sync(&client->dev);
pm_runtime_disable(&client->dev);
And then drop this bit:
@@ -1259,6 +1262,10 @@ static int i2c_hid_resume(struct device *dev)
static int i2c_hid_runtime_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
+ struct i2c_hid *ihid = i2c_get_clientdata(client);
+
+ if (ihid->quirks & I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM)
+ msleep(2500);
i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
disable_irq(client->irq);
From your patch and see if that also fixes things for you
(I would expect it will).
I updated to try your approach. Note that there are other pm refcounting
calls in open/close, but they should be no-ops since the counter never
drops to 0 now, right ?
Right.
If that also fixes things, please also rename the quirk from
I2C_HID_QUIRK_SLOW_DOWN_RUNTIME_PM to I2C_HID_QUIRK_NO_RUNTIME_PM
Also there already is an existing quirk for the hantick
touchpad which is necessary on at least some devices. So you need
to or ('|') your quirk flag together with the existing
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk for these devices
And then submit a v2 upstream.
I'll send v2 in reply to this message. From quick testing it seems to
work.
Thanks. If this really fixes:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1728244
You've made a lot of people very happy!
I've added a comment to that bug asking people to test.
Regards,
Hans
