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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\))
Subject: Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and
 algorithm
From:   Jessica Clarke <jrtc27@jrtc27.com>
In-Reply-To: <Y8GjySjm9OjoZvCF@spud>
Date:   Fri, 13 Jan 2023 19:24:56 +0000
Cc:     Nylon Chen <nylon.chen@sifive.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-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>,
        linux-pwm@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <95F1EAA0-D8D6-4F8A-8049-5E7BFDE4C06C@jrtc27.com>
References: <20230113083115.2590-1-nylon.chen@sifive.com>
 <Y8GjySjm9OjoZvCF@spud>
To:     Conor Dooley <conor@kernel.org>
Precedence: bulk

On 13 Jan 2023, at 18:32, Conor Dooley <conor@kernel.org> wrote:
>=20
> +CC Uwe, Thierry, linux-pwm
>=20
> Hey Nylon,
>=20
> 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.
>=20
> On Fri, Jan 13, 2023 at 04:31:13PM +0800, Nylon Chen wrote:
>=20
>> 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.
>>=20
>> 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) =
period[2].
>> The `frac` variable is pulse "inactive" time so we need to invert it.
>>=20
>> So this patchset removes active-low in DTS and adds reverse logic to
>> the driver.
>>=20
>> =
[0]:https://sifive-china.oss-cn-zhangjiakou.aliyuncs.com/HiFIve%20Unmatche=
d/hifive-unmatched-schematics-v3.pdf
>> =
[1]:https://sifive-china.oss-cn-zhangjiakou.aliyuncs.com/HiFIve%20Unmatche=
d/fu740-c000-manual-v1p2.pdf
>> [2]:https://en.wikipedia.org/wiki/Duty_cycle
>=20
> 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 message =
for
> the dts patch says what's obvious from the diff without any =
explanation
> as to why.
>=20
> 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=3D=
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=E2=80=99t 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=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.

Jess

> 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.
>=20
> The min() construct appears to have been there since the RFC driver =
was
> first posted.
>=20
> Thanks,
> Conor.
>=20
>>=20
>> Nylon Chen (2):
>>  riscv: dts: sifive unmatched: Remove PWM controlled LED's active-low
>=20
> nit: s/sifive unmatched:/sifive: unmatched:/
>=20
>>    properties
>>  pwm: sifive: change the PWM controlled LED algorithm
>>=20
>> arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
>> drivers/pwm/pwm-sifive.c                            | 1 +
>> 2 files changed, 1 insertion(+), 4 deletions(-)
>>=20
>> --=20
>> 2.36.1
>>=20
>>=20
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv