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 19:40:12 -0800 |
| |
On 1/17/23 5:46 PM, Ron Economos wrote: > On 1/17/23 7:08 AM, Ron Economos wrote: >> 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. >> > I've instrumented this patch with some printks. The reason the > heartbeat doesn't work is that setting the LEDs to maximum brightness > causes the variable "frac" to be set to -1 instead of 0. > > You can test it with: > > # /bin/echo "255" > /sys/class/leds/d12/brightness > Sorry for the noise, I didn't apply the patch correctly. It works fine.
| |