Re: [RFC PATCH v2] regulator: pwm-regulator: Make assumptions about disabled PWM consistent

[Uwe Kleine-König]

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@xxxxxxxxxxxx>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxx/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

Attachment: signature.asc
Description: PGP signature