Messages in this thread | | | Subject | Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and algorithm | From | Ron Economos <> | Date | Tue, 17 Jan 2023 07:08:56 -0800 |
| |
On 1/17/23 1:32 AM, Nylon Chen wrote: > Hi Conor, Jessica > > thanks for your reply. > > Jessica Clarke <jrtc27@jrtc27.com> 於 2023年1月14日 週六 上午3:24寫道: >> On 13 Jan 2023, at 18:32, Conor Dooley <conor@kernel.org> wrote: >>> +CC Uwe, Thierry, linux-pwm >>> >>> Hey Nylon, >>> >>> 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. > I got it. I will base it on get_maintainer.pl, re-sent it. >>> 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. > I got it. I will fix it. >>> 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? 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. >> >> Jess >> > This is a HiFive Unmatched/Unleashed LED-PWM layout > > VDD > | > | > _____ > \ / LED > \ / > --- > | > | > | > ______ > | | > - | > ^ --> |------ PWM > |___|___| > | > | > __ > - > GND > > - the waveform > e.g. duty=30s, period=100s, actvie-high = 30%, active-low = 70% > > V > ^ > | > | ----------| > | | > | | > |______ |__________ > t > > When VCC is high, the LED will be illuminated, which is an active-high > logic. This is why we need to remove "active-low". > > So, according to my understanding, Unleashed's DTS should also remove > active-low. >>> That doesn't explain the driver, but it does explain the dts being that >>> way. Perhaps a Fixes tag is also in order? But only if both patches get >>> one, otherwise backporting would lead to breakage. >>> >>> The min() construct appears to have been there since the RFC driver was >>> first posted. >>> >>> Thanks, >>> Conor. >>> >>>> Nylon Chen (2): >>>> riscv: dts: sifive unmatched: Remove PWM controlled LED's active-low >>> nit: s/sifive unmatched:/sifive: unmatched:/ > I got it. I will fix it. > >>>> properties >>>> pwm: sifive: change the PWM controlled LED algorithm >>>> >>>> arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ---- >>>> drivers/pwm/pwm-sifive.c | 1 + >>>> 2 files changed, 1 insertion(+), 4 deletions(-) >>>> >>>> -- >>>> 2.36.1
I've tested this patch. For some reason, the heartbeat function no longer works for D12. This is my /etc/udev/rules.d/99-pwm-leds.rules file contents:
# D12 LED: heartbeat SUBSYSTEM=="leds", KERNEL=="d12", ACTION=="add", ATTR{trigger}="heartbeat"
# D2 RGB LED: boot status SUBSYSTEM=="leds", KERNEL=="d2", ACTION=="add", ATTR{trigger}="default-on", ATTR{multi_intensity}="25 25 25"
This is from https://github.com/sifive/meta-sifive, but modified for the multi_intensity driver introduced in Linux 6.0.
| |