Re: [PATCH v7 3/8] HID: i2c-hid-of: skip post-power-on delay if powered on sufficiently long

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: i2c-hid-of: skipped post_power_delay_ms in i2c_hid_of_power_up()
--

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

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

This commit replaces regulator_bulk_enable() and the unconditional msleep() in
i2c_hid_of_power_up() with regulator_bulk_enable_and_wait() so that the
post-power-on delay is skipped when the regulator supplies have already been
enabled for at least post_power_delay_ms.

> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> index 59393d71ddb9d..fdaad451e7102 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c

[ ... ]

> @@ -48,16 +49,14 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
>  	struct device *dev = &ihid_of->client->dev;
>  	int ret;
>  
> -	ret = regulator_bulk_enable(ARRAY_SIZE(ihid_of->supplies),
> -				    ihid_of->supplies);
> +	ret = regulator_bulk_enable_and_wait(ARRAY_SIZE(ihid_of->supplies),
> +					     ihid_of->supplies,
> +					     ihid_of->post_power_delay_ms * USEC_PER_MSEC);
>  	if (ret) {
>  		dev_warn(dev, "Failed to enable supplies: %d\n", ret);
>  		return ret;
>  	}
>  
> -	if (ihid_of->post_power_delay_ms)
> -		msleep(ihid_of->post_power_delay_ms);
> -

[Severity: High]
As reported by Sashiko on v6 [1], could replacing the unconditional
msleep(ihid_of->post_power_delay_ms) with regulator_bulk_enable_and_wait() in
i2c_hid_of_power_up() cause initialization regressions on existing Device Tree
platforms where ihid_of->supplies are dummy or always-on regulators whose
last_on timestamp was recorded during early boot, even though the HID device
itself just exited reset or powered up?

When the delay is skipped, __i2c_hid_core_probe() or i2c_hid_core_resume()
immediately issues I2C transfers (such as i2c_hid_probe_address(),
i2c_hid_fetch_hid_descriptor(), or i2c_hid_set_power()) before the controller
firmware has finished booting, which can fail with -ENXIO.

There appear to be three in-tree Device Tree patterns affected by this:

1. Pinctrl-driven reset deassertion at driver bind time:
Because Documentation/devicetree/bindings/input/hid-over-i2c.yaml does not
define reset-gpios or post-reset-deassert-delay-ms (which i2c_hid_of_probe()
only populates for x86 platform data), several boards hold the controller in
reset at boot via a pin pulldown and drive the reset pin output-high in
pinctrl-0 when pinctrl_bind_pins() runs right before i2c_hid_of_probe(),
relying on post-power-on-delay-ms to wait for the device to boot out of reset.

For example, in arch/arm64/boot/dts/qcom/sdm850-samsung-w737.dts, digitizer@9
omits vdd-supply (using the dummy regulator), sets post-power-on-delay-ms =
<120>, and releases reset in pen_rst_l:

	pen_rst_l: pen-rst-l-state {
		pins = "gpio21";
		function = "gpio";
		bias-disable;
		drive-strength = <2>;

		/*
		 * The pen driver doesn't currently support
		 * driving this reset line.  By specifying
		 * output-high here we're relying on the fact
		 * that this pin has a default pulldown at boot
		 * (which makes sure the pen was in reset if it
		 * was powered) and then we set it high here to
		 * take it out of reset.  Better would be if the
		 * pen driver could control this and we could
		 * remove "output-high" here.
		 */
		output-high;
	};

Similarly, arch/arm64/boot/dts/mediatek/mt8186-corsola-magneton-sku393216.dts
(as well as sku393217.dts and mt8183-kukui-kodama.dtsi / kakadu.dtsi /
jacuzzi-burnet.dts) uses an always-on regulator while touchscreen_pins drives
the reset pin output-high in pinctrl-0:

	touchscreen@10 {
		compatible = "hid-over-i2c";
		reg = <0x10>;
		interrupts-extended = <&pio 12 IRQ_TYPE_LEVEL_LOW>;
		pinctrl-names = "default";
		pinctrl-0 = <&touchscreen_pins>;
		vdd-supply = <&pp3300_s3>;
		post-power-on-delay-ms = <350>;
		hid-descr-addr = <0x0001>;
	};

2. DRM panel followers where the panel driver sequences power and reset:
In arch/arm64/boot/dts/mediatek/mt8186-corsola-starmie-sku1.dts,
touchscreen_himax is a panel follower whose vdd-supply (mt6366_vio18_reg) is
marked regulator-always-on in mt8186-corsola.dtsi:

	touchscreen_himax: touchscreen@4f {
		compatible = "hid-over-i2c";
		reg = <0x4f>;
		interrupts-extended = <&pio 12 IRQ_TYPE_LEVEL_LOW>;
		pinctrl-names = "default";
		pinctrl-0 = <&touchscreen_pins>;
		vdd-supply = <&mt6366_vio18_reg>;
		panel = <&panel>;
		post-power-on-delay-ms = <450>;
		hid-descr-addr = <0x0001>;
	};

Here, the switched analog rails (avdd/avee) and hardware reset GPIO of the
integrated Himax HX83102 TDDI controller are toggled by hx83102_prepare() and
hx83102_unprepare() in drivers/gpu/drm/panel/panel-himax-hx83102.c. When
drm_panel_prepare() triggers ihid_core_panel_follower_work() ->
i2c_hid_core_probe_panel_follower() or i2c_hid_core_resume() ->
i2c_hid_of_power_up(), won't regulator_bulk_enable_and_wait() see the early
boot last_on timestamp of mt6366_vio18_reg and skip the 450 ms delay right
after hx83102_prepare() powered and reset the chip?

3. S3 suspend/resume with hardware-sequenced rails:
On boards using pp3300_s3 (modeled in mt8186-corsola.dtsi as a fixed regulator
with regulator-always-on because it is sequenced by PMIC EXT_PMIC_EN2 during
S3), regulator_bulk_disable() and regulator_bulk_enable_and_wait() do not
update rdev->last_on across S3 suspend and resume while CLOCK_BOOTTIME
advances. On S3 resume in i2c_hid_core_resume() -> i2c_hid_of_power_up(),
won't this skip post_power_delay_ms even though the rail was just re-enabled
by hardware?

[1] https://lore.kernel.org/r/20260901074930.764550-4-wenst@chromium.org

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