Re: [PATCH v3 1/9] regulator: core: Add "enable and wait" functions

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Jul 21, 2026 at 03:52:15PM +0800, Chen-Yu Tsai wrote:
> In device power sequencing and initialization use cases, it is common
> for the driver to enable the regulator and then wait for a certain
> period of time to pass before continuing.
> 
> In cases where the regulator supply is always on, or has been turned on
> or left on by another consumer, the driver could shorten the delay or
> skip it altogether, provided that enough time has already passed since
> the regulator was _actually_ turned on.
> 
> Tracking this requires support from the regulator core. Introduce a
> "last turned on" timestamp field to the regulator device, and "enable
> and wait" functions to the single and bulk regulator consumer APIs.
> The existing "enable without wait" functions are then converted to
> macros that expand to the new functions.
> 
> One case in particular is not optimized yet: a regulator left on either
> by hardware reset default or by the bootloader, but does not have the
> "regulator-boot-on" property set. As the enable timestamp only gets
> updated when enabled by a consumer or by the core, the first enablement
> always needs to wait.

...

> -/* locks held by regulator_enable() */
> -static int _regulator_enable(struct regulator *regulator)
> +/* locks held by regulator_enable_and_wait() */
> +static int _regulator_enable_and_wait(struct regulator *regulator, unsigned int wait_us)

Hmm...
This comment a bit confusing, perhaps adding some lockdep annotations help?

...

> +	if (wait_us) {
> +		ktime_t end = ktime_add_us(rdev->last_on, wait_us);
> +		s64 remaining = ktime_us_delta(end, ktime_get_boottime());
> +
> +		if (remaining > 0)
> +			fsleep(remaining);

This style is discouraged as it makes maintenance harder. Better

		s64 remaining;

		remaining = ktime_us_delta(end, ktime_get_boottime());
		if (remaining > 0)
			fsleep(remaining);

> +	}

...

>  	/* private: Internal use */
>  	int ret;
> +	unsigned int wait_us;

If you want to make it more private (the above is only for kernel-doc) add
__private annotation that will affect how sparse will check this.

...

> -int __must_check regulator_enable(struct regulator *regulator);
> +int __must_check regulator_enable_and_wait(struct regulator *regulator, unsigned int ms);

ms or us? Please, double check all units.

-- 
With Best Regards,
Andy Shevchenko