Messages in this thread |  | | Date | Sat, 14 Jan 2023 14:00:11 +0000 | From | Conor Dooley <> | 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@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."?
[unhandled content-type:application/pgp-signature] |  |