Generally it's not known how a disabled PWM behaves. However if the
bootloader hands over a disabled PWM that drives a regulator and it's
claimed the regulator is enabled, then the most likely assumption is
that the PWM emits the inactive level. This is represented by duty_cycle
= 0 even for .polarity == PWM_POLARITY_INVERSED.

Change the implementation to always use duty_cycle = 0, regardless of
the polarity. Also update the code so it keeps a copy of the pwm_state
around. Both of these changes result in the fact that the logic from
pwm_regulator_init_boot_on() is much less complex and can be simplified
to a point where the whole function is not needed anymore.

Suggested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
Changes from v1 [0] (which was sent by Uwe):
- keep the struct pwm_state around to prevent a regression on Meson8b
  Odroid-C1 boards where just calling pwm_enable() without adjusting
  the duty_cycle to 0 would hang the board
- I'm actively looking for input on the commit description and
  suggestions whether/why Fixes tags should be applied


[0] https://lore.kernel.org/lkml/CAFBinCB+S1wOpD-fCbcTORqXSV00Sd7-1GHUKY+rO859_dkhOA@mail.gmail.com/T/


 drivers/regulator/pwm-regulator.c | 119 ++++++++++++++----------------
 1 file changed, 55 insertions(+), 64 deletions(-)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 7434b6b22d32..5ea354a0654a 100644
--- 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,33 @@ struct pwm_voltages {
 	unsigned int dutycycle;
 };
 
