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

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

[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 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                            | 1 +
 3 files changed, 1 insertion(+), 8 deletions(-)

-- 
2.36.1

[PATCH v2 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].

[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

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

[PATCH v2 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].

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

Signed-off-by: Nylon Chen <nylon.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 v2 2/2] pwm: sifive: change the PWM controlled LED algorithm

[Geert Uytterhoeven]

Hi Nylon,

On Mon, Jan 30, 2023 at 10:32 AM 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].
>
> [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
>
> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>

Thanks for your patch!

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

Shouldn't the inversion be done before the hardware limitation fixup?

>
>         mutex_lock(&ddata->lock);
>         if (state->period != ddata->approx_period) {

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Uwe Kleine-König]

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

On Mon, Jan 30, 2023 at 05:32:29PM +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].
> 
> [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> 
> Signed-off-by: Nylon Chen <nylon.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;

The same problem exists in pwm_sifive_get_state(), doesn't it?

As fixing this is an interruptive change anyhow, this is the opportunity
to align the driver to the rules tested by PWM_DEBUG.

The problems I see in the driver (only checked quickly, so I might be
wrong):

 - state->period != ddata->approx_period isn't necessarily a problem. If
   state->period > ddata->real_period that's fine and the driver should
   continue

 - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
   is wrong for two reasons:
   it should round down and use the real period.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

[Conor Dooley]

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

Hey Nylon,

On Mon, Jan 30, 2023 at 05:32:28PM +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].
> 
> [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

Ideally these would be:
Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
so that they integrate nice with the git trailers mechanism.
If you resend, please update them to regular link tags.

I checked out the circuits last time around and agreed that they should
not be active-low.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

I expect that both patches will go through the PWM tree together, so:
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

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

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

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

[Nylon Chen]

Hi Geert,

Thanks for your reply.

Geert Uytterhoeven <geert@linux-m68k.org> 於 2023年1月30日 週一 下午5:53寫道：
>
> Hi Nylon,
>
> On Mon, Jan 30, 2023 at 10:32 AM 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].
> >
> > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> >
> > Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
>
> Thanks for your patch!
>
> > --- 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;
>
> Shouldn't the inversion be done before the hardware limitation fixup?
I think your inference is correct, I will use it.

thanks a lot.
>
> >
> >         mutex_lock(&ddata->lock);
> >         if (state->period != ddata->approx_period) {
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

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

[Nylon Chen]

Hi Uwe,

Thanks for your reply.

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道：
>
> On Mon, Jan 30, 2023 at 05:32:29PM +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].
> >
> > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> >
> > Signed-off-by: Nylon Chen <nylon.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;
>
> The same problem exists in pwm_sifive_get_state(), doesn't it?
>
> As fixing this is an interruptive change anyhow, this is the opportunity
> to align the driver to the rules tested by PWM_DEBUG.
>
> The problems I see in the driver (only checked quickly, so I might be
> wrong):
>
>  - state->period != ddata->approx_period isn't necessarily a problem. If
>    state->period > ddata->real_period that's fine and the driver should
>    continue
>
>  - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
>    is wrong for two reasons:
>    it should round down and use the real period.
>
I need a little time to clarify your assumptions. If possible, I will
make similar changes.

e.g.
rounddown(num, state->period);
if (state->period < ddata->approx_period)
    ...

thanks a lot.






> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Nylon Chen]

Hi Conor,

Thanks for your reply.

Conor Dooley <conor@kernel.org> 於 2023年2月1日 週三 上午2:32寫道：

>
> Hey Nylon,
>
> On Mon, Jan 30, 2023 at 05:32:28PM +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].
> >
> > [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
>
> Ideally these would be:
> Link: https://sifive.cdn.prismic.io/sifive/c52a8e32-05ce-4aaf-95c8-7bf8453f8698_hifive-unleashed-a00-schematics-1.pdf [0]
> so that they integrate nice with the git trailers mechanism.
> If you resend, please update them to regular link tags.
>
I will fix this issue.
> I checked out the circuits last time around and agreed that they should
> not be active-low.
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> I expect that both patches will go through the PWM tree together, so:
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>
> Thanks,
> Conor.
>
Thanks a lot.

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

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

[Nylon Chen]

Hi Uwe,

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道：
>
> On Mon, Jan 30, 2023 at 05:32:29PM +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].
> >
> > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> >
> > Signed-off-by: Nylon Chen <nylon.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;
>
> The same problem exists in pwm_sifive_get_state(), doesn't it?
>
> As fixing this is an interruptive change anyhow, this is the opportunity
> to align the driver to the rules tested by PWM_DEBUG.
>
> The problems I see in the driver (only checked quickly, so I might be
> wrong):
>

