[PATCH] leds: qcom-lpg: Fix PWM period limits

[PATCH] leds: qcom-lpg: Fix PWM period limits

[Bjorn Andersson]

The introduction of high resolution PWM support moved the parenthesis of
the divisions in the calculation of min and max period. The result in
both divisions is in most cases truncation to 0, which limits the period
to the range of [0, 0].

Both numerators (and denominators) are within 64 bits, so the whole
expression can be put directly into the div64_u64, instead of doing it
partially.

Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---

This fixes the regression in v6.4-rc1.

 drivers/leds/rgb/leds-qcom-lpg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index c9cea797a697..7287fadc00df 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -312,14 +312,14 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
 		max_res = LPG_RESOLUTION_9BIT;
 	}
 
-	min_period = (u64)NSEC_PER_SEC *
-			div64_u64((1 << pwm_resolution_arr[0]), clk_rate_arr[clk_len - 1]);
+	min_period = div64_u64((u64)NSEC_PER_SEC * (1 << pwm_resolution_arr[0]),
+			       clk_rate_arr[clk_len - 1]);
 	if (period <= min_period)
 		return -EINVAL;
 
 	/* Limit period to largest possible value, to avoid overflows */
-	max_period = (u64)NSEC_PER_SEC * max_res * LPG_MAX_PREDIV *
-			div64_u64((1 << LPG_MAX_M), 1024);
+	max_period = div64_u64((u64)NSEC_PER_SEC * max_res * LPG_MAX_PREDIV * (1 << LPG_MAX_M),
+			       1024);
 	if (period > max_period)
 		period = max_period;
 
-- 
2.25.1

Re: [PATCH] leds: qcom-lpg: Fix PWM period limits

[Steev Klimaszewski]

On Fri, May 12, 2023 at 11:55 AM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
>
> The introduction of high resolution PWM support moved the parenthesis of
> the divisions in the calculation of min and max period. The result in
> both divisions is in most cases truncation to 0, which limits the period
> to the range of [0, 0].
>
> Both numerators (and denominators) are within 64 bits, so the whole
> expression can be put directly into the div64_u64, instead of doing it
> partially.
>
> Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>
> This fixes the regression in v6.4-rc1.
>
>  drivers/leds/rgb/leds-qcom-lpg.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index c9cea797a697..7287fadc00df 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -312,14 +312,14 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
>                 max_res = LPG_RESOLUTION_9BIT;
>         }
>
> -       min_period = (u64)NSEC_PER_SEC *
> -                       div64_u64((1 << pwm_resolution_arr[0]), clk_rate_arr[clk_len - 1]);
> +       min_period = div64_u64((u64)NSEC_PER_SEC * (1 << pwm_resolution_arr[0]),
> +                              clk_rate_arr[clk_len - 1]);
>         if (period <= min_period)
>                 return -EINVAL;
>
>         /* Limit period to largest possible value, to avoid overflows */
> -       max_period = (u64)NSEC_PER_SEC * max_res * LPG_MAX_PREDIV *
> -                       div64_u64((1 << LPG_MAX_M), 1024);
> +       max_period = div64_u64((u64)NSEC_PER_SEC * max_res * LPG_MAX_PREDIV * (1 << LPG_MAX_M),
> +                              1024);
>         if (period > max_period)
>                 period = max_period;
>
> --
> 2.25.1
>
Thank you!
Without this fix, the display never activates on the Thinkpad X13s
Tested on the Lenovo Thinkpad X13s
Tested-by: Steev Klimaszewski <steev@kali.org>

Re: [PATCH] leds: qcom-lpg: Fix PWM period limits

[Caleb Connolly]

On 12/05/2023 16:55, Bjorn Andersson wrote:
> The introduction of high resolution PWM support moved the parenthesis of
> the divisions in the calculation of min and max period. The result in
> both divisions is in most cases truncation to 0, which limits the period
> to the range of [0, 0].

