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

[Gokul Praveen <g-praveen@ti.com>]

Hi Uwe,

Thank you for your prompt response and reply.

On 07/01/26 15:21, Uwe Kleine-König wrote:
> 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.
> 

Yes, it applies to all hardware variants, Uwe.

> 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@ti.com>
>> ---
>> 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.
> 

But the duty cycle, period and polarity values are extracted from the 
state parameter, right?

Please feel free to correct me if I am wrong, Uwe?
>>   {
>>   	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.
> 
The 2nd hunk is needed I believe because now, the ehrpwm_pwm_config 
function is called inside the ehrpwm_pwm_enable function.

Hence, calling the ehrpwm_pwm_config function before the 
ehrpwm_pwm_enable function within the ehrpwm_pwm_apply function would be 
a redundant call I believe when we try to enable the ehrpwm.

Now, coming to the ehrpwm_pwm_config function which I have added after 
the  ehrpwm_pwm_enable function in the else case, it would be needed I 
believe in order to change the duty cycle and period at runtime after 
the pwm has been enabled.

Please feel free and do correct me if I am wrong, Uwe.

> Best regards
> Uwe