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

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

[Nylon Chen]

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

Nylon Chen (2):
  riscv: dts: sifive unmatched: Remove PWM controlled LED's active-low
    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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Nylon Chen]

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

Nylon Chen (2):
  riscv: dts: sifive unmatched: Remove PWM controlled LED's active-low
    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

[PATCH 1/2] riscv: dts: sifive 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.

Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
 1 file changed, 4 deletions(-)

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 1/2] riscv: dts: sifive 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.

Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
---
 arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
 1 file changed, 4 deletions(-)

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

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1534 bytes --]

Hey Nylon,

On Fri, Jan 13, 2023 at 04:31:14PM +0800, Nylon Chen wrote:
> This removes the active-low properties of the PWM-controlled LEDs in
> the HiFive Unmatched device tree.

Does unleashed also have the same issue?
It has the same compatible at the very least.

Thanks,
Conor.

> 
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
>  1 file changed, 4 deletions(-)
> 
> 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.36.1
> 

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

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

Hey Nylon,

On Fri, Jan 13, 2023 at 04:31:14PM +0800, Nylon Chen wrote:
> This removes the active-low properties of the PWM-controlled LEDs in
> the HiFive Unmatched device tree.

Does unleashed also have the same issue?
It has the same compatible at the very least.

Thanks,
Conor.

> 
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
>  1 file changed, 4 deletions(-)
> 
> 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.36.1
> 

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

[PATCH 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Nylon Chen]

The `frac` variable represents the pulse inactive time, and the result of
this algorithm is the pulse active time. Therefore, we must reverse the
result.

Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
---
 drivers/pwm/pwm-sifive.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index 62b6acc6373d..a5eda165d071 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
 	/* The hardware cannot generate a 100% duty cycle */
 	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+	frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
 
 	mutex_lock(&ddata->lock);
 	if (state->period != ddata->approx_period) {
-- 
2.36.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Nylon Chen]

The `frac` variable represents the pulse inactive time, and the result of
this algorithm is the pulse active time. Therefore, we must reverse the
result.

Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
Reviewed-by: Vincent Chen <vincent.chen@sifive.com>
---
 drivers/pwm/pwm-sifive.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index 62b6acc6373d..a5eda165d071 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -158,6 +158,7 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
 	/* The hardware cannot generate a 100% duty cycle */
 	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+	frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
 
 	mutex_lock(&ddata->lock);
 	if (state->period != ddata->approx_period) {
-- 
2.36.1

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

[Conor Dooley]

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

+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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2684 bytes --]

+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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Jessica Clarke]

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

> 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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Jessica Clarke]

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

> 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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

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

Hi Jess!

On Fri, Jan 13, 2023 at 07:24:56PM +0000, Jessica Clarke wrote:
> On 13 Jan 2023, at 18:32, Conor Dooley <conor@kernel.org> wrote:
> > 

> > 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 DTS documentation makes no sense to me, why does what the LED is
> wired to matter?

```
      active-low:
        description:
          For PWMs where the LED is wired to supply rather than ground.
```

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

I would suspect that it was written with that assumption.
Probably was the case on the specific board this property was originally
added for.

Maybe it'd be a bit more foolproof written as "For LEDs that are
illuminated while the PWM output is low. For example, where an LED is
wired between supply and the PWM output."?


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

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3066 bytes --]

Hi Jess!

On Fri, Jan 13, 2023 at 07:24:56PM +0000, Jessica Clarke wrote:
> On 13 Jan 2023, at 18:32, Conor Dooley <conor@kernel.org> wrote:
> > 

> > 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 DTS documentation makes no sense to me, why does what the LED is
> wired to matter?

```
      active-low:
        description:
          For PWMs where the LED is wired to supply rather than ground.
```

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

I would suspect that it was written with that assumption.
Probably was the case on the specific board this property was originally
added for.

Maybe it'd be a bit more foolproof written as "For LEDs that are
illuminated while the PWM output is low. For example, where an LED is
wired between supply and the PWM output."?


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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Nylon Chen]

Conor Dooley <conor@kernel.org> 於 2023年1月14日 週六 下午10:00寫道：
>
> Hi Jess!
>
> On Fri, Jan 13, 2023 at 07:24:56PM +0000, Jessica Clarke wrote:
> > On 13 Jan 2023, at 18:32, Conor Dooley <conor@kernel.org> wrote:
> > >
>
> > > 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 DTS documentation makes no sense to me, why does what the LED is
> > wired to matter?
>
> ```
>       active-low:
>         description:
>           For PWMs where the LED is wired to supply rather than ground.
> ```
>
> > 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.
>
> I would suspect that it was written with that assumption.
> Probably was the case on the specific board this property was originally
> added for.
>
Hi Conor

As you can see, there is also the same description in U-Boot.

But in U-Boot, the DTS of Unmatched/Unleashed has not been added active-low.

This is because active-high should be correct if we look at the circuit diagram.
> Maybe it'd be a bit more foolproof written as "For LEDs that are
> illuminated while the PWM output is low. For example, where an LED is
> wired between supply and the PWM output."?
>

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

[Nylon Chen]