Huh, TIL C gives multiplication and division the same precedence when
deciding order of operations.
> 
> Both numerators (and denominators) are within 64 bits, so the whole
> expression can be put directly into the div64_u64, instead of doing it
> partially.
> 
> Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>

Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
> 
> This fixes the regression in v6.4-rc1.
> 
>  drivers/leds/rgb/leds-qcom-lpg.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index c9cea797a697..7287fadc00df 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -312,14 +312,14 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
>  		max_res = LPG_RESOLUTION_9BIT;
>  	}
>  
> -	min_period = (u64)NSEC_PER_SEC *
> -			div64_u64((1 << pwm_resolution_arr[0]), clk_rate_arr[clk_len - 1]);
> +	min_period = div64_u64((u64)NSEC_PER_SEC * (1 << pwm_resolution_arr[0]),
> +			       clk_rate_arr[clk_len - 1]);
>  	if (period <= min_period)
>  		return -EINVAL;
>  
>  	/* Limit period to largest possible value, to avoid overflows */
> -	max_period = (u64)NSEC_PER_SEC * max_res * LPG_MAX_PREDIV *
> -			div64_u64((1 << LPG_MAX_M), 1024);
> +	max_period = div64_u64((u64)NSEC_PER_SEC * max_res * LPG_MAX_PREDIV * (1 << LPG_MAX_M),
> +			       1024);
>  	if (period > max_period)
>  		period = max_period;
>  

-- 
Kind Regards,
Caleb (they/them)

Re: [PATCH] leds: qcom-lpg: Fix PWM period limits

[Bjorn Andersson]

On Sat, May 13, 2023 at 10:09:49AM +0000, Caleb Connolly wrote:
> 
> 
> On 12/05/2023 16:55, Bjorn Andersson wrote:
> > The introduction of high resolution PWM support moved the parenthesis of
> > the divisions in the calculation of min and max period. The result in
> > both divisions is in most cases truncation to 0, which limits the period
> > to the range of [0, 0].
> 
> Huh, TIL C gives multiplication and division the same precedence when
> deciding order of operations.

There where no explicit parenthesis in the original implementation. So
I guess it would be more correct to state that parenthesis was
introduced around part of the expression?

Let me know if you think the wording matters and you would prefer me to
rewrite it.

Regards,
Bjorn

> > 
> > Both numerators (and denominators) are within 64 bits, so the whole
> > expression can be put directly into the div64_u64, instead of doing it
> > partially.
> > 
> > Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> 
> Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> > ---
> > 
> > This fixes the regression in v6.4-rc1.
> > 
> >  drivers/leds/rgb/leds-qcom-lpg.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > index c9cea797a697..7287fadc00df 100644
> > --- a/drivers/leds/rgb/leds-qcom-lpg.c
> > +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > @@ -312,14 +312,14 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
> >  		max_res = LPG_RESOLUTION_9BIT;
> >  	}
> >  
> > -	min_period = (u64)NSEC_PER_SEC *
> > -			div64_u64((1 << pwm_resolution_arr[0]), clk_rate_arr[clk_len - 1]);
> > +	min_period = div64_u64((u64)NSEC_PER_SEC * (1 << pwm_resolution_arr[0]),
> > +			       clk_rate_arr[clk_len - 1]);
> >  	if (period <= min_period)
> >  		return -EINVAL;
> >  
> >  	/* Limit period to largest possible value, to avoid overflows */
> > -	max_period = (u64)NSEC_PER_SEC * max_res * LPG_MAX_PREDIV *
> > -			div64_u64((1 << LPG_MAX_M), 1024);
> > +	max_period = div64_u64((u64)NSEC_PER_SEC * max_res * LPG_MAX_PREDIV * (1 << LPG_MAX_M),
> > +			       1024);
> >  	if (period > max_period)
> >  		period = max_period;
> >  
> 
> -- 
> Kind Regards,
> Caleb (they/them)

Re: [PATCH] leds: qcom-lpg: Fix PWM period limits

[Caleb Connolly]

On 15/05/2023 02:20, Bjorn Andersson wrote:
> On Sat, May 13, 2023 at 10:09:49AM +0000, Caleb Connolly wrote:
>>
>>
>> On 12/05/2023 16:55, Bjorn Andersson wrote:
>>> The introduction of high resolution PWM support moved the parenthesis of
>>> the divisions in the calculation of min and max period. The result in
>>> both divisions is in most cases truncation to 0, which limits the period
>>> to the range of [0, 0].
>>
>> Huh, TIL C gives multiplication and division the same precedence when
>> deciding order of operations.
> 
> There where no explicit parenthesis in the original implementation. So
> I guess it would be more correct to state that parenthesis was
> introduced around part of the expression?

It might be clearer just to say the div64_u64 changed the order of
operations.
> 
> Let me know if you think the wording matters and you would prefer me to
> rewrite it.

Yeah, that would be appreciated

Thanks
> 
> Regards,
> Bjorn
> 
-- 
Kind Regards,
Caleb (they/them)