linux-kernel - Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and algorithm

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: <Y8K1a+xs6tbo7kV4@spud>
Date:   Sat, 14 Jan 2023 14:00:11 +0000
From:   Conor Dooley <conor@...nel.org>
To:     Jessica Clarke <jrtc27@...c27.com>
Cc:     Nylon Chen <nylon.chen@...ive.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        nylon7717@...il.com, zong.li@...ive.com, greentime.hu@...ive.com,
        vincent.chen@...ive.com, Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-pwm@...r.kernel.org
Subject: Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and
 algorithm

Hi Jess!

On Fri, Jan 13, 2023 at 07:24:56PM +0000, Jessica Clarke wrote:
> On 13 Jan 2023, at 18:32, Conor Dooley <conor@...nel.org> wrote:
> > 

> > Please run scripts/get_maintainer.pl before sending patches, you missed
> > both me & the PWM maintainers unfortunately!
> > AFAIK, the PWM maintainers use patchwork, so you will probably have to
> > resend this patchset so that it is on their radar.
> > I've marked the series as "Changes Requested" on the RISC-V one.
> > 
> > On Fri, Jan 13, 2023 at 04:31:13PM +0800, Nylon Chen wrote:
> > 
> >> According to the circuit diagram of User LEDs - RGB described in the
> >> manual hifive-unmatched-schematics-v3.pdf[0].
> >> The behavior of PWM is acitve-high.
> >> 
> >> According to the descriptionof PWM for pwmcmp in SiFive FU740-C000
> >> Manual[1].
> >> The pwm algorithm is (PW) pulse active time  = (D) duty * (T) period[2].
> >> The `frac` variable is pulse "inactive" time so we need to invert it.
> >> 
> >> So this patchset removes active-low in DTS and adds reverse logic to
> >> the driver.
> >> 
> >> [0]:https://sifive-china.oss-cn-zhangjiakou.aliyuncs.com/HiFIve%20Unmatched/hifive-unmatched-schematics-v3.pdf
> >> [1]:https://sifive-china.oss-cn-zhangjiakou.aliyuncs.com/HiFIve%20Unmatched/fu740-c000-manual-v1p2.pdf
> >> [2]:https://en.wikipedia.org/wiki/Duty_cycle
> > 
> > Please delete link 2, convert the other two to standard Link: tags and
> > put this information in the dts patch. Possibly into the PWM patch too,
> > depending on what the PWM maintainers think.
> > This info should be in the commit history IMO and the commit message for
> > the dts patch says what's obvious from the diff without any explanation
> > as to why.
> > 
> > I did a bit of looking around on lore, to see if I could figure out
> > why it was done like this in the first place, and I found:
> > https://lore.kernel.org/linux-pwm/CAJ2_jOG2M03aLBgUOgGjWH9CUxq2aTG97eSX70=UaSbGCMMF_g@mail.gmail.com/
> 
> That DTS documentation makes no sense to me, why does what the LED is
> wired to matter?

```
      active-low:
        description:
          For PWMs where the LED is wired to supply rather than ground.
```

> Whether you have your transistor next to ground or
> next to Vdd doesn’t matter, what matters is whether the transistor is
> on or off. Maybe what they mean is whether the *PWM's output* / *the
> transistor's input* is pulled to ground or Vdd? In which case the
> property would indeed not apply here.
> 
> Unless that’s written assuming the LED is wired directly to the PWM, in
> which case it would make sense, but that’s a very narrow-minded view of
> what the PWM output is (directly) driving.

I would suspect that it was written with that assumption.
Probably was the case on the specific board this property was originally
added for.

Maybe it'd be a bit more foolproof written as "For LEDs that are
illuminated while the PWM output is low. For example, where an LED is
wired between supply and the PWM output."?


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)