Re: [PATCH 1/2] regulator: pwm-regulator: Revert "Calculate the output voltage for disabled PWMs"
From: Martin Blumenstingl
Date: Mon Nov 09 2026 - 16:31:22 EST
On Mon, Nov 9, 2026 at 7:09 AM Mike Looijmans <mike.looijmans@xxxxxxxx> wrote:
>
> On 06-11-2026 22:57, Martin Blumenstingl wrote:
> > On Fri, Nov 6, 2026 at 2:42 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
> >> 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")
> >> Adding Martin who wrote that patch - please CC people if you're directly
> >> updating their patches.
> > IIRC get_maintainers.pl includes the author of the patch blamed by the
> > Fixes: tag (if there's one).
> >
> >>> 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.
> > How do we know if it's 0% or tri-stated (and what's the behavior if
> > it's tri-stated)?
>
> We don't know, that's the point I'm trying to make. If the PWM is
> disabled, likely its clock is also turned off, and the actual state of
> the output is not known. Depending on hardware, it could be high, low,
> tristated, pulled up/down, ...
Not knowing the actual status is what Uwe also noted 2 years ago: [0]
> What may happen now is:
>
> - The PWM is set at 50% and disabled.
>
> - This function returns "0%" as being the current level during probe of
> the supply.
>
> - The core wants it to be at 0%, so it doesn't change the setting and
> enables the PWM.
>
> - The PWM now runs at 50% and the core thinks it's at 0%.
>
>
> What happens in my case:
>
> - This function returns 0% because the PWM is still disabled
>
> - Core wants it to be 50%, so it calls set_voltage() which sets the PWM
> but doesn't enable it yet
>
> - Core calls this function again and (still) gets 0% as being the setting.
>
> - This goes on until the supply (and hence the PWM) is enabled.
>
>
> What the callers expect is that this function returns the value that
> would be set if the PWM were enabled.
I think this is a good description and something we can improve upon.
I attached a patch - please try it. If it's not enough: please apply
your patch 2/2 on top.
The goal of that patch is:
- keep duty cycle around once it has been set
- only reset to duty cycle 0 at initialization and only if the
regulator has been described with regulator-boot-on
- (it achieves this by keeping a copy of struct pwm_state around)
I'd like to hear about your testing results and initial code-review feedback.
+Cc Uwe - happy to take your feedback as well.
>
> >>> 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
> > What board are you testing with? I'd like to see the .dts to get a
> > better understanding of where your board differs from mine.
> > I think Uwe (PWM maintainer) also previously mentioned that there's no
> > defined behavior for disabling a PWM and then reading it's state
> > again. I'll see if I can find his explanation and whether I can come
> > up with a patch that may help here.
>
> Here's the DTS fragment for my power supply. You can trigger the
> behaviour with "pwm-dutycycle-range":
>
> dsp7_supply_pwm: dsp7-supply-pwm {
> compatible = "pwm-regulator";
> pwms = <&pwm 1 50000 0>; /* Period of 50000ns means
> 20kHz */
> regulator-name = "dsp7-pwm";
> /* VCOM = 3.9V + 0.9V * duty_cycle. */
> regulator-min-microvolt = <3900000>;
> regulator-max-microvolt = <4800000>;
> pwm-dutycycle-unit = <100>;
> pwm-dutycycle-range = <50 100>; /* Must be >= 4.35V */
> pwm-supply = <&dsp7_supply>;
> };
Thank you. Since this doesn't seem to be upstream yet I'm against
reverting my original patch (more below).
[...]
> > The problem with this specific revert (your patch 1/2) is that (from
> > memory) it will break these boards:
> > - arch/arm/boot/dts/amlogic/meson8b-ec100.dts
> > - arch/arm/boot/dts/amlogic/meson8b-mxq.dts
> > - arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
> > - arch/arm/boot/dts/amlogic/meson8m2-wetek-core.dts
>
> I honestly don't see how...
Back in the day I tried to describe it in detail: [1]
The short summary is:
- PWM settings match 860mV but PWM output is disabled (on this
SoC/board that means: duty cycle = 0)
- SoC is not stable below 1100mV
- pwm_enable() was used during boot and pwm-regulator did not adjust
the duty cycle and/or period before
-> hard lockup during boot
Best regards,
Martin
[0] https://lore.kernel.org/all/y7svv2jvcof3fnezvjbirkian5b7ajic7ajxgtyiejj3364ie7@uvmqlpjj6tv5/
[1] https://lore.kernel.org/all/20240113224628.377993-4-martin.blumenstingl@xxxxxxxxxxxxxx/
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -41,6 +41,8 @@ struct pwm_regulator_data {
/* Enable GPIO */
struct gpio_desc *enb_gpio;
+
+ struct pwm_state pwm_state;
};
struct pwm_voltages {
@@ -48,18 +50,40 @@ struct pwm_voltages {
unsigned int dutycycle;
};
+static int pwm_regulator_update_dutycycle(struct regulator_dev *rdev,
+ unsigned int dutycyle,
+ unsigned int dutycycle_unit)
+{
+ struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+ int ret;
+
+ pwm_set_relative_duty_cycle(&drvdata->pwm_state, dutycyle, dutycycle_unit);
+
+ if (!drvdata->pwm_state.enabled)
+ /*
+ * Keep the "disabled configuration" (disabled = false,
+ * period = 0, duty_cycle = 0) on the actual PWM output until
+ * the regulator is getting enabled.
+ */
+ return 0;
+
+ ret = pwm_apply_might_sleep(drvdata->pwm, &drvdata->pwm_state);
+ if (ret)
+ dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
+
+ return ret;
+}
+
/*
* Voltage table call-backs
*/
static void pwm_regulator_init_state(struct regulator_dev *rdev)
{
struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
- struct pwm_state pwm_state;
unsigned int dutycycle;
int i;
- pwm_get_state(drvdata->pwm, &pwm_state);
- dutycycle = pwm_get_relative_duty_cycle(&pwm_state, 100);
+ dutycycle = pwm_get_relative_duty_cycle(&drvdata->pwm_state, 100);
for (i = 0; i < rdev->desc->n_voltages; i++) {
if (dutycycle == drvdata->duty_cycle_table[i].dutycycle) {
@@ -83,18 +107,13 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
unsigned selector)
{
struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
- struct pwm_state pstate;
int ret;
- pwm_init_state(drvdata->pwm, &pstate);
- pwm_set_relative_duty_cycle(&pstate,
- drvdata->duty_cycle_table[selector].dutycycle, 100);
-
- ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
- if (ret) {
- dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
+ ret = pwm_regulator_update_dutycycle(rdev,
+ drvdata->duty_cycle_table[selector].dutycycle,
+ 100);
+ if (ret)
return ret;
- }
drvdata->state = selector;
@@ -115,17 +134,41 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
static int pwm_regulator_enable(struct regulator_dev *dev)
{
struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+ int ret;
gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
- return pwm_enable(drvdata->pwm);
+ drvdata->pwm_state.enabled = true;
+
+ ret = pwm_apply_might_sleep(drvdata->pwm, &drvdata->pwm_state);
+ if (ret)
+ dev_err(&dev->dev, "Failed to enable PWM: %d\n", ret);
+
+ return ret;
}
static int pwm_regulator_disable(struct regulator_dev *dev)
{
struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+ struct pwm_state new_pstate;
+ int ret;
+
+ drvdata->pwm_state.enabled = false;
+
+ /*
+ * Keep the period and duty_cycle within the copy in drvdata untouched
+ * so the regulator can be enabled again without a .set_voltage{,_sel}
+ * call prior to re-enabling it.
+ */
+ new_pstate = drvdata->pwm_state;
+ new_pstate.period = 0;
+ new_pstate.duty_cycle = 0;
- pwm_disable(drvdata->pwm);
+ ret = pwm_apply_might_sleep(drvdata->pwm, &new_pstate);
+ if (ret) {
+ dev_err(&dev->dev, "Failed to disable PWM: %d\n", ret);
+ return ret;
+ }
gpiod_set_value_cansleep(drvdata->enb_gpio, 0);
@@ -139,7 +182,7 @@ static int pwm_regulator_is_enabled(struct regulator_dev *dev)
if (drvdata->enb_gpio && !gpiod_get_value_cansleep(drvdata->enb_gpio))
return false;
- return pwm_is_enabled(drvdata->pwm);
+ return drvdata->pwm_state.enabled;
}
static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
@@ -151,20 +194,10 @@ static int pwm_regulator_get_voltage(struct regulator_dev *rdev)
int min_uV = rdev->constraints->min_uV;
int max_uV = rdev->constraints->max_uV;
int diff_uV = max_uV - min_uV;
- struct pwm_state pstate;
unsigned int diff_duty;
unsigned int voltage;
- 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;
- }
-
- voltage = pwm_get_relative_duty_cycle(&pstate, duty_unit);
+ voltage = pwm_get_relative_duty_cycle(&drvdata->pwm_state, duty_unit);
if (voltage < min(max_uV_duty, min_uV_duty) ||
voltage > max(max_uV_duty, min_uV_duty))
return -ENOTRECOVERABLE;
@@ -198,12 +231,8 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
int min_uV = rdev->constraints->min_uV;
int max_uV = rdev->constraints->max_uV;
int diff_uV = max_uV - min_uV;
- struct pwm_state pstate;
unsigned int diff_duty;
unsigned int dutycycle;
- int ret;
-
- pwm_init_state(drvdata->pwm, &pstate);
/*
* The dutycycle for min_uV might be greater than the one for max_uV.
@@ -224,15 +253,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
else
dutycycle = min_uV_duty + dutycycle;
- pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
-
- ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
- if (ret) {
- dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
- return ret;
- }
-
- return 0;
+ return pwm_regulator_update_dutycycle(rdev, dutycycle, duty_unit);
}
static const struct regulator_ops pwm_regulator_voltage_table_ops = {
@@ -321,32 +342,6 @@ static int pwm_regulator_init_continuous(struct platform_device *pdev,
return 0;
}
-static int pwm_regulator_init_boot_on(struct platform_device *pdev,
- struct pwm_regulator_data *drvdata,
- const struct regulator_init_data *init_data)
-{
- struct pwm_state pstate;
-
- if (!init_data->constraints.boot_on || drvdata->enb_gpio)
- return 0;
-
- pwm_get_state(drvdata->pwm, &pstate);
- if (pstate.enabled)
- return 0;
-
- /*
- * Update the duty cycle so the output does not change
- * when the regulator core enables the regulator (and
- * thus the PWM channel).
- */
- if (pstate.polarity == PWM_POLARITY_INVERSED)
- pstate.duty_cycle = pstate.period;
- else
- pstate.duty_cycle = 0;
-
- return pwm_apply_might_sleep(drvdata->pwm, &pstate);
-}
-
static int pwm_regulator_probe(struct platform_device *pdev)
{
const struct regulator_init_data *init_data;
@@ -404,10 +399,23 @@ static int pwm_regulator_probe(struct platform_device *pdev)
if (ret)
return ret;
- ret = pwm_regulator_init_boot_on(pdev, drvdata, init_data);
- if (ret)
- return dev_err_probe(&pdev->dev, ret,
- "Failed to apply boot_on settings\n");
+ pwm_init_state(drvdata->pwm, &drvdata->pwm_state);
+
+ if (init_data->constraints.boot_on && !drvdata->enb_gpio &&
+ !drvdata->pwm_state.enabled)
+ /*
+ * In general it's unknown what the output of a disabled PWM is.
+ * The only sane assumption here is it emits the inactive level
+ * which corresponds to duty_cycle = 0 (independent of the
+ * polarity).
+ *
+ * Update the duty cycle so the output does not change when the
+ * regulator core enables the regulator (and thus the PWM
+ * channel) and there's no change to the duty cycle because the
+ * voltage that is achieved with a disabled PWM is already the
+ * desired one.
+ */
+ drvdata->pwm_state.duty_cycle = 0;
regulator = devm_regulator_register(&pdev->dev,
&drvdata->desc, &config);