Conor Dooley <conor@kernel.org> 於 2023年1月14日 週六 下午10:00寫道：
>
> Hi Jess!
>
> On Fri, Jan 13, 2023 at 07:24:56PM +0000, Jessica Clarke wrote:
> > On 13 Jan 2023, at 18:32, Conor Dooley <conor@kernel.org> wrote:
> > >
>
> > > 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 DTS documentation makes no sense to me, why does what the LED is
> > wired to matter?
>
> ```
>       active-low:
>         description:
>           For PWMs where the LED is wired to supply rather than ground.
> ```
>
> > 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.
>
> I would suspect that it was written with that assumption.
> Probably was the case on the specific board this property was originally
> added for.
>
Hi Conor

As you can see, there is also the same description in U-Boot.

But in U-Boot, the DTS of Unmatched/Unleashed has not been added active-low.

This is because active-high should be correct if we look at the circuit diagram.
> Maybe it'd be a bit more foolproof written as "For LEDs that are
> illuminated while the PWM output is low. For example, where an LED is
> wired between supply and the PWM output."?
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

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

Hey Nylon,

On Wed, Jan 18, 2023 at 10:32:25AM +0800, Nylon Chen wrote:
> Conor Dooley <conor@kernel.org> 於 2023年1月14日 週六 下午10:00寫道：
> > On Fri, Jan 13, 2023 at 07:24:56PM +0000, Jessica Clarke wrote:
> > > On 13 Jan 2023, at 18:32, Conor Dooley <conor@kernel.org> wrote:

> > > > 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 DTS documentation makes no sense to me, why does what the LED is
> > > wired to matter?
> >
> > ```
> >       active-low:
> >         description:
> >           For PWMs where the LED is wired to supply rather than ground.
> > ```
> >
> > > 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.
> >
> > I would suspect that it was written with that assumption.
> > Probably was the case on the specific board this property was originally
> > added for.

> As you can see, there is also the same description in U-Boot.
> 
> But in U-Boot, the DTS of Unmatched/Unleashed has not been added active-low.
> 
> This is because active-high should be correct if we look at the circuit diagram.

I am loathe to speak for Jess, but I don't think either of us are
disagreeing with your patches. I was just trying to understand why it was
wrong in the first place to see if it was intentionally inverted, or if
there was something that could be improve to stop it happening again.
Apologies if that got lost in translation.

> > Maybe it'd be a bit more foolproof written as "For LEDs that are
> > illuminated while the PWM output is low. For example, where an LED is
> > wired between supply and the PWM output."?

Thanks,
Conor.


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

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2726 bytes --]

Hey Nylon,

On Wed, Jan 18, 2023 at 10:32:25AM +0800, Nylon Chen wrote:
> Conor Dooley <conor@kernel.org> 於 2023年1月14日 週六 下午10:00寫道：
> > On Fri, Jan 13, 2023 at 07:24:56PM +0000, Jessica Clarke wrote:
> > > On 13 Jan 2023, at 18:32, Conor Dooley <conor@kernel.org> wrote:

> > > > 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 DTS documentation makes no sense to me, why does what the LED is
> > > wired to matter?
> >
> > ```
> >       active-low:
> >         description:
> >           For PWMs where the LED is wired to supply rather than ground.
> > ```
> >
> > > 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.
> >
> > I would suspect that it was written with that assumption.
> > Probably was the case on the specific board this property was originally
> > added for.

> As you can see, there is also the same description in U-Boot.
> 
> But in U-Boot, the DTS of Unmatched/Unleashed has not been added active-low.
> 
> This is because active-high should be correct if we look at the circuit diagram.

I am loathe to speak for Jess, but I don't think either of us are
disagreeing with your patches. I was just trying to understand why it was
wrong in the first place to see if it was intentionally inverted, or if
there was something that could be improve to stop it happening again.
Apologies if that got lost in translation.

> > Maybe it'd be a bit more foolproof written as "For LEDs that are
> > illuminated while the PWM output is low. For example, where an LED is
> > wired between supply and the PWM output."?

Thanks,
Conor.


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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Nylon Chen]

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
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

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

[Nylon Chen]

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
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Ron Economos]

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.

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

[Ron Economos]

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.



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Ron Economos]

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

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

[Ron Economos]

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



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [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@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
>
Sorry for the noise, I didn't apply the patch correctly. It works fine.

Re: [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@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
>
Sorry for the noise, I didn't apply the patch correctly. It works fine.



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Nylon Chen]

Hi Jess,

as you said, I use LED directly connected to PWM logic to modify it.

As I stated in my previous article, the key is that the lower the PWM
output is, the brighter the LED light is(active-low), the higher the
PWM output is, the brighter the LED light is(active-high).

Therefore, I would point out the waveform diagram below, the output
result remains the same for the circuit, but when you use
active-low/active-high to look at it, you will get two completely
different results of brightness.

e.g. duty=30s, period=100s, actvie-high = 30%, active-low = 70%

V
^
|
| ----------|
|             |
|             |
|______ |__________ > t

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.
> >
> > 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 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
>
> > 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@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

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

[Nylon Chen]

Hi Jess,

as you said, I use LED directly connected to PWM logic to modify it.

As I stated in my previous article, the key is that the lower the PWM
output is, the brighter the LED light is(active-low), the higher the
PWM output is, the brighter the LED light is(active-high).

Therefore, I would point out the waveform diagram below, the output
result remains the same for the circuit, but when you use
active-low/active-high to look at it, you will get two completely
different results of brightness.

e.g. duty=30s, period=100s, actvie-high = 30%, active-low = 70%

V
^
|
| ----------|
|             |
|             |
|______ |__________ > t

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.
> >
> > 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 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
>
> > 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@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv