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". > 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. > 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 ? > > 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, Anisse
