lkml.org 
[lkml]   [2020]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/1] pwm: mediatek: add longer period support
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.

>
> 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?

> > 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.

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

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

\
 
 \ /
  Last update: 2020-02-27 11:53    [W:0.054 / U:1.644 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site