[PATCH v2 0/7] arm64: mediatek: Chromebook trackpad supply fixes

[PATCH v2 0/7] arm64: mediatek: Chromebook trackpad supply fixes

[Chen-Yu Tsai]

Hi everyone,

This series fixes the trackpad descriptions on some MediaTek-based
Chromebooks: either the trackpad's supply was set as always-on to
workaround missing delays in the driver, or the delay and supply
are missing from the trackpad's device node.

v1 was just the first patch [1]. It has since grown to cover multiple
drivers and devices.


Patch 1 adds the correct enable delay after enabling the supply regulator
for the Elan trackpad to initialize. Compared to v1, the delay is now
skipped if the regulator was already enabled to avoid impacting boot
time or time before the trackpad is operational.

Patch 2 applies the same logic of skipping the power on delay to the
i2c-hid-of driver.

Patch 3 applies the same logic of skipping the power on delay to the
i2c OF component prober library.

Patch 4 adds a delay between when the device node found is enabled and
when regulator_disable() is called. This gives an asynchronously probing
driver some time to increment the enable count of their regulator
reference, thus keeping the device operational and allowing the driver
to skip the initialization delay.

Patch 5 adds the correct delay for probing trackpads for Hana devices
to the ChromeOS OF component prober.

Patch 6 removes the "always-on" setting from the trackpad supply for
Elm / Hana and adds the correct delay to the second source trackpad.
This corrects the hardware description.

Patch 7 adds the supply and power on delay properties to the Synaptics
trackpad on the Spherion device. Combined with previous driver changes
this should cause no actual functional changes or delays.


Please take a look. There are no build time dependencies between any
of the patches, but the DT changes must go in after all the driver
changes land, especially the first one adding delays to the Elan
trackpad driver. Otherwise one could potentially end up with a
non-functional trackpad on the device.


Thanks
ChenYu

[1] https://lore.kernel.org/all/20241001093815.2481899-1-wenst@chromium.org/

Chen-Yu Tsai (7):
  Input: elan_i2c - Wait for initialization after enabling regulator
    supply
  HID: i2c-hid-of: skip post-power-on delay if already powered on
  i2c: of-prober: skip post-power-on delay if already powered on
  i2c: of-prober: Defer regulator_disable() on successful probe in
    simple helper
  platform/chrome: of_hw_prober: Add delay for hana trackpads
  arm64: dts: mediatek: mt8173-elm-hana: Unmark trackpad supply as
    always-on
  arm64: dts: mediatek: mt8192-asurada-spherion: Add Synaptics
    trackpad's supply

 .../boot/dts/mediatek/mt8173-elm-hana.dtsi    |  8 +----
 arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi  |  1 -
 .../mediatek/mt8192-asurada-spherion-r0.dts   |  2 ++
 drivers/hid/i2c-hid/i2c-hid-of.c              |  8 ++++-
 drivers/i2c/i2c-core-of-prober.c              | 29 +++++++++++++++----
 drivers/input/mouse/elan_i2c_core.c           | 13 +++++++++
 .../platform/chrome/chromeos_of_hw_prober.c   |  4 +--
 7 files changed, 48 insertions(+), 17 deletions(-)

-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 1/7] Input: elan_i2c - Wait for initialization after enabling regulator supply

[Chen-Yu Tsai]

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. To avoid impacting
the boot time of existing devices that have the power rails always on,
skip the delay if the regulator supply was already enabled. In this
case the regulator is either always on, was on by default at power up,
or was left on by some other driver, such as the I2C OF component
prober. Either way the controller has had ample time to initialize.

Fixes: 6696777c6506 ("Input: add driver for Elan I2C/SMbus touchpad")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1:
- Delay only if the regulator was previously disabled / turned off
- Link to v1
  https://lore.kernel.org/all/20241001093815.2481899-1-wenst@chromium.org/
---
 drivers/input/mouse/elan_i2c_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index f93dd545d66b..db48d7ef8357 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -47,6 +47,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)
@@ -1219,6 +1221,7 @@ static int elan_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct elan_tp_data *data;
 	unsigned long irqflags;
+	bool supply_was_enabled;
 	int error;
 
 	if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_I2C) &&
@@ -1250,6 +1253,8 @@ static int elan_probe(struct i2c_client *client)
 	if (IS_ERR(data->vcc))
 		return dev_err_probe(dev, PTR_ERR(data->vcc), "Failed to get 'vcc' regulator\n");
 
