Message-ID: <CAHh=Yk-WJMOc-h9V47CyMQpyXZ5hDrtOPnPE2QKkJKzMhjh+Cg@mail.gmail.com>
Date:   Tue, 17 Jan 2023 17:32:40 +0800
From:   Nylon Chen <nylon.chen@...ive.com>
To:     Jessica Clarke <jrtc27@...c27.com>
Cc:     Conor Dooley <conor@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        nylon7717@...il.com, zong.li@...ive.com, greentime.hu@...ive.com,
        vincent.chen@...ive.com, Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-pwm@...r.kernel.org
Subject: Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and algorithm

Hi Conor, Jessica

thanks for your reply.

Jessica Clarke <jrtc27@...c27.com> 於 2023年1月14日 週六 上午3:24寫道：
>
> On 13 Jan 2023, at 18:32, Conor Dooley <conor@...nel.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
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@...ts.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Powered by blists - more mailing lists