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

[Martin Blumenstingl]

Hi Uwe,

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 ....


Best regards,
Martin
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>;
 
 		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;
 
-	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);
 
 	/*
 	 * 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);