Messages in this thread | | | From | Nylon Chen <> | Date | Wed, 18 Jan 2023 10:32:25 +0800 | Subject | Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and algorithm |
| |
Conor Dooley <conor@kernel.org> 於 2023年1月14日 週六 下午10:00寫道: > > 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. > Hi Conor
As you can see, there is also the same description in U-Boot.
But in U-Boot, the DTS of Unmatched/Unleashed has not been added active-low.
This is because active-high should be correct if we look at the circuit diagram. > 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."? >
| |