Re: [PATCH v5 3/7] 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 2 potential issue(s) to consider:
- [High] The patch relies on a buggy core regulator API, resulting in incorrect concurrent wait times for regulator sequencing that truncates the required hardware initialization delay.
- [High] The reset assertion pulse during probe may be too short if regulators are already enabled, potentially violating hardware reset timing requirements.
--

--- Patch [3]: [PATCH v5 3/7] HID: i2c-hid-of: skip post-power-on delay if powered on sufficiently long ---
commit 3ba58a3bbf6e0eecad8d9882947c8fc478c04ddc
Author: Chen-Yu Tsai <wenst@chromium.org>
Subject: HID: i2c-hid-of: skip post-power-on delay if powered on sufficiently long

This patch switches the driver to use regulator_bulk_enable_and_wait() to
avoid unnecessary boot delays if the device has been powered on long enough.
It omits the unconditional post power delay in favor of measuring the time
since the regulators were actually enabled.

> 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);

[Severity: High]
Will this correctly wait for all regulator supplies to stabilize?

When looking at the implementation of regulator_bulk_enable_and_wait()
in drivers/regulator/core.c, the setup loop contains:

for (i = 0; i < num_consumers; i++) {
        ACCESS_PRIVATE(consumers, wait_us) = wait_us;
        async_schedule_domain(regulator_bulk_enable_async,
                              &consumers[i], &async_domain);
}

Since ACCESS_PRIVATE(consumers, wait_us) expands to consumers[0].wait_us
without indexing by i, only the first regulator receives the configured wait
time.

If the first regulator was already enabled by the bootloader (meaning its wait
time evaluates to 0) and the second regulator is newly enabled, won't the
function return instantly and skip the required hardware stabilization delay
for the second supply?

>  	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]
Could skipping this delay cause the reset assertion pulse to be too short?

In i2c_hid_of_probe(), the reset GPIO is asserted just before calling
i2c_hid_of_power_up(). If the regulators are already enabled,
regulator_bulk_enable_and_wait() will return almost instantly.

Since the unconditional delay is now omitted, the code reaches
gpiod_set_value_cansleep() immediately. Does this reduce the reset assertion
pulse width from the previously guaranteed post_power_delay_ms down to just
a few microseconds?

Could this fail to properly reset I2C HID devices that require a longer
minimum reset pulse?

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

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