>  - state->period != ddata->approx_period isn't necessarily a problem. If
>    state->period > ddata->real_period that's fine and the driver should
>    continue
>
>  - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
>    is wrong for two reasons:
>    it should round down and use the real period.
are you mean state->period is a redundancy variable so we can use
ddata->real_period directly?

it seems reasonable, but I don't get your point, why do we need to
change the algorithm to DIV_ROUND_DOWN_ULL() and change the if-else
condition.

frac = DIV_ROUND_DOWN_ULL(num, ddata->real_period);
if (state->period < ddata->approx_period) {
    ...
}

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Nylon Chen]

Hi Uwe,

Nylon Chen <nylon.chen@sifive.com> 於 2023年2月3日 週五 下午4:06寫道：
>
> Hi Uwe,
>
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道：
> >
> > On Mon, Jan 30, 2023 at 05:32:29PM +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].
> > >
> > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > >
> > > Signed-off-by: Nylon Chen <nylon.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;
> >
> > The same problem exists in pwm_sifive_get_state(), doesn't it?
> >
> > As fixing this is an interruptive change anyhow, this is the opportunity
> > to align the driver to the rules tested by PWM_DEBUG.
> >
> > The problems I see in the driver (only checked quickly, so I might be
> > wrong):
> >
>
> >  - state->period != ddata->approx_period isn't necessarily a problem. If
> >    state->period > ddata->real_period that's fine and the driver should
> >    continue
> >
> >  - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> >    is wrong for two reasons:
> >    it should round down and use the real period.
I have some results from my observations regarding the questions you raised.

I don't know if what we are thinking is the same thing.

If my assumptions are different from yours, please let me know. Thanks.
> are you mean state->period is a redundancy variable so we can use
> ddata->real_period directly?
>
> it seems reasonable, but I don't get your point, why do we need to
> change the algorithm to DIV_ROUND_DOWN_ULL() and change the if-else
> condition.
>
> frac = DIV_ROUND_DOWN_ULL(num, ddata->real_period);
> if (state->period < ddata->approx_period) {
>     ...
> }
>
> >
> > Best regards
> > Uwe
> >
> > --
> > Pengutronix e.K.                           | Uwe Kleine-König            |
> > Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Uwe Kleine-König]

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

Hello Nylon,

On Wed, Feb 01, 2023 at 04:56:42PM +0800, Nylon Chen wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道：
> > On Mon, Jan 30, 2023 at 05:32:29PM +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].
> > >
> > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > >
> > > Signed-off-by: Nylon Chen <nylon.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;
> >
> > The same problem exists in pwm_sifive_get_state(), doesn't it?
> >
> > As fixing this is an interruptive change anyhow, this is the opportunity
> > to align the driver to the rules tested by PWM_DEBUG.
> >
> > The problems I see in the driver (only checked quickly, so I might be
> > wrong):
> >
> >  - state->period != ddata->approx_period isn't necessarily a problem. If
> >    state->period > ddata->real_period that's fine and the driver should
> >    continue
> >
> >  - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> >    is wrong for two reasons:
> >    it should round down and use the real period.
> >
> I need a little time to clarify your assumptions. If possible, I will
> make similar changes.
> 
> e.g.
> rounddown(num, state->period);
> if (state->period < ddata->approx_period)
>     ...

the idea is that for a given request apply should do the following to
select the hardware setting:

 - Check polarity, if the hardware doesn't support it, return -EINVAL.
   (A period always starts with the active phase for the duration of
   duty_cycle. For normal polarity active = high.)
 - Pick the biggest period length possible that is not bigger than the
   requested period.
 - For the picked period, select the biggest duty_cycle possible that is
   not bigger than the requested duty_cycle.

Then if possible switch to the selected setting in an atomic step.

Does this clearify your doubts?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

[Nylon Chen]

Hi Uwe

