Re: [PATCH v2] pwm: tiehrpwm: Enable EHRPWM controller before setting configuration

[Uwe Kleine-König]

Hello,

adding Rafael to Cc: who sent a patch series for this driver that I
didn't come around to review yet. Given that neither he nor me noticed
the problem addressed in this patch I wonder if it applies to all
hardware variants.

On Wed, Jan 07, 2026 at 11:23:39AM +0530, Gokul Praveen wrote:
> The period and duty cycle configurations does not get reflected
> after setting them using sysfs nodes. This is because at the
> end of ehrpwm_pwm_config function, the put_sync function is
> called which resets the hardware.
> 
> Fix it by preventing the pwm controller from going into
> low-power mode.
> 
> Fixes: 5f027d9b83db("pwm: tiehrpwm: Implement .apply() callback")
> Signed-off-by: Gokul Praveen <g-praveen@xxxxxx>
> ---
> v2 <==> v1
> ==========
> * Removed space between Fixes and Signed-off tag
> 
>  drivers/pwm/pwm-tiehrpwm.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index 7a86cb090f76..408aed70be8c 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -237,7 +237,6 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (period_cycles < 1)
>  		period_cycles = 1;
>  
> -	pm_runtime_get_sync(pwmchip_parent(chip));
>  
>  	/* Update clock prescaler values */
>  	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CLKDIV_MASK, tb_divval);
> @@ -290,12 +289,11 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (!(duty_cycles > period_cycles))
>  		ehrpwm_write(pc->mmio_base, cmp_reg, duty_cycles);
>  
> -	pm_runtime_put_sync(pwmchip_parent(chip));
> -
>  	return 0;
>  }
>  
> -static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> +				const struct pwm_state *state)

With this function also caring for *state the name isn't appropriate any
more.

>  {
>  	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
>  	u16 aqcsfrc_val, aqcsfrc_mask;
> @@ -304,6 +302,13 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	/* Leave clock enabled on enabling PWM */
>  	pm_runtime_get_sync(pwmchip_parent(chip));
>  
> +	ret = ehrpwm_pwm_config(chip, pwm, state->duty_cycle, state->period, state->polarity);
> +
> +	if (ret) {
> +		pm_runtime_put_sync(pwmchip_parent(chip));
> +		return ret;
> +	}
> +
>  	/* Disabling Action Qualifier on PWM output */
>  	if (pwm->hwpwm) {
>  		aqcsfrc_val = AQCSFRC_CSFB_FRCDIS;
> @@ -391,12 +396,11 @@ static int ehrpwm_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return 0;
>  	}
>  
> -	err = ehrpwm_pwm_config(chip, pwm, state->duty_cycle, state->period, state->polarity);
> -	if (err)
> -		return err;
> -
>  	if (!enabled)
> -		err = ehrpwm_pwm_enable(chip, pwm);
> +		err = ehrpwm_pwm_enable(chip, pwm, state);
> +	else
> +		err = ehrpwm_pwm_config(chip, pwm, state->duty_cycle,
> +					state->period, state->polarity);
>  
>  	return err;
>  }

Why are the changes from the two hunks above needed? Reading the change
log I only understand the first hunk and would expect it to be enough.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature