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;
Subject: Re: [PATCH 0/2] Change PWM-controlled LED pin active mode and
 algorithm
From:   Ron Economos <re@w6rz.net>
To:     Nylon Chen <nylon.chen@sifive.com>,
        Jessica Clarke <jrtc27@jrtc27.com>
Cc:     Conor Dooley <conor@kernel.org>,
        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
References: <20230113083115.2590-1-nylon.chen@sifive.com>
 <Y8GjySjm9OjoZvCF@spud> <95F1EAA0-D8D6-4F8A-8049-5E7BFDE4C06C@jrtc27.com>
 <CAHh=Yk-WJMOc-h9V47CyMQpyXZ5hDrtOPnPE2QKkJKzMhjh+Cg@mail.gmail.com>
 <eb1ce3b1-309f-ca1c-f24b-bdbcfa7c494b@w6rz.net>
Message-ID: <c0866553-d1a0-c76f-dea1-db3efceb7129@w6rz.net>
Date:   Tue, 17 Jan 2023 17:46:41 -0800
User-Agent: Mozilla/5.0 (X11; Linux armv7l; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <eb1ce3b1-309f-ca1c-f24b-bdbcfa7c494b@w6rz.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

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@jrtc27.com> 於 2023年1月14日 週六 上午3:24寫道：
>>> On 13 Jan 2023, at 18:32, Conor Dooley <conor@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