+	supply_was_enabled = regulator_is_enabled(data->vcc);
+
 	error = regulator_enable(data->vcc);
 	if (error) {
 		dev_err(dev, "Failed to enable regulator: %d\n", error);
@@ -1263,6 +1268,9 @@ static int elan_probe(struct i2c_client *client)
 		return error;
 	}
 
+	if (!supply_was_enabled)
+		msleep(ETP_POWER_ON_DELAY);
+
 	/* Make sure there is something at this address */
 	error = i2c_smbus_read_byte(client);
 	if (error < 0) {
@@ -1406,11 +1414,16 @@ static int elan_resume(struct device *dev)
 	int error;
 
 	if (!device_may_wakeup(dev)) {
+		bool supply_was_enabled = regulator_is_enabled(data->vcc);
+
 		error = regulator_enable(data->vcc);
 		if (error) {
 			dev_err(dev, "error %d enabling regulator\n", error);
 			goto err;
 		}
+
+		if (!supply_was_enabled)
+			msleep(ETP_POWER_ON_DELAY);
 	}
 
 	error = elan_set_power(data, true);
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 2/7] HID: i2c-hid-of: skip post-power-on delay if already powered on

[Chen-Yu Tsai]

On some devices the HID device is powered from an always-on power rail,
or the power rail has been left on by either POR defaults or the
bootloader. By the time the driver probes, the device most certainly
has finished initializing. There is no need for the delay.

In such designs, the system integrators tend to work around the delay
to avoid the boot time penalty by simply omitting it from the device
tree. This is undesired, as the device tree is not fully describing
the hardware.

Instead, check if the regulator supplies are all enabled, and skip
the post-power-on delay if that is the case.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/hid/i2c-hid/i2c-hid-of.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 59393d71ddb9..70afdfb207ac 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -29,6 +29,7 @@
 #include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/regulator/consumer.h>
+#include <linux/string_choices.h>
 
 #include "i2c-hid.h"
 
@@ -46,8 +47,12 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
 {
 	struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
 	struct device *dev = &ihid_of->client->dev;
+	bool supply_was_enabled = true;
 	int ret;
 
+	for (unsigned int i = 0; i < ARRAY_SIZE(ihid_of->supplies); i++)
+		supply_was_enabled &= regulator_is_enabled(ihid_of->supplies[i].consumer);
+
 	ret = regulator_bulk_enable(ARRAY_SIZE(ihid_of->supplies),
 				    ihid_of->supplies);
 	if (ret) {
@@ -55,7 +60,8 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
 		return ret;
 	}
 
-	if (ihid_of->post_power_delay_ms)
+	dev_dbg(dev, "supply was %s.\n", str_on_off(supply_was_enabled));
+	if (!supply_was_enabled && ihid_of->post_power_delay_ms)
 		msleep(ihid_of->post_power_delay_ms);
 
 	gpiod_set_value_cansleep(ihid_of->reset_gpio, 0);
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 3/7] i2c: of-prober: skip post-power-on delay if already powered on

[Chen-Yu Tsai]

On some devices the I2C component is powered from an always-on power
rail, or the power rail has been left on by either POR defaults or
the bootloader. By the time the prober probes the device, the device
most certainly has finished initializing and can respond. There is no
need for the delay.

In such designs, the system integrators tend to work around the delay
to avoid the boot time penalty by simply omitting it from the device
tree and the component prober. This is undesired, as the device tree
is not fully describing the hardware.

