Re: [PATCH v2] regulator: core: repeat voltage setting request for stepped regulators

[Romain Gantois <romain.gantois@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]

Hi Jon,

On Tuesday, 29 July 2025 10:28:17 CEST Jon Hunter wrote:
> Hi Romain,
> 
...
> Looking at better closer at the issue, I noticed that it is the
> 'tps62361-vout' regulator that change is causing problem for. On boot
> I see regulator_set_voltage_unlocked() called for this regulator and
> min/max voltage requested is ...
> 
>   regulator regulator.5: min_uV 1000000 max_uV 1350000
> 
> The min delta is 300000, but in this case the delta never reaches 0
> and in fact never converges at all and so remains at 300000.
> 
> Looking at the above, if the delta never changes, then we get stuck
> in the above loop forever because 'new_delta - delta' is always 0
> and this is never greater than 'rdev->constraints->max_uV_step'.
> 
> There are two things that is not clear to me in the above change ...
> 
> 1. Why do we 'new_delta - delta' instead of 'delta - new_delta'?
>     Assuming that we should converge, then I would expect that
>     'new_delta' should be getting smaller as we converge.

Indeed it should. "new_delta - delta" is equal to the increase of voltage
"error". So if this value is positive, it's bad because it means we're
getting further away from the target voltage. Also, if it's negative but
too large, then it means that we're slowly crawling to the target voltage,
which is bad. Currently we do:

```
if (new_delta - delta > max_uV_step)
	give up and return -EWOULDBLOCK
```

but we should be doing:

```
if (new_delta - delta > -max_uV_step)
	give up and return -EWOULDBLOCK
```

which is equivalent to:

```
if (delta - new_delta < max_uV_step)
	give up and return -EWOULDBLOCK
```

> 2. If difference in the delta is greater than then 'max_uV_step'
>     doesn't this imply that we are converging quickly?
> 

Yes, the current logic is indeed flawed.

> I am wondering if we need something like ...
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 8ed9b96518cf..554d83c4af0c 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3884,7 +3884,7 @@ static int regulator_set_voltage_unlocked(struct
> regulator *regulator, new_delta = ret;
> 
>                          /* check that voltage is converging quickly enough */
>  -                       if (new_delta - delta > rdev->constraints->max_uV_step) {
> +                       if (delta - new_delta < rdev->constraints->max_uV_step) {

Yes, that would be correct. Do you want to send the fix yourself, or should I
do it and include your "Suggested-by"?

Thanks for reporting the issue and sorry for the trouble.

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]