linux-kernel - Re: [PATCH v8 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190308115726.smgooacsr37fmxwg@pengutronix.de>
Date:   Fri, 8 Mar 2019 12:57:26 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Yash Shah <yash.shah@...ive.com>
Cc:     Palmer Dabbelt <palmer@...ive.com>, linux-pwm@...r.kernel.org,
        linux-riscv@...ts.infradead.org,
        Thierry Reding <thierry.reding@...il.com>, robh+dt@...nel.org,
        mark.rutland@....com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Sachin Ghadi <sachin.ghadi@...ive.com>,
        Paul Walmsley <paul.walmsley@...ive.com>, kernel@...gutronix.de
Subject: Re: [PATCH v8 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

Hello,

On Fri, Mar 08, 2019 at 04:59:36PM +0530, Yash Shah wrote:
> On Thu, Mar 7, 2019 at 8:57 PM Uwe Kleine-König
> <u.kleine-koenig@...gutronix.de> wrote:
> > > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > > +                         struct pwm_state *state)
> > > +{
> > > +     struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > +     unsigned int duty_cycle, x;
> > > +     u32 frac;
> > > +     struct pwm_state cur_state;
> > > +     bool enabled;
> > > +     int ret = 0;
> > > +     unsigned long num;
> > > +
> > > +     if (state->polarity != PWM_POLARITY_INVERSED)
> > > +             return -EINVAL;
> > > +
> > > +     mutex_lock(&pwm->lock);
> > > +     pwm_get_state(dev, &cur_state);
> > > +     enabled = cur_state.enabled;
> > > +
> > > +     if (state->period != cur_state.period) {
> >
> > Did you test this with more than one consumer? For sure the following
> > should work:
> >
> >         pwm1 = pwm_get(.. the first ..);
> >         pwm_apply_state(pwm1, { .enabled = true, .period = 10000000, .... });
> >
> >         pwm2 = pwm_get(.. the second ..);
> >         pwm_apply_state(pwm2, { .enabled = true, .period = 10000000, .... });
> >
> > but for the second pwm_apply_state() run state->period is likely not
> > exactly 10000000.
> 
> Yes, I have tested multiple consumers using sysfs interface. It is working.

Can you provide details about your testing here? What is the parent clk
rate? Which settings did you test? Can you confirm my claim that the
above sequence would fail or point out my error in reasoning?

> > > +     x = 1U << PWM_SIFIVE_CMPWIDTH;
> > > +     num = (u64)duty_cycle * x + x / 2;
> > > +     frac = div_u64(num, state->period);
> >
> > I don't understand the "+ x / 2" part. Should this better be
> > "+ state->period / 2"? Something like
> 
> This eqn is as per your comments against v5 of this patch series.
>  frac = (duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) + (1 <<
> PWM_SIFIVE_CMPWIDTH) / 2) / period;

OK, then not only the code is wrong, but also my suggestion was. :-)

> > #define div_u64_round(a, b) ({typeof(b) __b = b; div_u64(a + __b / 2, __b)})
> >
> > would make this less error prone.

This still stands. It makes it easier to get the code right and makes it
easier to understand.

Best regards
Uwe

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