Re: [PATCH 1/1] pwm: mediatek: add longer period support

[Sam Shih <sam.shih@mediatek.com>]

Hello,

> 
> On Thu, Feb 27, 2020 at 05:59:50PM +0800, Sam Shih wrote:
> > 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@mediatek.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.
> > +  */
> 
> I'd write:
> 
> The source clock is divided by 2^clkdiv or iff the clksel bit is set
by
> 2^clkdiv + 1625.
> 

Great, the comment is short and clear. 
But maybe change “2^clkdiv + 1625” to “the product of 2^clkdiv and 1625”
is clearer ?

> > 
> > 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.
> 
> I didn't care enough to read your reasoning and retry to convince you
> with mine:
> 
> With your patch you have:
> 
> 	cnt_period = someexpression;
> 
> 	if (somecondition) {
> 		...
> 		cnt_period = someexpression;
> 	}
> 
> As somecondition doesn't make use of cnt_period this is equivalent to:
> 
> 	if (somecondition) {
> 		...
> 	}
> 	cnt_period = someexpression;
> 
> isn't it?
> 

Yes, you're right, I misunderstood. 
Your code clearly reminded me.

I just want to explain that the re-calculation of the cnt_period is
important.
However, after reading your code, I think the program logic will not be
break and the duplicates can be removed.

> > > 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?
> 
> I didn't check but I wouldn't be surprised if the code is more
> complicated than necessary. If you don't see something to simplify, go
> for adding an explanation as suggested and I will take a look in a
quiet
> moment.
>

I will send v2 patch to remove the repetition of “cnt_period =
someexpression”, and add some comment to clksel and cnt_period.
 
> Not sure I already pointed out that having a link to a publicly
> available reference manual in the driver's header is useful. If there
is
> such a manual, please add a link there. Your benefit is that you
> simplify others to improve your driver.
>
> 
> Best regards
> Uwe
> 
>

Best Regards.
Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel