Re: [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

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

Hello,

thanks to Martin for looping me in.

On Fri, Nov 06, 2026 at 01:59:55PM +0100, Mike Looijmans wrote:
> This reverts commit 6a7d11efd691 ("regulator: pwm-regulator: Calculate the
> output voltage for disabled PWMs")
> 
> A disabled PWM is not the same as "set to 0%". If the PWM is disabled,
> this could also mean that its output pin is tristated for example.

or even active.

> If the PWM range is limited to something else than 0..100% this causes the
> probe function to fail with messages like:
>  dsp7-pwm: failed to get the current voltage: -ENOTRECOVERABLE
>  pwm-regulator dsp7-supply-pwm: error -ENOTRECOVERABLE: Failed to register regulator pwm-regulator
>  pwm-regulator dsp7-supply-pwm: probe with driver pwm-regulator failed with error -131
> 
> The reason is that regulator_register() calls get_voltage(). The PWM is
> disabled at this point, and then the validity check triggers.
> 
> Anything that calls get_voltage() while the PWM is disabled would get an
> -ENOTRECOVERABLE return value for a pwm-regulator that doesn't allow 0%.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> 
>  drivers/regulator/pwm-regulator.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 7434b6b22d325..f50530c77d92f 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -157,13 +157,6 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
>  
>  	pwm_get_state(drvdata->pwm, &pstate);
>  
> -	if (!pstate.enabled) {
> -		if (pstate.polarity == PWM_POLARITY_INVERSED)
> -			pstate.duty_cycle = pstate.period;
> -		else
> -			pstate.duty_cycle = 0;
> -	}
> -

The status quo (ie. with the if removed in this patch) relies on
undefined behaviour because (as noted in the commit log) the output of a
disabled PWM is unknown. It might be high, or low, or tristate, or
active, or inactive or it might even still toggling. So without further
information about the involved hardware, you cannot say what the PWM
(and so the regulator) does.

However removing that block makes the driver ignore if the PWM is
enabled or not and diagnoses something invented. With a driver
implementing the waveform API (instead of the legacy .apply() and
.get_state()) you get

pstate = { .period = 0, .duty_cycle = 0, polarity = PWM_POLARITY_NORMAL,
.enabled = false, .usage_power = false }

For a driver with .get_state() you might get

pstate = { .period = 100000, .duty_cycle = 100000, polarity = PWM_POLARITY_NORMAL,
.enabled = false, .usage_power = false }

>  	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);

... which is interpreted as fully off and fully on respectively.

I think assuming a disabled regulator when the PWM is disabled is a sane
choice here, but IMHO definitively needs a comment and explict handling.
Something like:

	if (!pstate.enabled)
		/*
		 * We don't know what the PWM does here, it might just
		 * have stopped toggling at the active state. Let's
		 * assume the regulator is off instead of erroring out.
		 */
		return 0;

Technically more correct would be to say "I don't know", but that
probably involves some hassles.

Also note that pwm_get_state() returns the last requested state and not
the actually emitted waveform. (So if pwm_apply*() was called with
state->duty_cycle = 300 ns and the hardware rounds that down to 0
because the smallest duty_cycle the hardware can do it 400 ns,
pwm_get_state() still gives you .duty_cycle = 300.) Also pwm_get_state()
just reports "disabled" for drivers without a .get_state() callback if
pwm_apply*() wasn't called yet. You might want to switch that to
pwm_get_state_hw().

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]