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

[Ron Economos]

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@xxxxxxxxxx> 於 2023年1月14日 週六 上午3:24寫道：
> > > On 13 Jan 2023, at 18:32, Conor Dooley <conor@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/ 
> > > 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