+static int pwm_regulator_apply_state(struct regulator_dev *rdev,
+				     struct pwm_state *new_state)
+{
+	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
+	int ret;
+
+	ret = pwm_apply_might_sleep(drvdata->pwm, new_state);
+	if (ret) {
+		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
+		return ret;
+	}
+
+	drvdata->pwm_state = *new_state;
+
+	return 0;
+}
+
 /*
  * 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 +100,15 @@ 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;
+	struct pwm_state pstate = drvdata->pwm_state;
 	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_apply_state(rdev, &pstate);
+	if (ret)
 		return ret;
-	}
 
 	drvdata->state = selector;
 
@@ -115,17 +129,26 @@ 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);
+	struct pwm_state pstate = drvdata->pwm_state;
 
 	gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
 
-	return pwm_enable(drvdata->pwm);
+	pstate.enabled = true;
+
+	return pwm_regulator_apply_state(dev, &pstate);
 }
 
 static int pwm_regulator_disable(struct regulator_dev *dev)
 {
 	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
+	struct pwm_state pstate = drvdata->pwm_state;
+	int ret;
+
+	pstate.enabled = false;
 
-	pwm_disable(drvdata->pwm);
+	ret = pwm_regulator_apply_state(dev, &pstate);
+	if (ret)
+		return ret;
 
 	gpiod_set_value_cansleep(drvdata->enb_gpio, 0);
 
@@ -139,7 +162,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 +174,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;
@@ -195,15 +208,12 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 	unsigned int min_uV_duty = drvdata->continuous.min_uV_dutycycle;
 	unsigned int max_uV_duty = drvdata->continuous.max_uV_dutycycle;
 	unsigned int duty_unit = drvdata->continuous.dutycycle_unit;
+	struct pwm_state pstate = drvdata->pwm_state;
 	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.
@@ -226,13 +236,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 
 	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_apply_state(rdev, &pstate);
 }
 
 static const struct regulator_ops pwm_regulator_voltage_table_ops = {
@@ -321,32 +325,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 +382,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);
-- 
2.45.2

Hello Martin,

On Sat, Jun 22, 2024 at 09:15:04PM +0200, Martin Blumenstingl wrote:
> Generally it's not known how a disabled PWM behaves. However if the
> bootloader hands over a disabled PWM that drives a regulator and it's
> claimed the regulator is enabled, then the most likely assumption is
> that the PWM emits the inactive level. This is represented by duty_cycle
> = 0 even for .polarity == PWM_POLARITY_INVERSED.

I'd write: "This is represented by duty_cycle = 0 independent of the
polarity."

> Change the implementation to always use duty_cycle = 0, regardless of
> the polarity. Also update the code so it keeps a copy of the pwm_state
> around. Both of these changes result in the fact that the logic from
> pwm_regulator_init_boot_on() is much less complex and can be simplified
> to a point where the whole function is not needed anymore.

In my (German) ear the following sounds a bit nicer:

	Both of these changes reduce the complexity of
	pwm_regulator_init_boot_on() to a point where the whole function
	is not needed anymore.

> Suggested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> Changes from v1 [0] (which was sent by Uwe):
> - keep the struct pwm_state around to prevent a regression on Meson8b
>   Odroid-C1 boards where just calling pwm_enable() without adjusting
>   the duty_cycle to 0 would hang the board
> - I'm actively looking for input on the commit description and
>   suggestions whether/why Fixes tags should be applied

Apart of the nitpicking above, I like the commit description.

Regarding a Fixes tag: I'm unsure if without this patch, the
pwm-regulator driver is broken for your Odroid-C1 board. It's not,
right?
I think I wouldn't add a Fixes tag and just consider this patch a
cleanup then.
 
> [0] https://lore.kernel.org/lkml/CAFBinCB+S1wOpD-fCbcTORqXSV00Sd7-1GHUKY+rO859_dkhOA@mail.gmail.com/T/
> 
> 
>  drivers/regulator/pwm-regulator.c | 119 ++++++++++++++----------------
>  1 file changed, 55 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 7434b6b22d32..5ea354a0654a 100644
> --- 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,33 @@ struct pwm_voltages {
>  	unsigned int dutycycle;
>  };
>  
> +static int pwm_regulator_apply_state(struct regulator_dev *rdev,
> +				     struct pwm_state *new_state)
> +{
> +	struct pwm_regulator_data *drvdata = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	ret = pwm_apply_might_sleep(drvdata->pwm, new_state);
> +	if (ret) {
> +		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
> +		return ret;
> +	}
> +
> +	drvdata->pwm_state = *new_state;
> +
> +	return 0;
> +}
> +
>  /*
>   * 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 +100,15 @@ 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;
> +	struct pwm_state pstate = drvdata->pwm_state;
>  	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_apply_state(rdev, &pstate);
> +	if (ret)
>  		return ret;
> -	}

If you drop the local variable pstate and just do

	pwm_set_relative_duty_cycle(drvdata->pwm_state,
				drvdata->duty_cycle_table[selector].dutycycle, 100);

you might get a mismatch between actual configuration and
drvdata->pwm_state if pwm_regulator_apply_state() fails, but I think
that doesn't matter and simplifies the code a bit. (Drop the assignment
in pwm_regulator_apply_state() then.)
 
>  	drvdata->state = selector;
>  
> @@ -115,17 +129,26 @@ 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);
> +	struct pwm_state pstate = drvdata->pwm_state;
>  
>  	gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
>  
> -	return pwm_enable(drvdata->pwm);
> +	pstate.enabled = true;
> +
> +	return pwm_regulator_apply_state(dev, &pstate);
>  }
>  
>  static int pwm_regulator_disable(struct regulator_dev *dev)
>  {
>  	struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> +	struct pwm_state pstate = drvdata->pwm_state;
> +	int ret;
> +
> +	pstate.enabled = false;
>  
> -	pwm_disable(drvdata->pwm);
> +	ret = pwm_regulator_apply_state(dev, &pstate);
> +	if (ret)
> +		return ret;

With that part I'm a bit unhappy. You don't know what the pwm does when
disabled (it might yield a 100% relative duty cycle). So the safe bet is
to use .enabled=true, .duty_cycle=0 here.

The only code that "knows" if it's safe to disable the PWM is the
lowlevel pwm driver.

>  	gpiod_set_value_cansleep(drvdata->enb_gpio, 0);
>  
> @@ -139,7 +162,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;
>  }

Thanks for picking up the task to improve my patch!

Best regards
Uwe

Hello Uwe,

On Sun, Jun 23, 2024 at 11:32 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Hello Martin,
>
> On Sat, Jun 22, 2024 at 09:15:04PM +0200, Martin Blumenstingl wrote:
> > Generally it's not known how a disabled PWM behaves. However if the
> > bootloader hands over a disabled PWM that drives a regulator and it's
> > claimed the regulator is enabled, then the most likely assumption is
> > that the PWM emits the inactive level. This is represented by duty_cycle
> > = 0 even for .polarity == PWM_POLARITY_INVERSED.
>
> I'd write: "This is represented by duty_cycle = 0 independent of the
> polarity."
That makes things easier - I'll apply this, thank you!

> > Change the implementation to always use duty_cycle = 0, regardless of
> > the polarity. Also update the code so it keeps a copy of the pwm_state
> > around. Both of these changes result in the fact that the logic from
> > pwm_regulator_init_boot_on() is much less complex and can be simplified
> > to a point where the whole function is not needed anymore.
>
> In my (German) ear the following sounds a bit nicer:
>
>         Both of these changes reduce the complexity of
>         pwm_regulator_init_boot_on() to a point where the whole function
>         is not needed anymore.
Sounds fine to my (German) ear as well - thanks!

> > Suggested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > ---
> > Changes from v1 [0] (which was sent by Uwe):
> > - keep the struct pwm_state around to prevent a regression on Meson8b
> >   Odroid-C1 boards where just calling pwm_enable() without adjusting
> >   the duty_cycle to 0 would hang the board
> > - I'm actively looking for input on the commit description and
> >   suggestions whether/why Fixes tags should be applied
>
> Apart of the nitpicking above, I like the commit description.
>
> Regarding a Fixes tag: I'm unsure if without this patch, the
> pwm-regulator driver is broken for your Odroid-C1 board. It's not,
> right?
> I think I wouldn't add a Fixes tag and just consider this patch a
> cleanup then.
My Odroid-C1 works fine with and without this patch.
Only the patch that you found previously which you wanted to improve
is required (and reverting it breaks boot).

[...]
> > -     ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
> > -     if (ret) {
> > -             dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
> > +     ret = pwm_regulator_apply_state(rdev, &pstate);
> > +     if (ret)
> >               return ret;
> > -     }
>
> If you drop the local variable pstate and just do
>
>         pwm_set_relative_duty_cycle(drvdata->pwm_state,
>                                 drvdata->duty_cycle_table[selector].dutycycle, 100);
>
> you might get a mismatch between actual configuration and
> drvdata->pwm_state if pwm_regulator_apply_state() fails, but I think
> that doesn't matter and simplifies the code a bit. (Drop the assignment
> in pwm_regulator_apply_state() then.)
If you're fine with this potential mismatch (I am - I just was unsure
about potential side-effects) then you're right: this is an
improvement!

> >       drvdata->state = selector;
> >
> > @@ -115,17 +129,26 @@ 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);
> > +     struct pwm_state pstate = drvdata->pwm_state;
> >
> >       gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
> >
> > -     return pwm_enable(drvdata->pwm);
> > +     pstate.enabled = true;
> > +
> > +     return pwm_regulator_apply_state(dev, &pstate);
> >  }
> >
> >  static int pwm_regulator_disable(struct regulator_dev *dev)
> >  {
> >       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > +     struct pwm_state pstate = drvdata->pwm_state;
> > +     int ret;
> > +
> > +     pstate.enabled = false;
> >
> > -     pwm_disable(drvdata->pwm);
> > +     ret = pwm_regulator_apply_state(dev, &pstate);
> > +     if (ret)
> > +             return ret;
>
> With that part I'm a bit unhappy. You don't know what the pwm does when
> disabled (it might yield a 100% relative duty cycle). So the safe bet is
> to use .enabled=true, .duty_cycle=0 here.
>
> The only code that "knows" if it's safe to disable the PWM is the
> lowlevel pwm driver.
Here I don't know the regulator framework enough. Let's make two assumptions:
1. the optimization you suggest is implemented (I'm not against it,
it's just different from what pwm_disable() does)
2. regulator core does not expect the set voltage to change in a
.disable() callback

In that case disabling the PWM output is fine. Since we're now
updating the cached pwm_state with duty_cycle = 0 the next time the
regulator core calls the .enable() callback (without calling
.set_voltage() between disabling and enabling) we end up enabling the
PWM output with duty_cycle = 0 (and thus likely changing the voltage
output).
I see three options here:
- my assumption about the regulator core is incorrect, then your
optimization works just fine
- we only write enabled = false to the cached pwm_state but not duty_cycle = 0
- we drop the suggested optimization here (and maybe let PWM core handle this)

What do you think?


Best regards,
Martin

Hello Martin,

On Sun, Jun 23, 2024 at 11:09:08PM +0200, Martin Blumenstingl wrote:
> On Sun, Jun 23, 2024 at 11:32 AM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Sat, Jun 22, 2024 at 09:15:04PM +0200, Martin Blumenstingl wrote:
> > >       drvdata->state = selector;
> > >
> > > @@ -115,17 +129,26 @@ 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);
> > > +     struct pwm_state pstate = drvdata->pwm_state;
> > >
> > >       gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
> > >
> > > -     return pwm_enable(drvdata->pwm);
> > > +     pstate.enabled = true;
> > > +
> > > +     return pwm_regulator_apply_state(dev, &pstate);
> > >  }
> > >
> > >  static int pwm_regulator_disable(struct regulator_dev *dev)
> > >  {
> > >       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > > +     struct pwm_state pstate = drvdata->pwm_state;
> > > +     int ret;
> > > +
> > > +     pstate.enabled = false;
> > >
> > > -     pwm_disable(drvdata->pwm);
> > > +     ret = pwm_regulator_apply_state(dev, &pstate);
> > > +     if (ret)
> > > +             return ret;
> >
> > With that part I'm a bit unhappy. You don't know what the pwm does when
> > disabled (it might yield a 100% relative duty cycle). So the safe bet is
> > to use .enabled=true, .duty_cycle=0 here.
> >
> > The only code that "knows" if it's safe to disable the PWM is the
> > lowlevel pwm driver.
> Here I don't know the regulator framework enough. Let's make two assumptions:
> 1. the optimization you suggest is implemented (I'm not against it,
> it's just different from what pwm_disable() does)

In general you cannot know how a disabled PWM behaves. The objective of
.enabled = false is to save power and don't care about output. The
typical implementations yield the inactive level, but there are
exceptions that are hard to fix -- if at all. These include High-Z and
the active level. So if the pwm-regulator relies on the PWM emitting the
inactive level, .enabled = false is wrong. I guess in general you don't
know and .enabled = true + .duty_cycle = 0 is the right thing?

> 2. regulator core does not expect the set voltage to change in a
> .disable() callback
> 
> In that case disabling the PWM output is fine. Since we're now
> updating the cached pwm_state with duty_cycle = 0 the next time the
> regulator core calls the .enable() callback (without calling
> .set_voltage() between disabling and enabling) we end up enabling the
> PWM output with duty_cycle = 0 (and thus likely changing the voltage
> output).
> I see three options here:
> - my assumption about the regulator core is incorrect, then your
> optimization works just fine
> - we only write enabled = false to the cached pwm_state but not duty_cycle = 0
> - we drop the suggested optimization here (and maybe let PWM core handle this)
> 
> What do you think?

I'm unsure. I can tell the effect of .enabled = false, but I don't know
if this effect is ok for the pwm-regulator. I also don't know if
disabling and reenabling the regulator is expected to keep the voltage.
Who can tell? I'd hope Mark?

Best regards
Uwe