Thanks for your reply.

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年3月1日 週三 下午5:21寫道：
>
> Hello Nylon,
>
> On Wed, Feb 01, 2023 at 04:56:42PM +0800, Nylon Chen wrote:
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道：
> > > On Mon, Jan 30, 2023 at 05:32:29PM +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].
> > > >
> > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > > >
> > > > Signed-off-by: Nylon Chen <nylon.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;
> > >
> > > The same problem exists in pwm_sifive_get_state(), doesn't it?
> > >
> > > As fixing this is an interruptive change anyhow, this is the opportunity
> > > to align the driver to the rules tested by PWM_DEBUG.
> > >
> > > The problems I see in the driver (only checked quickly, so I might be
> > > wrong):
> > >
> > >  - state->period != ddata->approx_period isn't necessarily a problem. If
> > >    state->period > ddata->real_period that's fine and the driver should
> > >    continue
> > >
> > >  - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
> > >    is wrong for two reasons:
> > >    it should round down and use the real period.
> > >
> > I need a little time to clarify your assumptions. If possible, I will
> > make similar changes.
> >
> > e.g.
> > rounddown(num, state->period);
> > if (state->period < ddata->approx_period)
> >     ...
>
> the idea is that for a given request apply should do the following to
> select the hardware setting:
>
>  - Check polarity, if the hardware doesn't support it, return -EINVAL.
>    (A period always starts with the active phase for the duration of
>    duty_cycle. For normal polarity active = high.)
>  - Pick the biggest period length possible that is not bigger than the
>    requested period.
>  - For the picked period, select the biggest duty_cycle possible that is
>    not bigger than the requested duty_cycle.
>
> Then if possible switch to the selected setting in an atomic step.
>
> Does this clearify your doubts?
I need a little time to clarify your assumptions. Thanks again.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Nylon Chen]

Hi Uwe,

Sorry it's so long ago.

I have completed the implementation of the new version, but there is
one thing about this letter that I still don't quite understand.

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道：
>
> On Mon, Jan 30, 2023 at 05:32:29PM +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].
> >
> > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> >
> > Signed-off-by: Nylon Chen <nylon.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;
>
> The same problem exists in pwm_sifive_get_state(), doesn't it?
>
> As fixing this is an interruptive change anyhow, this is the opportunity
> to align the driver to the rules tested by PWM_DEBUG.
>
> The problems I see in the driver (only checked quickly, so I might be
> wrong):
>
>  - state->period != ddata->approx_period isn't necessarily a problem. If
>    state->period > ddata->real_period that's fine and the driver should
>    continue
>
I still don’t quite understand the description of this paragraph.

state->period != ddate->approx_period seems to be used to compare the
results of the previous and next two times.

I'm unsure what to do if I replace the conditional expression with
something else.

In addition, I don't understand the meaning of this.
"if state->period > ddata->real_period that's fine, and the driver
should continue"

At present, my new version of the implementation has passed the test
of the pwm_apply_state_debug() function.

Would you suggest I send the new implementation version before
continuing the discussion?

Thank you again for everything you’ve done.

>  - frac = DIV64_U64_ROUND_CLOSEST(num, state->period);
>    is wrong for two reasons:
>    it should round down and use the real period.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

[Uwe Kleine-König]

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

Hello,

On Fri, Sep 08, 2023 at 06:41:00PM +0800, Nylon Chen wrote:
> Sorry it's so long ago.
> 
> I have completed the implementation of the new version, but there is
> one thing about this letter that I still don't quite understand.
> 
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道：
> >
> > On Mon, Jan 30, 2023 at 05:32:29PM +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].
> > >
> > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > >
> > > Signed-off-by: Nylon Chen <nylon.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;
> >
> > The same problem exists in pwm_sifive_get_state(), doesn't it?
> >
> > As fixing this is an interruptive change anyhow, this is the opportunity
> > to align the driver to the rules tested by PWM_DEBUG.
> >
> > The problems I see in the driver (only checked quickly, so I might be
> > wrong):
> >
> >  - state->period != ddata->approx_period isn't necessarily a problem. If
> >    state->period > ddata->real_period that's fine and the driver should
> >    continue
>
> I still don’t quite understand the description of this paragraph.
> 
> state->period != ddate->approx_period seems to be used to compare the
> results of the previous and next two times.

There are two things to consider:

 - usually the hardware doesn't support all requestable states because
   the hardware's quantum is > 1 ns. That is, it might for example
   support periods in the form (160 ns * i / 3) for i in 1 .. 1023.

   If this hardware runs with i = 500 (that is period ~= 26666.66
   ns) because the first channel is configured to run with period =
   26667, and .request is called for the 2nd channel with .period =
   26700 ns, there is no need to refuse that, because 26666.66 is the
   best possible approximation for 26700 ns anyhow.

 - .apply is supposed to implement the highest possible period that
   isn't bigger than the requested period. So in the above case even if
   the hardware runs at 26666.66 ns without the possibility to change
   that, a request to configure for period = 30000 ns could succeed (and
   keep 26666.66 ns).

> Would you suggest I send the new implementation version before
> continuing the discussion?

Note that the above implements the optimal behaviour for a driver.
(For some definition of "optimal" that admittedly also yields strange
behaviour at times. The reasoning for this to the be thing to implement
is, that's the corner cases are easier to implement, idempotency is
possible and it's easier to work with than rounding to the nearest
possible value.)

While I'd like to see the sifive driver to implement this optimal
behaviour, it's not mandatory that you convert the driver to that
behaviour. Just make sure to not make it worse.

So to answer your question: If you understood what I wrote above and are
motivated to improve the driver, it would be great to do that before the
next review round.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

[Nylon Chen]

Hi Uwe

I'm glad the example was helpful to you. I will double-check and
verify my implementation once again.

Thank you for your assistance.

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年9月8日 週五 下午10:50寫道：
>
> Hello,
>
> On Fri, Sep 08, 2023 at 06:41:00PM +0800, Nylon Chen wrote:
> > Sorry it's so long ago.
> >
> > I have completed the implementation of the new version, but there is
> > one thing about this letter that I still don't quite understand.
> >
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 於 2023年1月30日 週一 下午6:17寫道：
> > >
> > > On Mon, Jan 30, 2023 at 05:32:29PM +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].
> > > >
> > > > [0]: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf
> > > >
> > > > Signed-off-by: Nylon Chen <nylon.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;
> > >
> > > The same problem exists in pwm_sifive_get_state(), doesn't it?
> > >
> > > As fixing this is an interruptive change anyhow, this is the opportunity
> > > to align the driver to the rules tested by PWM_DEBUG.
> > >
> > > The problems I see in the driver (only checked quickly, so I might be
> > > wrong):
> > >
> > >  - state->period != ddata->approx_period isn't necessarily a problem. If
> > >    state->period > ddata->real_period that's fine and the driver should
> > >    continue
> >
> > I still don’t quite understand the description of this paragraph.
> >
> > state->period != ddate->approx_period seems to be used to compare the
> > results of the previous and next two times.
>
> There are two things to consider:
>
>  - usually the hardware doesn't support all requestable states because
>    the hardware's quantum is > 1 ns. That is, it might for example
>    support periods in the form (160 ns * i / 3) for i in 1 .. 1023.
>
>    If this hardware runs with i = 500 (that is period ~= 26666.66
>    ns) because the first channel is configured to run with period =
>    26667, and .request is called for the 2nd channel with .period =
>    26700 ns, there is no need to refuse that, because 26666.66 is the
>    best possible approximation for 26700 ns anyhow.
>
>  - .apply is supposed to implement the highest possible period that
>    isn't bigger than the requested period. So in the above case even if
>    the hardware runs at 26666.66 ns without the possibility to change
>    that, a request to configure for period = 30000 ns could succeed (and
>    keep 26666.66 ns).
>
> > Would you suggest I send the new implementation version before
> > continuing the discussion?
>
> Note that the above implements the optimal behaviour for a driver.
> (For some definition of "optimal" that admittedly also yields strange
> behaviour at times. The reasoning for this to the be thing to implement
> is, that's the corner cases are easier to implement, idempotency is
> possible and it's easier to work with than rounding to the nearest
> possible value.)
>
> While I'd like to see the sifive driver to implement this optimal
> behaviour, it's not mandatory that you convert the driver to that
> behaviour. Just make sure to not make it worse.
>
> So to answer your question: If you understood what I wrote above and are
> motivated to improve the driver, it would be great to do that before the
> next review round.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |