Input: elan_i2c - Wait for initialization after enabling regulator supply

[PATCH] Input: elan_i2c - Wait for initialization after enabling regulator supply

Posted by Chen-Yu Tsai 1 month, 4 weeks ago

Elan trackpad controllers require some delay after enabling power to
the controller for the hardware and firmware to initialize:

  - 2ms for hardware initialization
  - 100ms for firmware initialization

Until then, the hardware will not respond to I2C transfers. This was
observed on the MT8173 Chromebooks after the regulator supply for the
trackpad was changed to "not always on".

Add proper delays after regulator_enable() calls.

Fixes: 6696777c6506 ("Input: add driver for Elan I2C/SMbus touchpad")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
This will unfortunately slightly slow down the driver probe and resume.
An optimization would be to check if the regulator is enabled already,
and shorten or skip the delay if it is. This would require a new API
though.
---
 drivers/input/mouse/elan_i2c_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index ce96513b34f6..89556f61004e 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -46,6 +46,8 @@
 #define ETP_FWIDTH_REDUCE	90
 #define ETP_FINGER_WIDTH	15
 #define ETP_RETRY_COUNT		3
+/* H/W init 2 ms + F/W init 100 ms w/ round up */
+#define ETP_POWER_ON_DELAY	110
 
 /* quirks to control the device */
 #define ETP_QUIRK_QUICK_WAKEUP	BIT(0)
@@ -1237,6 +1239,8 @@ static int elan_probe(struct i2c_client *client)
 		return error;
 	}
 
+	msleep(ETP_POWER_ON_DELAY);
+
 	/* Make sure there is something at this address */
 	error = i2c_smbus_read_byte(client);
 	if (error < 0) {
@@ -1374,6 +1378,8 @@ static int elan_resume(struct device *dev)
 			dev_err(dev, "error %d enabling regulator\n", error);
 			goto err;
 		}
+
+		msleep(ETP_POWER_ON_DELAY);
 	}
 
 	error = elan_set_power(data, true);
-- 
2.46.1.824.gd892dcdcdd-goog

Re: [PATCH] Input: elan_i2c - Wait for initialization after enabling regulator supply

Posted by Dmitry Torokhov 1 month, 4 weeks ago

On Tue, Oct 01, 2024 at 05:38:14PM +0800, Chen-Yu Tsai wrote:
> Elan trackpad controllers require some delay after enabling power to
> the controller for the hardware and firmware to initialize:
> 
>   - 2ms for hardware initialization
>   - 100ms for firmware initialization
> 
> Until then, the hardware will not respond to I2C transfers. This was
> observed on the MT8173 Chromebooks after the regulator supply for the
> trackpad was changed to "not always on".
> 
> Add proper delays after regulator_enable() calls.
> 
> Fixes: 6696777c6506 ("Input: add driver for Elan I2C/SMbus touchpad")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> This will unfortunately slightly slow down the driver probe and resume.
> An optimization would be to check if the regulator is enabled already,
> and shorten or skip the delay if it is. This would require a new API
> though.
> ---
>  drivers/input/mouse/elan_i2c_core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index ce96513b34f6..89556f61004e 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -46,6 +46,8 @@
>  #define ETP_FWIDTH_REDUCE	90
>  #define ETP_FINGER_WIDTH	15
>  #define ETP_RETRY_COUNT		3
> +/* H/W init 2 ms + F/W init 100 ms w/ round up */
> +#define ETP_POWER_ON_DELAY	110
>  
>  /* quirks to control the device */
>  #define ETP_QUIRK_QUICK_WAKEUP	BIT(0)
> @@ -1237,6 +1239,8 @@ static int elan_probe(struct i2c_client *client)
>  		return error;
>  	}
>  
> +	msleep(ETP_POWER_ON_DELAY);
> +
>  	/* Make sure there is something at this address */
>  	error = i2c_smbus_read_byte(client);
>  	if (error < 0) {
> @@ -1374,6 +1378,8 @@ static int elan_resume(struct device *dev)
>  			dev_err(dev, "error %d enabling regulator\n", error);
>  			goto err;
>  		}
> +
> +		msleep(ETP_POWER_ON_DELAY);

This will add an unwanted delay on ACPI systems that handle power
sequencing in firmware. However use of "optional" regulators is frowned
upon in this case as the supply in reality is not optional.

Mark, has there been any developments that would help reconciling
difference in behavior between ACPI and DT systems. Ideally we'd need to
know when supply was turned on, and adjust the wait accordingly.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: elan_i2c - Wait for initialization after enabling regulator supply

Posted by Mark Brown 1 month, 3 weeks ago

On Tue, Oct 01, 2024 at 02:57:10AM -0700, Dmitry Torokhov wrote:

> This will add an unwanted delay on ACPI systems that handle power
> sequencing in firmware. However use of "optional" regulators is frowned
> upon in this case as the supply in reality is not optional.

