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

Open Source and information security mailing list archives

Message-ID: <Y8GjySjm9OjoZvCF@spud>
Date:   Fri, 13 Jan 2023 18:32:41 +0000
From:   Conor Dooley <conor@...nel.org>
To:     Nylon Chen <nylon.chen@...ive.com>
Cc:     paul.walmsley@...ive.com, palmer@...belt.com,
        linux-kernel@...r.kernel.org, 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

+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.

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.
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 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:/

>     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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives