Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230113083115.2590-1-nylon.chen@sifive.com> <Y8GjySjm9OjoZvCF@spud>
 <95F1EAA0-D8D6-4F8A-8049-5E7BFDE4C06C@jrtc27.com> <Y8K1a+xs6tbo7kV4@spud>
In-Reply-To: <Y8K1a+xs6tbo7kV4@spud>
From:   Nylon Chen <nylon.chen@sifive.com>
Date:   Wed, 18 Jan 2023 10:32:25 +0800
Message-ID: <CAHh=Yk9jUFiwzw+RQDoyb7xDoHGXFzNprdXn8szmMZb5FB4jLw@mail.gmail.com>
Subject: Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and algorithm
To:     Conor Dooley <conor@kernel.org>
Cc:     Jessica Clarke <jrtc27@jrtc27.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        nylon7717@gmail.com, zong.li@sifive.com, greentime.hu@sifive.com,
        vincent.chen@sifive.com, Thierry Reding <thierry.reding@gmail.com>,
        =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-pwm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Conor Dooley <conor@kernel.org> =E6=96=BC 2023=E5=B9=B41=E6=9C=8814=E6=97=
=A5 =E9=80=B1=E5=85=AD =E4=B8=8B=E5=8D=8810:00=E5=AF=AB=E9=81=93=EF=BC=9A
>
> 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 miss=
ed
> > > both me & the PWM maintainers unfortunately!
> > > AFAIK, the PWM maintainers use patchwork, so you will probably have t=
o
> > > 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  =3D (D) duty * (T) peri=
od[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%20Un=
matched/hifive-unmatched-schematics-v3.pdf
> > >> [1]:https://sifive-china.oss-cn-zhangjiakou.aliyuncs.com/HiFIve%20Un=
matched/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 an=
d
> > > put this information in the dts patch. Possibly into the PWM patch to=
o,
> > > 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 explanati=
on
> > > 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_jOG2M03aLBgUOgGjWH9CUxq2aTG97e=
SX70=3DUaSbGCMMF_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=E2=80=99t matter, what matters is whether the transis=
tor 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=E2=80=99s written assuming the LED is wired directly to the=
 PWM, in
> > which case it would make sense, but that=E2=80=99s 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 dia=
gram.
> 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."?
>