> Mark, has there been any developments that would help reconciling
> difference in behavior between ACPI and DT systems. Ideally we'd need to
> know when supply was turned on, and adjust the wait accordingly.

There's not been any changes here, but there's always been an event
availalbe when a regulator is turned on?  There's also no difference
between DT and ACPI systems here, both could have the regulator fixed on
and I'd certainly not want to rely on an ACPI system implementing a
device specific delay after power on given the sort of stuff they like
to put into machine specific drivers.

Re: [PATCH] Input: elan_i2c - Wait for initialization after enabling regulator supply

Posted by Dmitry Torokhov 1 month, 3 weeks ago

On Tue, Oct 01, 2024 at 12:07:16PM +0100, Mark Brown wrote:
> On Tue, Oct 01, 2024 at 02:57:10AM -0700, Dmitry Torokhov wrote:
> 
> > This will add an unwanted delay on ACPI systems that handle power
> > sequencing in firmware. However use of "optional" regulators is frowned
> > upon in this case as the supply in reality is not optional.
> 
> > Mark, has there been any developments that would help reconciling
> > difference in behavior between ACPI and DT systems. Ideally we'd need to
> > know when supply was turned on, and adjust the wait accordingly.
> 
> There's not been any changes here, but there's always been an event
> availalbe when a regulator is turned on?  There's also no difference
> between DT and ACPI systems here, both could have the regulator fixed on
> and I'd certainly not want to rely on an ACPI system implementing a
> device specific delay after power on given the sort of stuff they like
> to put into machine specific drivers.

Well with Elan in native mode ACPI FW does do proper power sequencing,
that is why this commit mentions failures observed on Mediatek devices.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: elan_i2c - Wait for initialization after enabling regulator supply

Posted by Mark Brown 1 month, 3 weeks ago

On Tue, Oct 01, 2024 at 05:58:58AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 01, 2024 at 12:07:16PM +0100, Mark Brown wrote:

> > availalbe when a regulator is turned on?  There's also no difference
> > between DT and ACPI systems here, both could have the regulator fixed on
> > and I'd certainly not want to rely on an ACPI system implementing a
> > device specific delay after power on given the sort of stuff they like
> > to put into machine specific drivers.

> Well with Elan in native mode ACPI FW does do proper power sequencing,
> that is why this commit mentions failures observed on Mediatek devices.

Yeah, but that's got to get washed through the individual system
firmwares to get deployed and my confidence in vendors is not high.

Re: [PATCH] Input: elan_i2c - Wait for initialization after enabling regulator supply

Posted by Dmitry Torokhov 1 month, 3 weeks ago

On Tue, Oct 01, 2024 at 02:06:19PM +0100, Mark Brown wrote:
> On Tue, Oct 01, 2024 at 05:58:58AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 01, 2024 at 12:07:16PM +0100, Mark Brown wrote:
> 
> > > availalbe when a regulator is turned on?  There's also no difference
> > > between DT and ACPI systems here, both could have the regulator fixed on
> > > and I'd certainly not want to rely on an ACPI system implementing a
> > > device specific delay after power on given the sort of stuff they like
> > > to put into machine specific drivers.
> 
> > Well with Elan in native mode ACPI FW does do proper power sequencing,
> > that is why this commit mentions failures observed on Mediatek devices.
> 
> Yeah, but that's got to get washed through the individual system
> firmwares to get deployed and my confidence in vendors is not high.

I think native Elan is only used in Chromebooks where firmware is
decent, the rest are I2C-HID.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: elan_i2c - Wait for initialization after enabling regulator supply

Posted by Mark Brown 1 month, 3 weeks ago

On Tue, Oct 01, 2024 at 06:11:50AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 01, 2024 at 02:06:19PM +0100, Mark Brown wrote:

> > Yeah, but that's got to get washed through the individual system
> > firmwares to get deployed and my confidence in vendors is not high.

> I think native Elan is only used in Chromebooks where firmware is
> decent, the rest are I2C-HID.

Ah, OK - in that case I agree there should be no problems with ACPI.

Re: [PATCH] Input: elan_i2c - Wait for initialization after enabling regulator supply

Posted by Chen-Yu Tsai 1 month, 3 weeks ago

On Tue, Oct 1, 2024 at 9:13 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Oct 01, 2024 at 06:11:50AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 01, 2024 at 02:06:19PM +0100, Mark Brown wrote:
>
> > > Yeah, but that's got to get washed through the individual system
> > > firmwares to get deployed and my confidence in vendors is not high.
>
> > I think native Elan is only used in Chromebooks where firmware is
> > decent, the rest are I2C-HID.
>
> Ah, OK - in that case I agree there should be no problems with ACPI.

FWIW all the users on ARM (at least in kernel) are also Chromebooks,
and all their supplies are either missing or marked as always on.

So I suppose to avoid affecting the power sequencing time, we should delay
this until there is proper support to handle wait times with the regulator
API.

I might find time to work on that but I make no promises at the moment.

ChenYu