| Message ID | 20230515162604.649203-1-quic_bjorande@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] leds: qcom-lpg: Fix PWM period limits | expand |
On Mon, May 15, 2023 at 09:26:04AM -0700, Bjorn Andersson wrote: > The introduction of high resolution PWM support changed the order of the > operations in the calculation of min and max period. The result in both > divisions is in most cases a 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") > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > Tested-by: Steev Klimaszewski <steev@kali.org> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> Tested-by: Johan Hovold <johan+linaro@kernel.org> Pavel or Lee, could you pick this one up for 6.4 as it fixes a regression (e.g. broken backlight on a number of laptops like the X13s)? > --- > > Changes since v1: > - Reworded first sentence to express that it's the order and not the > previously non-existent parenthesis that changed... > - Picked up review tags. > > 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; Johan
On 15/05/2023 18:26, Bjorn Andersson wrote: > The introduction of high resolution PWM support changed the order of the > operations in the calculation of min and max period. The result in both > divisions is in most cases a 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") > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > Tested-by: Steev Klimaszewski <steev@kali.org> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > > Changes since v1: > - Reworded first sentence to express that it's the order and not the > previously non-existent parenthesis that changed... > - Picked up review tags. > > 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; > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD
On Wed, 24 May 2023, Johan Hovold wrote: > On Mon, May 15, 2023 at 09:26:04AM -0700, Bjorn Andersson wrote: > > The introduction of high resolution PWM support changed the order of the > > operations in the calculation of min and max period. The result in both > > divisions is in most cases a 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") > > Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> > > Tested-by: Steev Klimaszewski <steev@kali.org> > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > Tested-by: Johan Hovold <johan+linaro@kernel.org> > > Pavel or Lee, could you pick this one up for 6.4 as it fixes a > regression (e.g. broken backlight on a number of laptops like the X13s)? I don't presently have any plans for a -fixes submission. If anyone else would like to submit it, please be my guest: Acked-by: Lee Jones <lee@kernel.org>
On Fri, Jun 02, 2023 at 10:19:28AM +0100, Lee Jones wrote: > On Wed, 24 May 2023, Johan Hovold wrote: > > Pavel or Lee, could you pick this one up for 6.4 as it fixes a > > regression (e.g. broken backlight on a number of laptops like the X13s)? > > I don't presently have any plans for a -fixes submission. > > If anyone else would like to submit it, please be my guest: > > Acked-by: Lee Jones <lee@kernel.org> That was not the answer I expected, but sure, I've sent it on to Linus: https://lore.kernel.org/lkml/ZHte5sPkB6-D-94G@hovoldconsulting.com/ Johan
On Sat, 03 Jun 2023, Johan Hovold wrote: > On Fri, Jun 02, 2023 at 10:19:28AM +0100, Lee Jones wrote: > > On Wed, 24 May 2023, Johan Hovold wrote: > > > > Pavel or Lee, could you pick this one up for 6.4 as it fixes a > > > regression (e.g. broken backlight on a number of laptops like the X13s)? > > > > I don't presently have any plans for a -fixes submission. > > > > If anyone else would like to submit it, please be my guest: > > > > Acked-by: Lee Jones <lee@kernel.org> > > That was not the answer I expected, but sure, I've sent it on to Linus: Sorry, soooo busy right now. Trying not to drop too many plates. > https://lore.kernel.org/lkml/ZHte5sPkB6-D-94G@hovoldconsulting.com/ *fist bump*
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;
The introduction of high resolution PWM support changed the order of the operations in the calculation of min and max period. The result in both divisions is in most cases a 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") Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org> Tested-by: Steev Klimaszewski <steev@kali.org> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> --- Changes since v1: - Reworded first sentence to express that it's the order and not the previously non-existent parenthesis that changed... - Picked up review tags. drivers/leds/rgb/leds-qcom-lpg.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)