Re: [RFC PATCH v1] regulator: pwm-regulator: Fix continuous get_voltage for disabled PWM

[Uwe Kleine-König]

Good morning Martin,

On Fri, Dec 22, 2023 at 01:17:44AM +0100, Martin Blumenstingl wrote:
> On Thu, Dec 21, 2023 at 11:03 PM Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> [...]
> > Note this isn't save in general. You're implicitly assuming that a
> > disabled PWM runs with the minimal supported duty_cycle. Most disabled
> > PWMs yield the inactive level (which corresponds to a 0% relative duty
> > cycle). But there are exceptions.
> Good catch - thank you!
> 
> [...]
> > Without claiming to understand all implications, I'd say
> > pwm_regulator_get_voltage should signal to the caller when the
> > duty_cycle isn't contained in [min(max_uV_duty, min_uV_duty),
> > max(max_uV_duty, min_uV_duty)].
> It seems like there's -ENOTRECOVERABLE that we can signal to the caller.
> This makes the following message appear in my kernel log:
>   VDDEE: Setting 1100000-1140000uV
> Which means that pwm_regulator_set_voltage() is called which will then
> pick the minimum voltage.
> 
> To make this work I will need to update meson8b-odroidc1.dts (which
> isn't a problem, I just want to point it out as it's mandatory for
> that solution. also I will send that in a separate patch).
> 
> See my attached patch (which replaces the initial patch I sent, it's
> not meant to be applied on top).
> One question that I still have is whether we are allowed to just
> enable the PWM output within pwm_regulator_set_voltage().
> Doing so is actually mandatory, otherwise we end up in an infinite
> loop of not being able to read the voltage, then sets the minimum
> voltage (but leaves the PWM output disabled) which then means that it
> can't read back the voltage which means it tries to set the minimum
> voltage ....

Without enabling the PWM pwm_regulator_set_voltage() doesn't work if the
PWM isn't enabled already when the function is entered. So I'd say yes,
you must enable it.

> diff --git a/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts b/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> index b03273d90ad8..df348e119643 100644
> --- a/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> +++ b/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> @@ -217,13 +217,13 @@ vddee: regulator-vddee {
>  		compatible = "pwm-regulator";
>  
>  		regulator-name = "VDDEE";
> -		regulator-min-microvolt = <860000>;
> +		regulator-min-microvolt = <1100000>;
>  		regulator-max-microvolt = <1140000>;
>  
>  		pwm-supply = <&p5v0>;
>  
>  		pwms = <&pwm_cd 1 12218 0>;
> -		pwm-dutycycle-range = <91 0>;
> +		pwm-dutycycle-range = <14 0>;

This is ugly. You have to take away the description that a relative
duty-cycle of 91% yields 860 mV just because the linux driver behaves in
a certain way.

Also the calculation is wrong: If a relative duty-cyle in the interval
[91%; 0%] maps lineary to [860 mV; 1140 mV] you get 1100 mV at

	     1100 mV - 860 mV
	91 + ---------------- * (0 - 91) = 13
	     1140 mV - 860 mV

(If the calculations in the driver used signed multiplication and
division, all the checks for max_uV_duty < min_uV_duty could just go
away.)

So you want

+		pwm-dutycycle-range = <13 0>;

(if this restriction is really necessary).

>  		regulator-boot-on;
>  		regulator-always-on;

> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 30402ee18392..cb4e5fad5702 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -156,13 +156,10 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
>  	unsigned int voltage;
>  
>  	pwm_get_state(drvdata->pwm, &pstate);
> +	if (!pstate.enabled)
> +		return -ENOTRECOVERABLE;

This is good.

>  
> -	if (pstate.enabled)
> -		voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
> -	else if (max_uV_duty < min_uV_duty)
> -		voltage = max_uV_duty;
> -	else
> -		voltage = min_uV_duty;
> +	voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);

I'd add here:

	if (voltage < min(max_uV_duty, min_uV_duty) ||
	    voltage > max(max_uV_duty, min_uV_duty))
		return -ENOTRECOVERABLE;

>  	/*
>  	 * The dutycycle for min_uV might be greater than the one for max_uV.
> @@ -221,6 +218,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>  
>  	pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
>  
> +	pstate.enabled = true;
>  	ret = pwm_apply_state(drvdata->pwm, &pstate);
>  	if (ret) {
>  		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);

Otherwise the change to the driver looks fine to me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature