Hi, On 1/6/22 07:50, Jingle.Wu wrote: > Hi Hans/Dmitry: > > It is not OK to skip elan_enable_power() for all Elan touchpad controllers. > > I suggest to deal with this touchpad module ID as "quirk" case to support > this special case. There is nothing special / quirky about this, regulator enable imbalances simply must not happen / must be fixed. I'll prepare a new version of the patch which adds a parameter to elan_enable_power() to skip the regulator enable if the regulator was not disabled on suspend. This will allow still always calling elan_enable_power() on resume, while also fixing the regulator imbalance. Note that this will also help save power, the regulator imbalance also means that the regulator will no longer get disabled _ever_ after the first suspend/resume cycle where device_may_wakeup() returns true. Regards, Hans > -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] > Sent: Tuesday, January 4, 2022 8:30 AM > To: Hans de Goede <hdegoede@xxxxxxxxxx> > Cc: Jingle Wu <jingle.wu@xxxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] Input: elan_i2c - Fix regulator enable count imbalance > after suspend/resume > > Hi Hans, > > On Wed, Dec 22, 2021 at 11:06:41PM +0100, Hans de Goede wrote: >> Before these changes elan_suspend() would only call >> elan_disable_power() when device_may_wakeup() returns false; whereas >> elan_resume() would unconditionally call elan_enable_power(), leading >> to an enable count imbalance when device_may_wakeup() returns true. >> >> This triggers the "WARN_ON(regulator->enable_count)" in >> regulator_put() when the elan_i2c driver gets unbound, this happens >> e.g. with the hot-plugable dock with Elan I2C touchpad for the Asus TF103C > 2-in-1. >> >> Fix this by making the elan_enable_power() call also be conditional on >> device_may_wakeup() returning false. > > elan_enable_power() not only tries to toggle the regulator, but also tries > to issue command to the controller to power/wake it up. I need confirmation > from Jingle Wu that skipping this is OK for all Elan touchpad controllers, > or if we need to add call to either power control or sleep control method to > make sure the controller it fully resumed. > > Jingle, what can you tell us here? > >> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >> --- >> drivers/input/mouse/elan_i2c_core.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/input/mouse/elan_i2c_core.c >> b/drivers/input/mouse/elan_i2c_core.c >> index 47af62c12267..cdb36d35ffa4 100644 >> --- a/drivers/input/mouse/elan_i2c_core.c >> +++ b/drivers/input/mouse/elan_i2c_core.c >> @@ -1412,17 +1412,17 @@ static int __maybe_unused elan_resume(struct > device *dev) >> struct elan_tp_data *data = i2c_get_clientdata(client); >> int error; >> >> - if (device_may_wakeup(dev) && data->irq_wake) { >> + if (!device_may_wakeup(dev)) { >> + error = elan_enable_power(data); >> + if (error) { >> + dev_err(dev, "power up when resuming failed: %d\n", > error); >> + goto err; >> + } >> + } else if (data->irq_wake) { >> disable_irq_wake(client->irq); >> data->irq_wake = false; >> } >> >> - error = elan_enable_power(data); >> - if (error) { >> - dev_err(dev, "power up when resuming failed: %d\n", error); >> - goto err; >> - } >> - >> error = elan_initialize(data, data->quirks & > ETP_QUIRK_QUICK_WAKEUP); >> if (error) >> dev_err(dev, "initialize when resuming failed: %d\n", > error); >> -- >> 2.33.1 >> > > Thanks. >
