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

From: Conor Dooley <conor@kernel.org>
To: Jessica Clarke <jrtc27@jrtc27.com>
Cc: "Nylon Chen" <nylon.chen@sifive.com>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-riscv <linux-riscv@lists.infradead.org>,
	nylon7717@gmail.com, zong.li@sifive.com, greentime.hu@sifive.com,
	vincent.chen@sifive.com,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	linux-pwm@vger.kernel.org
Subject: Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and algorithm
Date: Sat, 14 Jan 2023 14:00:11 +0000	[thread overview]
Message-ID: <Y8K1a+xs6tbo7kV4@spud> (raw)
In-Reply-To: <95F1EAA0-D8D6-4F8A-8049-5E7BFDE4C06C@jrtc27.com>

[-- Attachment #1: Type: text/plain, Size: 3066 bytes --]

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@kernel.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."?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]