Message-ID: <eb1ce3b1-309f-ca1c-f24b-bdbcfa7c494b@w6rz.net>
Date:   Tue, 17 Jan 2023 07:08:56 -0800
From:   Ron Economos <re@...z.net>
To:     Nylon Chen <nylon.chen@...ive.com>,
        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

On 1/17/23 1:32 AM, Nylon Chen wrote:
> 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

I've tested this patch. For some reason, the heartbeat function no 
longer works for D12. This is my /etc/udev/rules.d/99-pwm-leds.rules 
file contents:

# D12 LED: heartbeat
SUBSYSTEM=="leds", KERNEL=="d12", ACTION=="add", ATTR{trigger}="heartbeat"

# D2 RGB LED: boot status
SUBSYSTEM=="leds", KERNEL=="d2", ACTION=="add", 
ATTR{trigger}="default-on", ATTR{multi_intensity}="25 25 25"

This is from https://github.com/sifive/meta-sifive, but modified for the 
multi_intensity driver introduced in Linux 6.0.

Powered by blists - more mailing lists