Date:   Thu, 27 Feb 2020 17:59:50 +0800
From:   Sam Shih <sam.shih@...iatek.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
CC:     Thierry Reding <thierry.reding@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        John Crispin <john@...ozen.org>, <linux-pwm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH 1/1] pwm: mediatek: add longer period support

On Thu, 2020-02-27 at 09:04 +0100, Uwe Kleine-König wrote:
On Thu, Feb 27, 2020 at 03:46:50PM +0800, Sam Shih wrote:
> > The pwm clock source could be divided by 1625 with PWM_CON
> > BIT(3) setting in mediatek hardware.
> > 
> > This patch add support for longer pwm period configuration,
> > which allowing blinking LEDs via pwm interface.
> > 
> > Signed-off-by: Sam Shih <sam.shih@...iatek.com>
> > ---
> >  drivers/pwm/pwm-mediatek.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index b94e0d09c300..9af309bea01a 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -121,8 +121,8 @@ static int pwm_mediatek_config(struct pwm_chip
*chip, struct pwm_device *pwm,
> >  			       int duty_ns, int period_ns)
> >  {
> >  	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> > -	u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
> > -	    reg_thres = PWMTHRES;
> > +	u32 clkdiv = 0, clksel = 0, cnt_period, cnt_duty,
> > +	    reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
> >  	u64 resolution;
> >  	int ret;
> >  
> Adding some more context:
> 

+ /* The pwm source clock can be divided by 2^clkdiv. When the clksel  +
* bit is set to 1, The final clock output needs to be divided by an +  *
extra 1625.
+  */

Is this ok ?


> > @@ -139,11 +139,20 @@ static int pwm_mediatek_config(struct pwm_chip
*chip, struct pwm_device *pwm,
> > 	while (cnt_period > 8191) {
> >  		resolution *= 2;
> >  		clkdiv++;
> >  		cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
> >  						   resolution);
> > +		if (clkdiv > PWM_CLK_DIV_MAX && !clksel) {
> > +			clksel = 1;
> > +			clkdiv = 0;
> > +			resolution = (u64)NSEC_PER_SEC * 1000 * 1625;
> > +			do_div(resolution,
> > +				clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
> > +			cnt_period = DIV_ROUND_CLOSEST_ULL(
> > +					(u64)period_ns * 1000, resolution);
> 
> The assignment is a repetition from just above the if. Maybe just put
it
> once after this if block?

The cnt_period represents the effective range of the PWM period counter,
when we need changing the pwm output period to a longer value at the
same clock frequency, we can setting a larger cnt_period, but the width
of the cnt_peroid register is 12 bits,
When the request period is too long, we need to divide the clock source
and then recalculate cnt_period outputs the correct waveform.
As mentioned above, when changing clkdiv, we need to recalculate
cnt_period immediately.

If the request period is very long (for example, LED blinking), clkdiv
may be insufficient. 
In this case, we will use clksel to divide the pwm source clock by an
additional 1625, and recalculate clkdiv and cnt_period.

I don't think we can just place assignments after the if block.

> 
> > +		}
> >  	}
> >  
> > -	if (clkdiv > PWM_CLK_DIV_MAX) {
> > +	if (clkdiv > PWM_CLK_DIV_MAX && clksel) {
> 
> Is this change actually relevant? If the while loop that starts at
line
> 139 is never run (because cnt_period is <= 8191) clkdiv is zero and so
> the condition is false with and without "&& clksel". If however the
> while loop is entered and clkdiv becomes bigger than PWM_CLK_DIV_MAX
> clksel is 1 and the "&& clksel" doesn't make a difference, too.
> 

You are right, I will remove this.

> The code is hard to follow, I wonder if this could be cleaned up with
> some comments added that explain the hardware details enough to be
able
> to actually understand the code without having the hardware reference
> manual handy.
> 

Is it sufficient to add some context into comment like the response of
the second question?


> >  		pwm_mediatek_clk_disable(chip, pwm);
> >  		dev_err(chip->dev, "period %d not supported\n", period_ns);
> >  		return -EINVAL;
> > @@ -159,7 +168,11 @@ static int pwm_mediatek_config(struct pwm_chip
*chip, struct pwm_device *pwm,
> >  	}
> >  
> >  	cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution);
> > -	pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> > +	if (clksel)
> > +		pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) |
> > +				    clkdiv);
> > +	else
> > +		pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> >  	pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period);
> >  	pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty);
> >  
> > -- 
> > 2.17.1
> 
> Best regards
> Uwe
> 
>

Best Regards,
Sam

Powered by blists - more mailing lists