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

[Ron Economos]

On 1/17/23 5:46 PM, Ron Economos wrote:
> On 1/17/23 7:08 AM, Ron Economos wrote:
>> On 1/17/23 1:32 AM, Nylon Chen wrote:
>>> Hi Conor, Jessica
>>>
>>> thanks for your reply.
>>>
>>> Jessica Clarke <jrtc27 at jrtc27.com> 於 2023年1月14日 週六 上午3:24寫道：
>>>> On 13 Jan 2023, at 18:32, Conor Dooley <conor at kernel.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.
>>
> I've instrumented this patch with some printks. The reason the 
> heartbeat doesn't work is that setting the LEDs to maximum brightness 
> causes the variable "frac" to be set to -1 instead of 0.
>
> You can test it with:
>
> # /bin/echo "255" > /sys/class/leds/d12/brightness
>
Sorry for the noise, I didn't apply the patch correctly. It works fine.