Re: [PATCH] pwm: meson: fix harware duty calculation

[Jerome Brunet]

On Wed, 2017-11-29 at 11:03 +0800, Yixun Lan wrote:
> From: Jian Hu <jian.hu@xxxxxxxxxxx>
> 
> The actual HIGH/LOW signal output from the PWM is equal to
> the value programed to HW register plus one, this is designed by HW.
> 
> This fix should apply to all Meson SoC(include GX/GXL/GXBB, Meson6,8)
> 
> Fixes: 211ed630753d ("pwm: Add support for Meson PWM Controller")
> Signed-off-by: Jian Hu <jian.hu@xxxxxxxxxxx>
> Signed-off-by: Yixun Lan <yixun.lan@xxxxxxxxxxx>
> ---
>  drivers/pwm/pwm-meson.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index d589331d1884..78d9b8c1a4bc 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -193,6 +193,11 @@ static int meson_pwm_calc(struct meson_pwm *meson,
>  			break;
>  	}
>  
> +	if (cnt < 2) {
> +		dev_err(meson->chip.dev, "invalid period\n");
> +		return -EINVAL;
> +	}
> +
>  	if (pre_div == MISC_CLK_DIV_MASK) {
>  		dev_err(meson->chip.dev, "unable to get period pre_div\n");
>  		return -EINVAL;
> @@ -201,19 +206,23 @@ static int meson_pwm_calc(struct meson_pwm *meson,
>  	dev_dbg(meson->chip.dev, "period=%u pre_div=%u cnt=%u\n", period,
>  		pre_div, cnt);
>  
> +	/*
> +	 * Due to the design of hardware, values of 'hi', 'lo' are 1 based
> +	 * which mean the actual output from hardware is 'hi' + 1, 'lo' + 1
> +	 */
>  	if (duty == period) {
>  		channel->pre_div = pre_div;
> -		channel->hi = cnt;
> +		channel->hi = cnt - 1;
>  		channel->lo = 0;
>  	} else if (duty == 0) {
>  		channel->pre_div = pre_div;
>  		channel->hi = 0;
> -		channel->lo = cnt;
> +		channel->lo = cnt - 1;
>  	} else {
>  		/* Then check is we can have the duty with the same pre_div
> */
>  		duty_cnt = DIV_ROUND_CLOSEST_ULL((u64)duty * 1000,
>  						 fin_ps * (pre_div + 1));
> -		if (duty_cnt > 0xffff) {
> +		if (duty_cnt > 0xffff || !duty_cnt) {

duty_cnt = 0 is a valid value here. It will be the case for duty != 0 but low
enough for the HW (calculation) to approximate the duty cycle to zero.

>  			dev_err(meson->chip.dev, "unable to get duty
> cycle\n");
>  			return -EINVAL;
>  		}
> @@ -222,8 +231,8 @@ static int meson_pwm_calc(struct meson_pwm *meson,
>  			duty, pre_div, duty_cnt);
>  
>  		channel->pre_div = pre_div;
> -		channel->hi = duty_cnt;
> -		channel->lo = cnt - duty_cnt;
> +		channel->hi = duty_cnt - 1;

As explained above, duty_cnt could be zero, you need to take care of this here

> +		channel->lo = cnt - duty_cnt - 1;

Same here, it is possible duty_cnt to be egual to cnt so you also need to be
careful here

>  	}
>  
>  	return 0;