Re: [PATCH] Input: elan_i2c - Fix regulator enable count imbalance after suspend/resume

[Hans de Goede <hdegoede@redhat.com>]

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@gmail.com] 
> Sent: Tuesday, January 4, 2022 8:30 AM
> To: Hans de Goede <hdegoede@redhat.com>
> Cc: Jingle Wu <jingle.wu@emc.com.tw>; linux-input@vger.kernel.org
> 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@redhat.com>
>> ---
>>  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.
>