Re: [PATCH v6 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] Skipping the post-power delay when regulators are already enabled removes the implicit minimum reset pulse width (T_reset), which can leave devices in an undefined hardware state.
--

commit b221f2dda3539367a70e306f0fab88390cca40e7
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 an unconditional sleep with
regulator_bulk_enable_and_wait(). This skips the post-power delay if the
regulators have been enabled long enough.

> 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);
> -
>  	gpiod_set_value_cansleep(ihid_of->reset_gpio, 0);

[Severity: High]
Does skipping the post-power delay when regulators are already enabled remove
the implicit minimum reset pulse width (T_reset)?

The driver unconditionally asserts the reset line initially in
i2c_hid_of_probe():

    ihid_of->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);

If a device's regulator is shared or left enabled by the bootloader,
regulator_bulk_enable_and_wait() skips the delay. This causes the reset line
to be de-asserted immediately here, creating an extremely short pulse.

Could this regression leave the HID device in an undefined hardware state and
cause it to fail to probe or become unresponsive?

>  	if (ihid_of->post_reset_delay_ms)
>  		msleep(ihid_of->post_reset_delay_ms);

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