Re: [PATCH v8 3/5] pwm: dwc: add PWM bit unset in get_state call

[Uwe Kleine-König]

On Wed, Jun 14, 2023 at 06:14:55PM +0100, Ben Dooks wrote:
> If we are not in PWM mode, then the output is technically a 50%
> output based on a single timer instead of the high-low based on
> the two counters. Add a check for the PWM mode in dwc_pwm_get_state()
> and if DWC_TIM_CTRL_PWM is not set, then return a 50% cycle.
> 
> This may only be an issue on initialisation, as the rest of the
> code currently assumes we're always going to have the extended
> PWM mode using two counters.
> 
> Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxx>
> ---
> v8:
>  - fixed rename issues
> v4:
>  - fixed review comment on mulit-line calculations
> ---
>  drivers/pwm/pwm-dwc-core.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> index 4b4b7b9e1d82..38cd2163fe01 100644
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c
> @@ -122,24 +122,31 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  {
>  	struct dwc_pwm *dwc = to_dwc_pwm(chip);
>  	u64 duty, period;
> +	u32 ctrl, ld, ld2;
>  
>  	pm_runtime_get_sync(chip->dev);
>  
> -	state->enabled = !!(dwc_pwm_readl(dwc,
> -				DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
> +	ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
> +	ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> +	ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
>  
> -	duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> -	duty += 1;
> -	duty *= dwc->clk_ns;
> -	state->duty_cycle = duty;
> +	state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
>  
> -	period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
> -	period += 1;
> -	period *= dwc->clk_ns;
> -	period += duty;
> -	state->period = period;
> +	/* If we're not in PWM, technically the output is a 50-50

Multiline comments should have /* on a line for itself.

> +	 * based on the timer load-count only.
> +	 */
> +	if (ctrl & DWC_TIM_CTRL_PWM) {
> +		duty = (ld + 1) * dwc->clk_ns;
> +		period = (ld2 + 1)  * dwc->clk_ns;

s/  / /

> +		period += duty;
> +	} else {
> +		duty = (ld + 1) * dwc->clk_ns;
> +		period = duty * 2;
> +	}
>  
>  	state->polarity = PWM_POLARITY_INVERSED;
> +	state->period = period;
> +	state->duty_cycle = duty;
>  
>  	pm_runtime_put_sync(chip->dev);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature