| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1582806427.27775.1.camel@mtksdccf07>
Date: Thu, 27 Feb 2020 20:27:07 +0800
From: Sam Shih <sam.shih@...iatek.com>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
CC: <linux-pwm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
Thierry Reding <thierry.reding@...il.com>,
<linux-mediatek@...ts.infradead.org>,
"John Crispin" <john@...ozen.org>,
Matthias Brugger <matthias.bgg@...il.com>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [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@...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.
> > + */
>
> 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
Powered by blists - more mailing lists