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

[PATCH v3 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-unleashed-a00.pdf[0] and hifive-unmatched-schematics-v3.pdf[1].

The behavior of PWM is acitve-high.

According to the descriptionof PWM for pwmcmp in SiFive FU740-C000 Manual[2].

The pwm algorithm is (PW) pulse active time  = (D) duty * (T) period.
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.

Updated patches: 2
New patches: (none)
Unchanged patches: 1

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

Changed in v3:
 - Convert the reference link to standard link.
 - Move the inverted function before taking the minimum value.
 - Change polarity check condition(high and low).
 - Pick the biggest period length possible that is not bigger than the requested period.
 
Changed in v2:
 - Convert the reference link to standard link.
 - Fix typo: s/sifive unmatched:/sifive: unmatched:/.
 - Remove active-low from hifive-unleashed-a00.dts.
 - Include this reference link in the dts and pwm commit messages.

Nylon Chen (2):
  riscv: dts: sifive: unleashed/unmatched: Remove PWM controlled LED's
    active-low properties
  pwm: sifive: change the PWM controlled LED algorithm

 arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 4 ----
 arch/riscv/boot/dts/sifive/hifive-unmatched-a00.dts | 4 ----
 drivers/pwm/pwm-sifive.c                            | 9 ++++++---
 3 files changed, 6 insertions(+), 11 deletions(-)

-- 
2.40.0

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

[Nylon Chen]

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

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

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

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

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

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

[Conor Dooley]

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

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

Just a minor nit here, there should not be a blank line between the
link:s and the rest of the trailers.

Cheers,
Conor.

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

[PATCH v3 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.

The reference is SiFive FU740-C000 Manual[0]

Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
---
 drivers/pwm/pwm-sifive.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
index 393a4b97fc19..d5d5f36da297 100644
--- a/drivers/pwm/pwm-sifive.c
+++ b/drivers/pwm/pwm-sifive.c
@@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
 	struct pwm_state cur_state;
-	unsigned int duty_cycle;
+	unsigned int duty_cycle, period;
 	unsigned long long num;
 	bool enabled;
 	int ret = 0;
 	u32 frac;
 
-	if (state->polarity != PWM_POLARITY_INVERSED)
+	if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
 		return -EINVAL;
 
 	cur_state = pwm->state;
@@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * calculating the register values first and then writing them
 	 * consecutively
 	 */
+	period = max(state->period, ddata->approx_period);
 	num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
 	frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
-	/* The hardware cannot generate a 100% duty cycle */
 	frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
+	/* The hardware cannot generate a 100% duty cycle */
+	frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
+
 
 	mutex_lock(&ddata->lock);
 	if (state->period != ddata->approx_period) {
-- 
2.40.0

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

[Emil Renner Berthing]

On Thu, 20 Apr 2023 at 11:35, Nylon Chen <nylon.chen@sifive.com> wrote:
>
> 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.
>
> The reference is SiFive FU740-C000 Manual[0]
>
> Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> ---
>  drivers/pwm/pwm-sifive.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> index 393a4b97fc19..d5d5f36da297 100644
> --- a/drivers/pwm/pwm-sifive.c
> +++ b/drivers/pwm/pwm-sifive.c
> @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  {
>         struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
>         struct pwm_state cur_state;
> -       unsigned int duty_cycle;
> +       unsigned int duty_cycle, period;
>         unsigned long long num;
>         bool enabled;
>         int ret = 0;
>         u32 frac;
>
> -       if (state->polarity != PWM_POLARITY_INVERSED)
> +       if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
>                 return -EINVAL;
>
>         cur_state = pwm->state;
> @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>          * calculating the register values first and then writing them
>          * consecutively
>          */
> +       period = max(state->period, ddata->approx_period);

Hi Nylon,

I don't understand this patch. You introduce this new variable,
period, and set it here but you never seem to use it. If you planned
to use it instead of state->period below, why should it be the max of
the old period and what is requested? What happens if the consumer
wants to lower the period?

Also above you now allow both PWM_POLARITY_NORMAL and
PWM_POLARITY_INVERSED but you treat both cases the same.

/Emil

>         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
>         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> -       /* The hardware cannot generate a 100% duty cycle */
>         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> +       /* The hardware cannot generate a 100% duty cycle */
> +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> +
>
>         mutex_lock(&ddata->lock);
>         if (state->period != ddata->approx_period) {
> --
> 2.40.0
>

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

[Emil Renner Berthing]

On Thu, 20 Apr 2023 at 12:41, Nylon Chen <nylon.chen@sifive.com> wrote:
>
> Hi, Emil
>
> Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日 週四 下午6:04寫道：
> >
> > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <nylon.chen@sifive.com> wrote:
> > >
> > > 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.
> > >
> > > The reference is SiFive FU740-C000 Manual[0]
> > >
> > > Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> > >
> > > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > > ---
> > >  drivers/pwm/pwm-sifive.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > index 393a4b97fc19..d5d5f36da297 100644
> > > --- a/drivers/pwm/pwm-sifive.c
> > > +++ b/drivers/pwm/pwm-sifive.c
> > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  {
> > >         struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > >         struct pwm_state cur_state;
> > > -       unsigned int duty_cycle;
> > > +       unsigned int duty_cycle, period;
> > >         unsigned long long num;
> > >         bool enabled;
> > >         int ret = 0;
> > >         u32 frac;
> > >
> > > -       if (state->polarity != PWM_POLARITY_INVERSED)
> > > +       if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > >                 return -EINVAL;
> > >
> > >         cur_state = pwm->state;
> > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >          * calculating the register values first and then writing them
> > >          * consecutively
> > >          */
> > > +       period = max(state->period, ddata->approx_period);
> >
> > Hi Nylon,
> >
> > I don't understand this patch. You introduce this new variable,
> > period, and set it here but you never seem to use it. If you planned
> > to use it instead of state->period below, why should it be the max of
> > the old period and what is requested? What happens if the consumer
> > wants to lower the period?
> Sorry this was an oversight on my part, there was a line correction that didn't change to
> - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> + frac = DIV64_U64_ROUND_CLOSEST(num, period);

I see, so then my second question was why period needs to be the
larger of the previous period and the requested period.

What happens if the requested period, state->period, is lower than the
old period, ddata->approx_period? Then the period will be changed to
state->period below, but the calculations will be made using period =
ddata->approx_period, right?

> >
> > Also above you now allow both PWM_POLARITY_NORMAL and
> > PWM_POLARITY_INVERSED but you treat both cases the same.
> I may have misunderstood what Uwe means here, I will confirm again here
> >
> > /Emil
> >
> > >         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> > >         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > -       /* The hardware cannot generate a 100% duty cycle */
> > >         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > +       /* The hardware cannot generate a 100% duty cycle */
> > > +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > +
> > >
> > >         mutex_lock(&ddata->lock);
> > >         if (state->period != ddata->approx_period) {
> > > --
> > > 2.40.0
> > >

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

[Nylon Chen]

Hi Emil,

Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日
週四 下午6:46寫道：
>
> On Thu, 20 Apr 2023 at 12:41, Nylon Chen <nylon.chen@sifive.com> wrote:
> >
> > Hi, Emil
> >
> > Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日 週四 下午6:04寫道：
> > >
> > > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <nylon.chen@sifive.com> wrote:
> > > >
> > > > 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.
> > > >
> > > > The reference is SiFive FU740-C000 Manual[0]
> > > >
> > > > Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> > > >
> > > > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > > > ---
> > > >  drivers/pwm/pwm-sifive.c | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > > index 393a4b97fc19..d5d5f36da297 100644
> > > > --- a/drivers/pwm/pwm-sifive.c
> > > > +++ b/drivers/pwm/pwm-sifive.c
> > > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >  {
> > > >         struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > > >         struct pwm_state cur_state;
> > > > -       unsigned int duty_cycle;
> > > > +       unsigned int duty_cycle, period;
> > > >         unsigned long long num;
> > > >         bool enabled;
> > > >         int ret = 0;
> > > >         u32 frac;
> > > >
> > > > -       if (state->polarity != PWM_POLARITY_INVERSED)
> > > > +       if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > > >                 return -EINVAL;
> > > >
> > > >         cur_state = pwm->state;
> > > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >          * calculating the register values first and then writing them
> > > >          * consecutively
> > > >          */
> > > > +       period = max(state->period, ddata->approx_period);
> > >
> > > Hi Nylon,
> > >
> > > I don't understand this patch. You introduce this new variable,
> > > period, and set it here but you never seem to use it. If you planned
> > > to use it instead of state->period below, why should it be the max of
> > > the old period and what is requested? What happens if the consumer
> > > wants to lower the period?
> > Sorry this was an oversight on my part, there was a line correction that didn't change to
> > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > + frac = DIV64_U64_ROUND_CLOSEST(num, period);
>
> I see, so then my second question was why period needs to be the
> larger of the previous period and the requested period.
>
> What happens if the requested period, state->period, is lower than the
> old period, ddata->approx_period? Then the period will be changed to
> state->period below, but the calculations will be made using period =
> ddata->approx_period, right?

Your understanding is correct. According to the new algorithm proposed
by Uwe, the goal is to:
Pick the biggest period length possible that is not bigger than the
requested period.
>
> > >
> > > Also above you now allow both PWM_POLARITY_NORMAL and
> > > PWM_POLARITY_INVERSED but you treat both cases the same.
> > I may have misunderstood what Uwe means here, I will confirm again here
> > >
> > > /Emil
> > >
> > > >         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> > > >         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > > -       /* The hardware cannot generate a 100% duty cycle */
> > > >         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > > +       /* The hardware cannot generate a 100% duty cycle */
> > > > +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > > +
> > > >
> > > >         mutex_lock(&ddata->lock);
> > > >         if (state->period != ddata->approx_period) {
> > > > --
> > > > 2.40.0
> > > >

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

[Emil Renner Berthing]

On Fri, 21 Apr 2023 at 08:16, Nylon Chen <nylon.chen@sifive.com> wrote:
>
> Hi Emil,
>
> Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日
> 週四 下午6:46寫道：
> >
> > On Thu, 20 Apr 2023 at 12:41, Nylon Chen <nylon.chen@sifive.com> wrote:
> > >
> > > Hi, Emil
> > >
> > > Emil Renner Berthing <emil.renner.berthing@canonical.com> 於 2023年4月20日 週四 下午6:04寫道：
> > > >
> > > > On Thu, 20 Apr 2023 at 11:35, Nylon Chen <nylon.chen@sifive.com> wrote:
> > > > >
> > > > > 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.
> > > > >
> > > > > The reference is SiFive FU740-C000 Manual[0]
> > > > >
> > > > > Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> > > > >
> > > > > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> > > > > Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
> > > > > ---
> > > > >  drivers/pwm/pwm-sifive.c | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c
> > > > > index 393a4b97fc19..d5d5f36da297 100644
> > > > > --- a/drivers/pwm/pwm-sifive.c
> > > > > +++ b/drivers/pwm/pwm-sifive.c
> > > > > @@ -132,13 +132,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > >  {
> > > > >         struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
> > > > >         struct pwm_state cur_state;
> > > > > -       unsigned int duty_cycle;
> > > > > +       unsigned int duty_cycle, period;
> > > > >         unsigned long long num;
> > > > >         bool enabled;
> > > > >         int ret = 0;
> > > > >         u32 frac;
> > > > >
> > > > > -       if (state->polarity != PWM_POLARITY_INVERSED)
> > > > > +       if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
> > > > >                 return -EINVAL;
> > > > >
> > > > >         cur_state = pwm->state;
> > > > > @@ -154,10 +154,13 @@ static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > >          * calculating the register values first and then writing them
> > > > >          * consecutively
> > > > >          */
> > > > > +       period = max(state->period, ddata->approx_period);
> > > >
> > > > Hi Nylon,
> > > >
> > > > I don't understand this patch. You introduce this new variable,
> > > > period, and set it here but you never seem to use it. If you planned
> > > > to use it instead of state->period below, why should it be the max of
> > > > the old period and what is requested? What happens if the consumer
> > > > wants to lower the period?
> > > Sorry this was an oversight on my part, there was a line correction that didn't change to
> > > - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > + frac = DIV64_U64_ROUND_CLOSEST(num, period);
> >
> > I see, so then my second question was why period needs to be the
> > larger of the previous period and the requested period.
> >
> > What happens if the requested period, state->period, is lower than the
> > old period, ddata->approx_period? Then the period will be changed to
> > state->period below, but the calculations will be made using period =
> > ddata->approx_period, right?
>
> Your understanding is correct. According to the new algorithm proposed
> by Uwe, the goal is to:
> Pick the biggest period length possible that is not bigger than the
> requested period.

Right, and to be clear: this patch doesn't do that.

If the previous period in ddata->approx_period is bigger than the
requested period in state->period, it will do the frac calculations
with the old period, but still set the period to the shorter requested
period.

> > > >
> > > > Also above you now allow both PWM_POLARITY_NORMAL and
> > > > PWM_POLARITY_INVERSED but you treat both cases the same.
> > > I may have misunderstood what Uwe means here, I will confirm again here
> > > >
> > > > /Emil
> > > >
> > > > >         num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
> > > > >         frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > > > > -       /* The hardware cannot generate a 100% duty cycle */
> > > > >         frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > > > +       /* The hardware cannot generate a 100% duty cycle */
> > > > > +       frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
> > > > > +
> > > > >
> > > > >         mutex_lock(&ddata->lock);
> > > > >         if (state->period != ddata->approx_period) {
> > > > > --
> > > > > 2.40.0
> > > > >

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

[Conor Dooley]

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

On Thu, Apr 20, 2023 at 05:34:57PM +0800, Nylon Chen wrote:
> 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.
> 
> The reference is SiFive FU740-C000 Manual[0]
> 
> Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Hmm, I don't recall reviewing or acking this patch. I do recalling doing
it for 1/2 though:
https://lore.kernel.org/linux-pwm/Y9len4GinXQ101xr@spud/

Please remove these from your next submission, I don't have any knowledge
of this driver nor do I maintain it, thanks.

> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>

This SoB is new too AFAICT and looks a bit odd.
Should there be a Co-developed-by for Vincent?

Thanks,
Conor.

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

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

[kernel test robot]

Hi Nylon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.3-rc7 next-20230419]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230420/202304202141.JYCKBVOQ-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
        git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304202141.JYCKBVOQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:26,
                    from include/linux/clk.h:13,
                    from drivers/pwm/pwm-sifive.c:14:
   drivers/pwm/pwm-sifive.c: In function 'pwm_sifive_apply':
   include/linux/minmax.h:20:35: warning: comparison of distinct pointer types lacks a cast
      20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
         |                                   ^~
   include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
      26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
         |                  ^~~~~~~~~~~
   include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
      36 |         __builtin_choose_expr(__safe_cmp(x, y), \
         |                               ^~~~~~~~~~
   include/linux/minmax.h:74:25: note: in expansion of macro '__careful_cmp'
      74 | #define max(x, y)       __careful_cmp(x, y, >)
         |                         ^~~~~~~~~~~~~
   drivers/pwm/pwm-sifive.c:157:18: note: in expansion of macro 'max'
     157 |         period = max(state->period, ddata->approx_period);
         |                  ^~~
>> drivers/pwm/pwm-sifive.c:135:34: warning: variable 'period' set but not used [-Wunused-but-set-variable]
     135 |         unsigned int duty_cycle, period;
         |                                  ^~~~~~


vim +/period +135 drivers/pwm/pwm-sifive.c

   129	
   130	static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
   131				    const struct pwm_state *state)
   132	{
   133		struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
   134		struct pwm_state cur_state;
 > 135		unsigned int duty_cycle, period;
   136		unsigned long long num;
   137		bool enabled;
   138		int ret = 0;
   139		u32 frac;
   140	
   141		if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
   142			return -EINVAL;
   143	
   144		cur_state = pwm->state;
   145		enabled = cur_state.enabled;
   146	
   147		duty_cycle = state->duty_cycle;
   148		if (!state->enabled)
   149			duty_cycle = 0;
   150	
   151		/*
   152		 * The problem of output producing mixed setting as mentioned at top,
   153		 * occurs here. To minimize the window for this problem, we are
   154		 * calculating the register values first and then writing them
   155		 * consecutively
   156		 */
   157		period = max(state->period, ddata->approx_period);
   158		num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
   159		frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
   160		frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
   161		/* The hardware cannot generate a 100% duty cycle */
   162		frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
   163	
   164	
   165		mutex_lock(&ddata->lock);
   166		if (state->period != ddata->approx_period) {
   167			/*
   168			 * Don't let a 2nd user change the period underneath the 1st user.
   169			 * However if ddate->approx_period == 0 this is the first time we set
   170			 * any period, so let whoever gets here first set the period so other
   171			 * users who agree on the period won't fail.
   172			 */
   173			if (ddata->user_count != 1 && ddata->approx_period) {
   174				mutex_unlock(&ddata->lock);
   175				return -EBUSY;
   176			}
   177			ddata->approx_period = state->period;
   178			pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
   179		}
   180		mutex_unlock(&ddata->lock);
   181	
   182		/*
   183		 * If the PWM is enabled the clk is already on. So only enable it
   184		 * conditionally to have it on exactly once afterwards independent of
   185		 * the PWM state.
   186		 */
   187		if (!enabled) {
   188			ret = clk_enable(ddata->clk);
   189			if (ret) {
   190				dev_err(ddata->chip.dev, "Enable clk failed\n");
   191				return ret;
   192			}
   193		}
   194	
   195		writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
   196	
   197		if (!state->enabled)
   198			clk_disable(ddata->clk);
   199	
   200		return 0;
   201	}
   202	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[kernel test robot]

Hi Nylon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.3-rc7 next-20230421]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: sparc64-randconfig-s031-20230421 (https://download.01.org/0day-ci/archive/20230422/202304222135.B9PoQ5w3-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
        git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/pwm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304222135.B9PoQ5w3-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/pwm/pwm-sifive.c:157:18: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> drivers/pwm/pwm-sifive.c:157:18: sparse:    unsigned long long const *
>> drivers/pwm/pwm-sifive.c:157:18: sparse:    unsigned int *

vim +157 drivers/pwm/pwm-sifive.c

   129	
   130	static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
   131				    const struct pwm_state *state)
   132	{
   133		struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
   134		struct pwm_state cur_state;
   135		unsigned int duty_cycle, period;
   136		unsigned long long num;
   137		bool enabled;
   138		int ret = 0;
   139		u32 frac;
   140	
   141		if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
   142			return -EINVAL;
   143	
   144		cur_state = pwm->state;
   145		enabled = cur_state.enabled;
   146	
   147		duty_cycle = state->duty_cycle;
   148		if (!state->enabled)
   149			duty_cycle = 0;
   150	
   151		/*
   152		 * The problem of output producing mixed setting as mentioned at top,
   153		 * occurs here. To minimize the window for this problem, we are
   154		 * calculating the register values first and then writing them
   155		 * consecutively
   156		 */
 > 157		period = max(state->period, ddata->approx_period);
   158		num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
   159		frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
   160		frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
   161		/* The hardware cannot generate a 100% duty cycle */
   162		frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
   163	
   164	
   165		mutex_lock(&ddata->lock);
   166		if (state->period != ddata->approx_period) {
   167			/*
   168			 * Don't let a 2nd user change the period underneath the 1st user.
   169			 * However if ddate->approx_period == 0 this is the first time we set
   170			 * any period, so let whoever gets here first set the period so other
   171			 * users who agree on the period won't fail.
   172			 */
   173			if (ddata->user_count != 1 && ddata->approx_period) {
   174				mutex_unlock(&ddata->lock);
   175				return -EBUSY;
   176			}
   177			ddata->approx_period = state->period;
   178			pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
   179		}
   180		mutex_unlock(&ddata->lock);
   181	
   182		/*
   183		 * If the PWM is enabled the clk is already on. So only enable it
   184		 * conditionally to have it on exactly once afterwards independent of
   185		 * the PWM state.
   186		 */
   187		if (!enabled) {
   188			ret = clk_enable(ddata->clk);
   189			if (ret) {
   190				dev_err(ddata->chip.dev, "Enable clk failed\n");
   191				return ret;
   192			}
   193		}
   194	
   195		writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
   196	
   197		if (!state->enabled)
   198			clk_disable(ddata->clk);
   199	
   200		return 0;
   201	}
   202	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[kernel test robot]

Hi Nylon,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on thierry-reding-pwm/for-next rockchip/for-next linus/master v6.4-rc1 next-20230508]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230420093457.18936-3-nylon.chen%40sifive.com
patch subject: [PATCH v3 2/2] pwm: sifive: change the PWM controlled LED algorithm
config: powerpc-randconfig-s041-20230507 (https://download.01.org/0day-ci/archive/20230508/202305081759.wgN4Q80I-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nylon-Chen/riscv-dts-sifive-unleashed-unmatched-Remove-PWM-controlled-LED-s-active-low-properties/20230420-173619
        git checkout f2d706bf61190a45a8f90f1f455bc943d4ac7b6e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/pwm/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305081759.wgN4Q80I-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/pwm/pwm-sifive.c:157:18: sparse: sparse: incompatible types in comparison expression (different type sizes):
>> drivers/pwm/pwm-sifive.c:157:18: sparse:    unsigned long long const *
>> drivers/pwm/pwm-sifive.c:157:18: sparse:    unsigned int *

vim +157 drivers/pwm/pwm-sifive.c

   129	
   130	static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *pwm,
   131				    const struct pwm_state *state)
   132	{
   133		struct pwm_sifive_ddata *ddata = pwm_sifive_chip_to_ddata(chip);
   134		struct pwm_state cur_state;
   135		unsigned int duty_cycle, period;
   136		unsigned long long num;
   137		bool enabled;
   138		int ret = 0;
   139		u32 frac;
   140	
   141		if (state->polarity != PWM_POLARITY_NORMAL && state->polarity != PWM_POLARITY_INVERSED)
   142			return -EINVAL;
   143	
   144		cur_state = pwm->state;
   145		enabled = cur_state.enabled;
   146	
   147		duty_cycle = state->duty_cycle;
   148		if (!state->enabled)
   149			duty_cycle = 0;
   150	
   151		/*
   152		 * The problem of output producing mixed setting as mentioned at top,
   153		 * occurs here. To minimize the window for this problem, we are
   154		 * calculating the register values first and then writing them
   155		 * consecutively
   156		 */
 > 157		period = max(state->period, ddata->approx_period);
   158		num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH);
   159		frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
   160		frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
   161		/* The hardware cannot generate a 100% duty cycle */
   162		frac = (1U << PWM_SIFIVE_CMPWIDTH) - 1 - frac;
   163	
   164	
   165		mutex_lock(&ddata->lock);
   166		if (state->period != ddata->approx_period) {
   167			/*
   168			 * Don't let a 2nd user change the period underneath the 1st user.
   169			 * However if ddate->approx_period == 0 this is the first time we set
   170			 * any period, so let whoever gets here first set the period so other
   171			 * users who agree on the period won't fail.
   172			 */
   173			if (ddata->user_count != 1 && ddata->approx_period) {
   174				mutex_unlock(&ddata->lock);
   175				return -EBUSY;
   176			}
   177			ddata->approx_period = state->period;
   178			pwm_sifive_update_clock(ddata, clk_get_rate(ddata->clk));
   179		}
   180		mutex_unlock(&ddata->lock);
   181	
   182		/*
   183		 * If the PWM is enabled the clk is already on. So only enable it
   184		 * conditionally to have it on exactly once afterwards independent of
   185		 * the PWM state.
   186		 */
   187		if (!enabled) {
   188			ret = clk_enable(ddata->clk);
   189			if (ret) {
   190				dev_err(ddata->chip.dev, "Enable clk failed\n");
   191				return ret;
   192			}
   193		}
   194	
   195		writel(frac, ddata->regs + PWM_SIFIVE_PWMCMP(pwm->hwpwm));
   196	
   197		if (!state->enabled)
   198			clk_disable(ddata->clk);
   199	
   200		return 0;
   201	}
   202	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests