Messages in this thread | | | Subject | Re: [PATCH 1/1] pwm: mediatek: add longer period support | From | Sam Shih <> | Date | Thu, 27 Feb 2020 20:27:07 +0800 |
| |
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
| |