Instead, check if the regulator supplies are all enabled, and skip
the post-power-on delay if that is the case.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/i2c/i2c-core-of-prober.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index 6a82b03809d4..f274e260353c 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -18,6 +18,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/stddef.h>
+#include <linux/string_choices.h>
 
 /*
  * Some devices, such as Google Hana Chromebooks, are produced by multiple
@@ -219,19 +220,25 @@ static void i2c_of_probe_simple_put_supply(struct i2c_of_probe_simple_ctx *ctx)
 
 static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
 {
+	bool supply_was_on;
 	int ret;
 
 	if (!ctx->supply)
 		return 0;
 
-	dev_dbg(dev, "Enabling regulator supply \"%s\"\n", ctx->opts->supply_name);
+	supply_was_on = regulator_is_enabled(ctx->supply);
+
+	dev_dbg(dev, "Enabling regulator supply \"%s\" (was %s)\n", ctx->opts->supply_name,
+		str_on_off(supply_was_on));
 
 	ret = regulator_enable(ctx->supply);
 	if (ret)
 		return ret;
 
-	if (ctx->opts->post_power_on_delay_ms)
+	if (!supply_was_on && ctx->opts->post_power_on_delay_ms) {
+		dev_dbg(dev, "Waiting after enabling regulator\n");
 		msleep(ctx->opts->post_power_on_delay_ms);
+	}
 
 	return 0;
 }
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 4/7] i2c: of-prober: Defer regulator_disable() on successful probe in simple helper

[Chen-Yu Tsai]

When a I2C component is found, it's device node is immediately enabled.
This triggers device creation and driver binding. The prober will hold
the regulator enable reference across this part. If the driver probes
synchronously, then it happens within this window. On the other hand,
if the driver probes asynchronously, there is high chance that it
happens after the prober's cleanup function was called, in which case
the regulator would have been disabled when the driver's probe function
is called. This would then require the driver to wait 100 ms for the
hardware to reinitialize, even if the probe function was just a split
second late and the regulator was disabled a few milliseconds ago.

Recently, some of the drivers for the component that are targeted by the
I2C OF component prober gained the ability to skip waiting for hardware
initialization if the regulator was left enabled. This happens when the
PMIC has them on by default, or if the component prober left them on
after probing the component.

Wait a bit of time before dropping the enable refcount on our end so
that the actual driver has the opportunity to catch and increase the
refcount on their end. The 100 ms delay was arbitrarily chosen.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/i2c/i2c-core-of-prober.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
index f274e260353c..17495125467e 100644
--- a/drivers/i2c/i2c-core-of-prober.c
+++ b/drivers/i2c/i2c-core-of-prober.c
@@ -243,11 +243,23 @@ static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_o
 	return 0;
 }
 
-static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
+static void i2c_of_probe_simple_disable_regulator(struct device *dev,
+						  struct i2c_of_probe_simple_ctx *ctx,
+						  bool defer_disable)
 {
 	if (!ctx->supply)
 		return;
 
+	/*
+	 * Wait a bit of time for async drivers to probe and increase the
+	 * regulator enable count. This allows the drivers to check and
+	 * skip waiting for re-initialization.
+	 */
+	if (defer_disable) {
+		dev_dbg(dev, "Deferring regulator disable\n");
+		msleep(100);
+	}
+
 	dev_dbg(dev, "Disabling regulator supply \"%s\"\n", ctx->opts->supply_name);
 
 	regulator_disable(ctx->supply);
@@ -364,7 +376,7 @@ int i2c_of_probe_simple_enable(struct device *dev, struct device_node *bus_node,
 	return 0;
 
 out_disable_regulator:
-	i2c_of_probe_simple_disable_regulator(dev, ctx);
+	i2c_of_probe_simple_disable_regulator(dev, ctx, false);
 out_put_gpiod:
 	i2c_of_probe_simple_put_gpiod(ctx);
 out_put_supply:
@@ -409,7 +421,7 @@ void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
 	i2c_of_probe_simple_disable_gpio(dev, ctx);
 	i2c_of_probe_simple_put_gpiod(ctx);
 
-	i2c_of_probe_simple_disable_regulator(dev, ctx);
+	i2c_of_probe_simple_disable_regulator(dev, ctx, true);
 	i2c_of_probe_simple_put_supply(ctx);
 }
 EXPORT_SYMBOL_NS_GPL(i2c_of_probe_simple_cleanup, "I2C_OF_PROBER");
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 5/7] platform/chrome: of_hw_prober: Add delay for hana trackpads

[Chen-Yu Tsai]

Up until now, the MT8173 elm/hana device tree has set the dedicated
regulator supplying the trackpad as always-on, simply because the Elan
driver was missing proper delays. As a result the delay for the
Synaptics trackpad was also omitted, as it was not strictly required
under such a model and delayed the availability of the trackpad to the
user.

The Elan driver recently gained proper delays after power-up, with
adaptive skipping of the delay if the regulator was originally
on. The I2C HID driver and I2C OF component prober library gained
similar adaptive delay skipping. The device tree will be fixed to have
the regulator not be always on, and proper post-power-on delay time
added to the I2C HID device.

Also add the post-power-on delay to the ChromeOS OF component prober,
so that if the regulator is off at the time of probing, the prober knows
to wait for the hardware to initialize.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/platform/chrome/chromeos_of_hw_prober.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/chromeos_of_hw_prober.c b/drivers/platform/chrome/chromeos_of_hw_prober.c
index 8562a0e89dc6..54d8941617e2 100644
--- a/drivers/platform/chrome/chromeos_of_hw_prober.c
+++ b/drivers/platform/chrome/chromeos_of_hw_prober.c
@@ -70,10 +70,8 @@ static const struct chromeos_i2c_probe_data chromeos_i2c_probe_hana_trackpad = {
 		/*
 		 * ELAN trackpad needs 2 ms for H/W init and 100 ms for F/W init.
 		 * Synaptics trackpad needs 100 ms.
-		 * However, the regulator is set to "always-on", presumably to
-		 * avoid this delay. The ELAN driver is also missing delays.
 		 */
-		.post_power_on_delay_ms = 0,
+		.post_power_on_delay_ms = 110,
 	},
 };
 
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 6/7] arm64: dts: mediatek: mt8173-elm-hana: Unmark trackpad supply as always-on

[Chen-Yu Tsai]

Up until now, the MT8173 elm/hana device tree has set the dedicated
regulator supplying the trackpad as always-on, simply because the Elan
driver was missing proper delays. As a result the delay for the
Synaptics trackpad was also omitted, as it was not strictly required
under such a model and delayed the availability of the trackpad to the
user.

The Elan driver recently gained proper delays after power up, with
opportunistic skipping of the delay when the regulator was originally
on. The I2C HID driver gained similar opportunistic delay skipping.
So has the I2C OF component prober library.

Now fix the device tree to have the regulator not be always on, and
let the I2C HID device have the correct post-power-on delay time.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi | 8 +-------
 arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi      | 1 -
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
index 1004eb8ea52c..b9e311fcd9a0 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
@@ -62,13 +62,7 @@ trackpad2: trackpad@2c {
 		pinctrl-0 = <&trackpad_irq>;
 		reg = <0x2c>;
 		hid-descr-addr = <0x0020>;
-		/*
-		 * The trackpad needs a post-power-on delay of 100ms,
-		 * but at time of writing, the power supply for it on
-		 * this board is always on. The delay is therefore not
-		 * added to avoid impacting the readiness of the
-		 * trackpad.
-		 */
+		post-power-on-delay-ms = <100>;
 		vdd-supply = <&mt6397_vgp6_reg>;
 		wakeup-source;
 		status = "fail-needs-probe";
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index a0573bc359fb..6b9f47f515c7 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -1093,7 +1093,6 @@ mt6397_vgp6_reg: ldo_vgp6 {
 				regulator-min-microvolt = <3300000>;
 				regulator-max-microvolt = <3300000>;
 				regulator-enable-ramp-delay = <218>;
-				regulator-always-on;
 			};
 
 			mt6397_vibr_reg: ldo_vibr {
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH v2 7/7] arm64: dts: mediatek: mt8192-asurada-spherion: Add Synaptics trackpad's supply

[Chen-Yu Tsai]

The Synaptics trackpad, like the Elan trackpad option, is fed from the
system 3.3V power rail. Add it to the trackpad device node.

Also add the correct post-power-on delay, even though in practice it is
not required. The Synaptics trackpad requires 100ms after power-on (or
deasserting the reset, whichever comes later) to fully initialize. The
power is always on and the reset pin is not routed out, so the
implementation could try skipping the delay.

Cc: <stable+noautosel@kernel.org> # Without driver changes only lengthens probe time
Fixes: 925ebc0cd55c ("arm64: dts: mt8192-asurada-spherion: Add Synaptics trackpad support")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
I think this shouldn't be backported, as backporting it without the
driver enhancements just delays the trackpad probing with no real
gains.
---
 arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts b/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
index 163960f58db5..147a8e9a3a71 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada-spherion-r0.dts
@@ -90,6 +90,8 @@ trackpad@2c {
 		hid-descr-addr = <0x20>;
 		interrupts-extended = <&pio 15 IRQ_TYPE_LEVEL_LOW>;
 		wakeup-source;
+		vdd-supply = <&pp3300_u>;
+		post-power-on-delay-ms = <100>;
 		status = "fail-needs-probe";
 	};
 };
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2 3/7] i2c: of-prober: skip post-power-on delay if already powered on

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The integer return value of `regulator_is_enabled()` (which can be a negative error code) is implicitly cast to or bitwise-ANDed with `bool` variables, leading to incorrect evaluations that inadvertently skip required hardware power-on delays.
- [Medium] Shared regulator logic flaw and TOCTOU race allow skipping required power-on delays.

Pre-existing issues:
- [High] Optional regulator supplies missing from the device tree incorrectly cause the prober to fail, contradicting the intended optional behavior.
--

commit 7072efff03a9502c14f9b2eb5a3863213e3992ef
Author: Chen-Yu Tsai <wenst@chromium.org>

i2c: of-prober: skip post-power-on delay if already powered on

This commit modifies the I2C OF prober to check if regulator supplies are
already enabled before applying a post-power-on delay. If the supply is
already active, it skips the delay to avoid unnecessary boot time penalties.

> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> index 6a82b03809d4f..f274e260353cd 100644
> --- a/drivers/i2c/i2c-core-of-prober.c
> +++ b/drivers/i2c/i2c-core-of-prober.c

[ ... ]

> @@ -219,19 +220,25 @@ static void i2c_of_probe_simple_put_supply(struct i2c_of_probe_simple_ctx *ctx)
>  
>  static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
>  {
> +	bool supply_was_on;
>  	int ret;
>  
>  	if (!ctx->supply)
>  		return 0;
>  
> -	dev_dbg(dev, "Enabling regulator supply \"%s\"\n", ctx->opts->supply_name);
> +	supply_was_on = regulator_is_enabled(ctx->supply);

[Severity: High]
Could this introduce a regression by skipping required hardware power-on
delays if regulator_is_enabled() fails?

The integer return value of regulator_is_enabled() can be a negative error
code, but it is implicitly cast to the bool variable supply_was_on without
error checking. Any non-zero integer, including negative values, becomes true.

If regulator_is_enabled() returns an error, the code will consider the supply
as already on, causing the device to skip the mandatory post-power-on delay
and fail to probe because it has not finished initializing.

I noticed similar patterns in other drivers:
- drivers/hid/i2c-hid/i2c-hid-of.c:i2c_hid_of_power_up()
- drivers/input/mouse/elan_i2c_core.c:elan_probe()
- drivers/input/mouse/elan_i2c_core.c:elan_resume()

[Severity: Medium]
Is there a potential regression here regarding a shared regulator TOCTOU race
condition?

If two devices share a regulator and probe concurrently, Driver A might enable
the regulator. When Driver B checks regulator_is_enabled(), it returns true.
Driver B will then skip its required power-on delay and attempt to communicate
with the device.

If Driver A enabled the regulator less than the required delay time ago,
Driver B will access its device before the hardware has initialized, leading
to probe failure or I2C communication errors.

> +
> +	dev_dbg(dev, "Enabling regulator supply \"%s\" (was %s)\n", ctx->opts->supply_name,
> +		str_on_off(supply_was_on));
>  
>  	ret = regulator_enable(ctx->supply);
>  	if (ret)
>  		return ret;
>  
> -	if (ctx->opts->post_power_on_delay_ms)
> +	if (!supply_was_on && ctx->opts->post_power_on_delay_ms) {
> +		dev_dbg(dev, "Waiting after enabling regulator\n");
>  		msleep(ctx->opts->post_power_on_delay_ms);
> +	}
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed optional regulator supplies missing
from the device tree incorrectly cause the prober to fail, contradicting the
intended optional behavior.

drivers/i2c/i2c-core-of-prober.c:i2c_of_probe_simple_get_supply() {
    ...
	supply = of_regulator_get_optional(dev, node, supply_name);
	if (IS_ERR(supply)) {
		return dev_err_probe(dev, PTR_ERR(supply),
				     "Failed to get regulator supply \"%s\" from %pOF\n",
				     supply_name, node);
	}
    ...
}

of_regulator_get_optional() returns -ENODEV when the supply is not in the
device tree. The function checks IS_ERR(supply) and propagates the error,
failing the probe unconditionally on hardware platforms where the device tree
omits the optional regulator.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=3

Re: [PATCH v2 1/7] Input: elan_i2c - Wait for initialization after enabling regulator supply

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The return value of `regulator_is_enabled()` is implicitly cast to `bool`, causing negative error codes to evaluate as true and incorrectly bypassing the required power-on delay.
- [High] Skipping the power-on delay for shared regulators is susceptible to unsafe power domain assumptions and TOCTOU races, leading to bypassed hardware initialization.
--

commit 62ef124436c7ca613cd0f8d922094f42cdd2a0b8
Author: Chen-Yu Tsai <wenst@chromium.org>

Input: elan_i2c - Wait for initialization after enabling regulator supply

This commit adds proper delays after enabling the regulator supply for Elan
trackpad controllers, which require time for hardware and firmware to
initialize. It attempts to avoid boot time impact by skipping the delay if
the regulator was already enabled.

> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index fee1796da3d08..110c662cef96f 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c

[ ... ]

> @@ -1187,6 +1189,7 @@ static int elan_probe(struct i2c_client *client)
>  	struct device *dev = &client->dev;
>  	struct elan_tp_data *data;
>  	unsigned long irqflags;
> +	bool supply_was_enabled;
>  	int error;
>  
>  	if (IS_ENABLED(CONFIG_MOUSE_ELAN_I2C_I2C) &&
> @@ -1218,6 +1221,8 @@ static int elan_probe(struct i2c_client *client)
>  	if (IS_ERR(data->vcc))
>  		return dev_err_probe(dev, PTR_ERR(data->vcc), "Failed to get 'vcc' regulator\n");
>  
> +	supply_was_enabled = regulator_is_enabled(data->vcc);

[Severity: High]
In elan_probe(), regulator_is_enabled() returns an int, which can include
negative error codes like -EPROBE_DEFER or -EREMOTEIO. Will assigning this to
the bool supply_was_enabled cause negative error codes to evaluate as true?

If this happens, it looks like it would incorrectly bypass the power-on delay
and lead to a permanent -ENXIO probe failure later rather than allowing a
proper deferral.

> +
>  	error = regulator_enable(data->vcc);
>  	if (error) {
>  		dev_err(dev, "Failed to enable regulator: %d\n", error);
> @@ -1231,6 +1236,9 @@ static int elan_probe(struct i2c_client *client)
>  		return error;
>  	}
>  
> +	if (!supply_was_enabled)
> +		msleep(ETP_POWER_ON_DELAY);

[Severity: High]
Is it safe to assume that a regulator that is already enabled has been powered
on for a sufficient amount of time?

If the trackpad shares a regulator with another device (like a touchscreen)
that just finished its own probe milliseconds earlier, the regulator would
already be enabled, but the trackpad hardware wouldn't be fully initialized.

Could this cause elan_probe() to skip the required hardware initialization
delay and fail the subsequent i2c_smbus_read_byte() check?

[ ... ]

> @@ -1374,11 +1382,16 @@ static int elan_resume(struct device *dev)
>  	int error;
>  
>  	if (!device_may_wakeup(dev)) {
> +		bool supply_was_enabled = regulator_is_enabled(data->vcc);

[Severity: High]
In elan_resume(), could the same boolean cast issue happen if
regulator_is_enabled() encounters a transient failure during system resume,
causing the resume delay to be skipped?

> +
>  		error = regulator_enable(data->vcc);
>  		if (error) {
>  			dev_err(dev, "error %d enabling regulator\n", error);
>  			goto err;
>  		}
> +
> +		if (!supply_was_enabled)
> +			msleep(ETP_POWER_ON_DELAY);

[Severity: High]
Could the same shared-regulator timing race occur during the system resume
path, skipping the initialization delay if another device on the same shared
power domain resumed just before the trackpad?

>  	}
>  
>  	error = elan_set_power(data, true);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=1

Re: [PATCH v2 2/7] HID: i2c-hid-of: skip post-power-on delay if already powered on

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bitwise AND assignment (`&=`) between a `bool` and the `int` return value of `regulator_is_enabled()` causes pseudo-random evaluation based on the least significant bit (LSB) of the integer, potentially skipping mandatory delays.
- [High] Skipping the post-power-on delay based solely on the current state of `regulator_is_enabled()` introduces a race condition for devices sharing a power rail.
--

commit 14324865d2e3b5ccaf90d6ff025e907befb17313
Author: Chen-Yu Tsai <wenst@chromium.org>

HID: i2c-hid-of: skip post-power-on delay if already powered on

This patch checks if the regulator supplies for an i2c-hid device are
already enabled during probe. If all supplies are already on, it skips
the post-power-on delay to save boot time.

> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -46,8 +47,12 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
>  {
>  	struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
>  	struct device *dev = &ihid_of->client->dev;
> +	bool supply_was_enabled = true;
>  	int ret;
>  
> +	for (unsigned int i = 0; i < ARRAY_SIZE(ihid_of->supplies); i++)
> +		supply_was_enabled &= regulator_is_enabled(ihid_of->supplies[i].consumer);

[Severity: High]
Could this bitwise operation cause a regression by inadvertently evaluating
the least significant bit of the return value?

The function regulator_is_enabled() returns an int (positive state, 0, or 
a negative error). In C, performing a bitwise AND (&=) between a bool 
(promoted to integer 1) and an int isolates the least significant bit.

For example, if regulator_is_enabled() returns a negative error code like 
-ENODEV (-19), the least significant bit is 1, causing supply_was_enabled 
to evaluate to true despite the error. If it returns an even positive 
state like 2, the least significant bit is 0, making it false.

[ ... ]

>  	if (ret) {
>  		dev_warn(dev, "Failed to enable supplies: %d\n", ret);
>  		return ret;
>  	}
>  
> -	if (ihid_of->post_power_delay_ms)
> +	dev_dbg(dev, "supply was %s.\n", str_on_off(supply_was_enabled));
> +	if (!supply_was_enabled && ihid_of->post_power_delay_ms)
>  		msleep(ihid_of->post_power_delay_ms);

[Severity: High]
Is it safe to skip this delay based solely on whether the regulator is
currently enabled?

If two devices (e.g. a touchscreen and a trackpad) share the same 
non-always-on regulator and probe asynchronously:

Device 1 starts probing, calls regulator_bulk_enable(), and begins
waiting its post-power delay.

Device 2 concurrently probes in i2c_hid_of_power_up(), sees 
regulator_is_enabled() is true, and skips its post_power_delay_ms here.

Device 2 would then immediately issue I2C transactions to a newly powered
device that might not have finished initializing, potentially resulting
in NACKs and a probe regression. 

Does the regulator framework provide a way to ensure the rail has been on
for the required duration, rather than just currently enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=2

Re: [PATCH v2 4/7] i2c: of-prober: Defer regulator_disable() on successful probe in simple helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The prober unconditionally sleeps for 100ms during the cleanup path even when no matching I2C component is found, unnecessarily delaying system boot.
--

commit 772a8f5b00b1078ef6740a0746df36699760f2f2
Author: Chen-Yu Tsai <wenst@chromium.org>

i2c: of-prober: Defer regulator_disable() on successful probe in simple helper

This patch defers regulator disabling in the I2C OF component prober by adding
a 100ms delay. The delay allows asynchronously probing drivers to catch and
increase the regulator enable count, avoiding hardware re-initialization.

> diff --git a/drivers/i2c/i2c-core-of-prober.c b/drivers/i2c/i2c-core-of-prober.c
> index f274e260353cd..17495125467ed 100644
> --- a/drivers/i2c/i2c-core-of-prober.c
> +++ b/drivers/i2c/i2c-core-of-prober.c
> @@ -243,11 +243,23 @@ static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_o
>  	return 0;
>  }
>  
> -static void i2c_of_probe_simple_disable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx)
> +static void i2c_of_probe_simple_disable_regulator(struct device *dev,
> +						  struct i2c_of_probe_simple_ctx *ctx,
> +						  bool defer_disable)
>  {
>  	if (!ctx->supply)
>  		return;
>  
> +	/*
> +	 * Wait a bit of time for async drivers to probe and increase the
> +	 * regulator enable count. This allows the drivers to check and
> +	 * skip waiting for re-initialization.
> +	 */
> +	if (defer_disable) {
> +		dev_dbg(dev, "Deferring regulator disable\n");
> +		msleep(100);
> +	}
> +

[ ... ]

> @@ -409,7 +421,7 @@ void i2c_of_probe_simple_cleanup(struct device *dev, void *data)
>  	i2c_of_probe_simple_disable_gpio(dev, ctx);
>  	i2c_of_probe_simple_put_gpiod(ctx);
>  
> -	i2c_of_probe_simple_disable_regulator(dev, ctx);
> +	i2c_of_probe_simple_disable_regulator(dev, ctx, true);

[Severity: Medium]
Does this unconditionally sleep for 100ms even when no matching I2C component
is found?

If i2c_of_probe_component() iterates over potential components but finds none,
it calls ops->cleanup(dev, ctx), which invokes i2c_of_probe_simple_cleanup().

Because defer_disable is hardcoded to true here,
i2c_of_probe_simple_disable_regulator() will execute the 100ms sleep.

Could this result in an unnecessary 100ms boot delay on systems missing the
probed component, since no async driver will ever probe in this failure path?

>  	i2c_of_probe_simple_put_supply(ctx);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=4

Re: [PATCH v2 5/7] platform/chrome: of_hw_prober: Add delay for hana trackpads

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The result of regulator_is_enabled() is assigned to or bitwise-ANDed with a boolean variable, causing negative error codes to incorrectly evaluate to true and skip required hardware initialization delays.
--

commit 1aa1abec8abc658e89bd1583fb36c5b23bda7cf8
Author: Chen-Yu Tsai <wenst@chromium.org>

platform/chrome: of_hw_prober: Add delay for hana trackpads

Up until now, the MT8173 elm/hana device tree has set the dedicated
regulator supplying the trackpad as always-on, simply because the Elan
driver was missing proper delays. As a result the delay for the
Synaptics trackpad was also omitted, as it was not strictly required
under such a model and delayed the availability of the trackpad to the
user.

> The Elan driver recently gained proper delays after power-up, with
> adaptive skipping of the delay if the regulator was originally
> on. The I2C HID driver and I2C OF component prober library gained
> similar adaptive delay skipping. The device tree will be fixed to have
> the regulator not be always on, and proper post-power-on delay time
> added to the I2C HID device.

[Severity: High]
Does the adaptive skipping logic mentioned here safely handle negative error
codes from regulator_is_enabled()?

Looking at the implementations of this adaptive skipping,
regulator_is_enabled() can return a negative error code (e.g., -EIO, -EBUSY)
on failure, but this result is being directly assigned to or bitwise-ANDed
with a boolean.

For example, in drivers/hid/i2c-hid/i2c-hid-of.c:i2c_hid_of_power_up():

    for (unsigned int i = 0; i < ARRAY_SIZE(ihid_of->supplies); i++)
        supply_was_enabled &= regulator_is_enabled(ihid_of->supplies[i].consumer);

Since supply_was_enabled is promoted to int for the bitwise AND, does this
outcome depend on the parity of the error code? It looks like odd errors
(like -EIO) will yield true (skipping the delay), while even errors (like
-EBUSY) will yield false.

Similarly, in drivers/i2c/i2c-core-of-prober.c:
i2c_of_probe_simple_enable_regulator():

    supply_was_on = regulator_is_enabled(ctx->supply);

And in drivers/input/mouse/elan_i2c_core.c:elan_probe() and elan_resume():

    supply_was_enabled = regulator_is_enabled(data->vcc);

Since any non-zero integer evaluates to true in C, assigning a negative error
code directly to a bool means all errors will incorrectly signal that the
regulator was already on.

Could this cause a regression where a transient I2C or hardware error when
querying the regulator state makes the driver incorrectly skip required
hardware initialization delays and fail the device probe?

> Also add the post-power-on delay to the ChromeOS OF component prober,
> so that if the regulator is off at the time of probing, the prober knows
> to wait for the hardware to initialize.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703115601.1323491-1-wenst@chromium.org?part=5

Re: [PATCH v2 1/7] Input: elan_i2c - Wait for initialization after enabling regulator supply

[Dmitry Torokhov]

Hi Chen-Yu,

On Fri, Jul 03, 2026 at 07:55:54PM +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. To avoid impacting
> the boot time of existing devices that have the power rails always on,
> skip the delay if the regulator supply was already enabled. In this
> case the regulator is either always on, was on by default at power up,
> or was left on by some other driver, such as the I2C OF component
> prober. Either way the controller has had ample time to initialize.

Unfortunately we do not know that [it had ample time]. For this code be
reliable we need to record the time at which given regulator was turned
on and then execute/adjust the delay as needed. Until we have it we need
to assume the regulator was enabled at the time of regulator_enable()
call.

I am not concerned with increased boot time too much given the driver is
set up for asynchronous probing.

Thanks.

-- 
Dmitry