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

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

[Nylon Chen]

According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].

The behavior of PWM is acitve-high.

Removed patches: 1
New patches: (none)

Links:
- [0]:  https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf
- [1]:  https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf
- [2]:  https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf

Changed in v4:
 - Remove previous updates to the PWM algorithm.

Changed in v3:
 - Convert the reference link to standard link.
 - Move the inverted function before taking the minimum value.
 - Change polarity check condition(high and low).
 - Pick the biggest period length possible that is not bigger than the
   requested period.

Changed in v2:
 - Convert the reference link to standard link.
 - Fix typo: s/sifive unmatched:/sifive: unmatched:/.
 - Remove active-low from hifive-unleashed-a00.dts.
 - Include this reference link in the dts and pwm commit messages.


Nylon Chen (1):
  riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's
    active-low properties

 arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 4 ----
 arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
 2 files changed, 8 deletions(-)

-- 
2.40.1

[PATCH v4 1/1] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Nylon Chen]

This removes the active-low properties of the PWM-controlled LEDs in
the HiFive Unmatched device tree.

The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].

Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
---
 arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 4 ----
 arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
 2 files changed, 8 deletions(-)

diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
index 900a50526d77..7a9f336a391c 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
@@ -50,7 +50,6 @@ led-controller {
 
 		led-d1 {
 			pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
-			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
 			label = "d1";
@@ -58,7 +57,6 @@ led-d1 {
 
 		led-d2 {
 			pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
-			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
 			label = "d2";
@@ -66,7 +64,6 @@ led-d2 {
 
 		led-d3 {
 			pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
-			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
 			label = "d3";
@@ -74,7 +71,6 @@ led-d3 {
 
 		led-d4 {
 			pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
-			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
 			label = "d4";
diff --git a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
index 07387f9c135c..11f08a545ee6 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts
@@ -52,7 +52,6 @@ led-controller-1 {
 
 		led-d12 {
 			pwms = <&pwm0 0 7812500 PWM_POLARITY_INVERTED>;
-			active-low;
 			color = <LED_COLOR_ID_GREEN>;
 			max-brightness = <255>;
 			label = "d12";
@@ -69,19 +68,16 @@ multi-led {
 
 			led-red {
 				pwms = <&pwm0 2 7812500 PWM_POLARITY_INVERTED>;
-				active-low;
 				color = <LED_COLOR_ID_RED>;
 			};
 
 			led-green {
 				pwms = <&pwm0 1 7812500 PWM_POLARITY_INVERTED>;
-				active-low;
 				color = <LED_COLOR_ID_GREEN>;
 			};
 
 			led-blue {
 				pwms = <&pwm0 3 7812500 PWM_POLARITY_INVERTED>;
-				active-low;
 				color = <LED_COLOR_ID_BLUE>;
 			};
 		};
-- 
2.40.1

Re: [PATCH v4 1/1] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

On Thu, Aug 03, 2023 at 04:57:34PM +0800, Nylon Chen wrote:
> This removes the active-low properties of the PWM-controlled LEDs in
> the HiFive Unmatched device tree.
> 
> The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> 
> Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
> Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]
> 

> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Since there is no PWM driver patch for this to go along with, this ack
doesn't mean anything anymore. Please drop it if you resubmit.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 1/1] riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's active-low properties

[Nylon Chen]

Hi Conor,

I apologize for forgetting about this update earlier. Just now,
I tried to pull rebase master and noticed that other developers seem
to have made some fixes to the algorithm. Upon closer inspection, I
found that they addressed the part we previously discussed with Emil
and Uwe, such as "first pwm_apply_state."

Therefore, my instinct tells me that they have already taken care of
the issues we discussed before.

I will review the conflicting parts in the pwm-sifive.c code in my v4
version once again to ensure there are no omissions. If I find any, I
will submit v5 accordingly.

Conor Dooley <conor.dooley@microchip.com> 於 2023年8月3日 週四 下午5:19寫道：
>
> On Thu, Aug 03, 2023 at 04:57:34PM +0800, Nylon Chen wrote:
> > This removes the active-low properties of the PWM-controlled LEDs in
> > the HiFive Unmatched device tree.
> >
> > The reference is hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> >
> > Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
> > Link: https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf [1]
> >
>
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
>
> Since there is no PWM driver patch for this to go along with, this ack
> doesn't mean anything anymore. Please drop it if you resubmit.
>

Re: [PATCH v4 0/1] Change PWM-controlled LED pin active mode and algorithm

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> 
> The behavior of PWM is acitve-high.
> 
> Removed patches: 1
> New patches: (none)
> 
> Links:
> - [0]:  https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf
> - [1]:  https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf
> - [2]:  https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> 
> Changed in v4:
>  - Remove previous updates to the PWM algorithm.

Why? I don't recall the conclusion on the previous version being that
that patch was not needed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 0/1] Change PWM-controlled LED pin active mode and algorithm

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

Hey Nylon,

(I yoinked the reply to 1/1 to here, as it makes more sense in this
context)

> On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote:
> > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > > 
> > > The behavior of PWM is acitve-high.
> > > 
> > > Removed patches: 1
> > > New patches: (none)
> > > 
> > > Links:
> > > - [0]:  https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf
> > > - [1]:  https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf
> > > - [2]:  https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > > 
> > > Changed in v4:
> > >  - Remove previous updates to the PWM algorithm.
> > 
> > Why? I don't recall the conclusion on the previous version being that
> > that patch was not needed.
>
> I apologize for forgetting about this update earlier. Just now,
> I tried to pull rebase master and noticed that other developers seem
> to have made some fixes to the algorithm. Upon closer inspection, I
> found that they addressed the part we previously discussed with Emil
> and Uwe, such as "first pwm_apply_state."
> 
> Therefore, my instinct tells me that they have already taken care of
> the issues we discussed before.

I didn't see anything in linux-next that would solve this problem of
inversion. The last meaningful change is:
	commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75
	Author:     Emil Renner Berthing <emil.renner.berthing@canonical.com>
	AuthorDate: Wed Nov 9 12:37:24 2022 +0100
	Commit:     Thierry Reding <thierry.reding@gmail.com>
	CommitDate: Mon Jan 30 16:42:45 2023 +0100
	
	    pwm: sifive: Always let the first pwm_apply_state succeed

which predates your v3 by quite a bit.

> I will review the conflicting parts in the pwm-sifive.c code in my v4
> version once again to ensure there are no omissions. If I find any, I
> will submit v5 accordingly.

And if this patch is okay in isolation, please reply here explaining
which commit fixed the algorithm, so that I can pick it up.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 0/1] Change PWM-controlled LED pin active mode and algorithm

[Nylon Chen]

Hi Conor,

Conor Dooley <conor.dooley@microchip.com> 於 2023年8月3日 週四 下午5:44寫道：
>
> Hey Nylon,
>
> (I yoinked the reply to 1/1 to here, as it makes more sense in this
> context)
>
> > On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote:
> > > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> > > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > > >
> > > > The behavior of PWM is acitve-high.
> > > >
> > > > Removed patches: 1
> > > > New patches: (none)
> > > >
> > > > Links:
> > > > - [0]:  https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf
> > > > - [1]:  https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf
> > > > - [2]:  https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > > >
> > > > Changed in v4:
> > > >  - Remove previous updates to the PWM algorithm.
> > >
> > > Why? I don't recall the conclusion on the previous version being that
> > > that patch was not needed.
> >
> > I apologize for forgetting about this update earlier. Just now,
> > I tried to pull rebase master and noticed that other developers seem
> > to have made some fixes to the algorithm. Upon closer inspection, I
> > found that they addressed the part we previously discussed with Emil
> > and Uwe, such as "first pwm_apply_state."
> >
> > Therefore, my instinct tells me that they have already taken care of
> > the issues we discussed before.
>
> I didn't see anything in linux-next that would solve this problem of
> inversion. The last meaningful change is:
>         commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75
>         Author:     Emil Renner Berthing <emil.renner.berthing@canonical.com>
>         AuthorDate: Wed Nov 9 12:37:24 2022 +0100
>         Commit:     Thierry Reding <thierry.reding@gmail.com>
>         CommitDate: Mon Jan 30 16:42:45 2023 +0100
>
>             pwm: sifive: Always let the first pwm_apply_state succeed
>
> which predates your v3 by quite a bit.
>
> > I will review the conflicting parts in the pwm-sifive.c code in my v4
> > version once again to ensure there are no omissions. If I find any, I
> > will submit v5 accordingly.
>
> And if this patch is okay in isolation, please reply here explaining
> which commit fixed the algorithm, so that I can pick it up.
This patch needs to be accompanied by modifications to the
pwm_sifive_apply() function to make sense.

I will double-check and review the previous discussions to ensure
that. Thank you for your response.
>
> Thanks,
> Conor.

Re: [PATCH v4 0/1] Change PWM-controlled LED pin active mode and algorithm

[Nylon Chen]

Hi Conor,

Thank you for patiently giving me advice. I appreciate your help.

Not long ago, I said, "This patch needs to be accompanied by
modifications to the pwm_sifive_apply() function to make sense."

I recently reviewed the v3 version, and after discussing it with Emil,
there are several areas that require modification. I will provide the
necessary changes for each of them:

1. polarity check. (Suggestion from Uwe)
- if (state->polarity != PWM_POLARITY_INVERSED)
+ if (state->polarity != PWM_POLARITY_NORMAL)
2. avoid using old periodperiod, not state->period
- period = max(state->period, ddata->approx_period);
- frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
+ frac = DIV64_U64_ROUND_CLOSEST(num, period);
3. add a conditional check can be added in the code to set
ddata->approx_period to state->period when state->period is smaller
than ddata->approx_period
  if (state->period != ddata->approx_period) {
  ...
+       if (state->period < ddata->approx_period) {
+               ddata->approx_period = state->period;
+       }
-       ddata->approx_period = state->period;
+       period = ddata->approx_period;

I will use 'unmatched' on my end to verify again. If there are any
other errors, feel free to point them out. Thank you.

Nylon Chen <nylon.chen@sifive.com> 於 2023年8月4日 週五 上午9:42寫道：
>
> Hi Conor,
>
> Conor Dooley <conor.dooley@microchip.com> 於 2023年8月3日 週四 下午5:44寫道：
> >
> > Hey Nylon,
> >
> > (I yoinked the reply to 1/1 to here, as it makes more sense in this
> > context)
> >
> > > On Thu, Aug 03, 2023 at 10:15:10AM +0100, Conor Dooley wrote:
> > > > On Thu, Aug 03, 2023 at 04:57:33PM +0800, Nylon Chen wrote:
> > > > > According to the circuit diagram of User LEDs - RGB described in themanual hifive-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].
> > > > >
> > > > > The behavior of PWM is acitve-high.
> > > > >
> > > > > Removed patches: 1
> > > > > New patches: (none)
> > > > >
> > > > > Links:
> > > > > - [0]:  https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf
> > > > > - [1]:  https://sifive.cdn.prismic.io/sifive/6a06d6c0-6e66-49b5-8e9e-e68ce76f4192_hifive-unmatched-schematics-v3.pdf
> > > > > - [2]:  https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > > > >
> > > > > Changed in v4:
> > > > >  - Remove previous updates to the PWM algorithm.
> > > >
> > > > Why? I don't recall the conclusion on the previous version being that
> > > > that patch was not needed.
> > >
> > > I apologize for forgetting about this update earlier. Just now,
> > > I tried to pull rebase master and noticed that other developers seem
> > > to have made some fixes to the algorithm. Upon closer inspection, I
> > > found that they addressed the part we previously discussed with Emil
> > > and Uwe, such as "first pwm_apply_state."
> > >
> > > Therefore, my instinct tells me that they have already taken care of
> > > the issues we discussed before.
> >
> > I didn't see anything in linux-next that would solve this problem of
> > inversion. The last meaningful change is:
> >         commit 334c7b13d38321e47d1a51dba0bef9f4c403ec75
> >         Author:     Emil Renner Berthing <emil.renner.berthing@canonical.com>
> >         AuthorDate: Wed Nov 9 12:37:24 2022 +0100
> >         Commit:     Thierry Reding <thierry.reding@gmail.com>
> >         CommitDate: Mon Jan 30 16:42:45 2023 +0100
> >
> >             pwm: sifive: Always let the first pwm_apply_state succeed
> >
> > which predates your v3 by quite a bit.
> >
> > > I will review the conflicting parts in the pwm-sifive.c code in my v4
> > > version once again to ensure there are no omissions. If I find any, I
> > > will submit v5 accordingly.
> >
> > And if this patch is okay in isolation, please reply here explaining
> > which commit fixed the algorithm, so that I can pick it up.
> This patch needs to be accompanied by modifications to the
> pwm_sifive_apply() function to make sense.
>
> I will double-check and review the previous discussions to ensure
> that. Thank you for your response.
> >
> > Thanks,
> > Conor.

Re: [PATCH v4 0/1] Change PWM-controlled LED pin active mode and algorithm

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On Fri, Aug 04, 2023 at 02:54:33PM +0800, Nylon Chen wrote:
> Hi Conor,
> 
> Thank you for patiently giving me advice. I appreciate your help.
> 
> Not long ago, I said, "This patch needs to be accompanied by
> modifications to the pwm_sifive_apply() function to make sense."
> 
> I recently reviewed the v3 version, and after discussing it with Emil,
> there are several areas that require modification. I will provide the
> necessary changes for each of them:
> 
> 1. polarity check. (Suggestion from Uwe)
> - if (state->polarity != PWM_POLARITY_INVERSED)
> + if (state->polarity != PWM_POLARITY_NORMAL)
> 2. avoid using old periodperiod, not state->period
> - period = max(state->period, ddata->approx_period);
> - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> + frac = DIV64_U64_ROUND_CLOSEST(num, period);
> 3. add a conditional check can be added in the code to set
> ddata->approx_period to state->period when state->period is smaller
> than ddata->approx_period
>   if (state->period != ddata->approx_period) {
>   ...
> +       if (state->period < ddata->approx_period) {
> +               ddata->approx_period = state->period;
> +       }
> -       ddata->approx_period = state->period;
> +       period = ddata->approx_period;
> 

> I will use 'unmatched' on my end to verify again. If there are any
> other errors, feel free to point them out. Thank you.

I'm not sure of the driver details without going and looking into the
code itself, but this sounds like it makes a lot more sense than just
flipping the polarity in the dts. Thanks for taking another look!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]