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;
Date:   Wed, 18 Jan 2023 08:13:49 +0000
From:   Conor Dooley <conor@kernel.org>
To:     Nylon Chen <nylon.chen@sifive.com>
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>,
        Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-pwm@vger.kernel.org
Subject: Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and
 algorithm
Message-ID: <Y8eqPdifnWbMQI/T@spud>
References: <20230113083115.2590-1-nylon.chen@sifive.com>
 <Y8GjySjm9OjoZvCF@spud>
 <95F1EAA0-D8D6-4F8A-8049-5E7BFDE4C06C@jrtc27.com>
 <Y8K1a+xs6tbo7kV4@spud>
 <CAHh=Yk9jUFiwzw+RQDoyb7xDoHGXFzNprdXn8szmMZb5FB4jLw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="KB0jfWFHyOhAHNFP"
Content-Disposition: inline
In-Reply-To: <CAHh=Yk9jUFiwzw+RQDoyb7xDoHGXFzNprdXn8szmMZb5FB4jLw@mail.gmail.com>
Precedence: bulk


--KB0jfWFHyOhAHNFP
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hey Nylon,

On Wed, Jan 18, 2023 at 10:32:25AM +0800, Nylon Chen wrote:
> 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
> > 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 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.
> > > > This info should be in the commit history IMO and the commit messag=
e for
> > > > the dts patch says what's obvious from the diff without any explana=
tion
> > > > 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_jOG2M03aLBgUOgGjWH9CUxq2aTG9=
7eSX70=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 trans=
istor 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 t=
he PWM, in
> > > which case it would make sense, but that=E2=80=99s a very narrow-mind=
ed 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.

> As you can see, there is also the same description in U-Boot.
>=20
> But in U-Boot, the DTS of Unmatched/Unleashed has not been added active-l=
ow.
>=20
> This is because active-high should be correct if we look at the circuit d=
iagram.

I am loathe to speak for Jess, but I don't think either of us are
disagreeing with your patches. I was just trying to understand why it was
wrong in the first place to see if it was intentionally inverted, or if
there was something that could be improve to stop it happening again.
Apologies if that got lost in translation.

> > 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."?

Thanks,
Conor.


--KB0jfWFHyOhAHNFP
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCY8eqPQAKCRB4tDGHoIJi
0l1NAP4hoQtZQkitKUCK7iMO1hSdPTd/IA6bhrI7bWiQNAnlKQD+MVDCawwKnyHs
GVs7HhE/g6s3aK6sBM0Gi3kXEppZCwE=
=zYGs
-----END PGP SIGNATURE-----

--KB0jfWFHyOhAHNFP--