On Thu, Feb 20, 2025 at 12:31:00PM +0200, Abel Vesa wrote:
> Currently, for the high resolution PWMs, the resolution, clock,
> pre-divider and exponent are being selected based on period. Basically,
Is this problem really limited to the high resolution channels?
> the implementation loops over each one of these and tries to find the
> closest (higher) period based on the following formula:
>
> period * refclk
> prediv_exp = log2 -------------------------------------
> NSEC_PER_SEC * pre_div * resolution
>
> Since the resolution is power of 2, the actual period resulting is
> usually higher than what the resolution allows. That's why the duty
> cycle requested needs to be capped to the maximum value allowed by the
> resolution (known as PWM size).
>
> Here is an example of how this can happen:
>
> For a requested period of 5000000, the best clock is 19.2MHz, the best
> prediv is 5, the best exponent is 6 and the best resolution is 256.
>
> Then, the pwm value is determined based on requested period and duty
> cycle, best prediv, best exponent and best clock, using the following
> formula:
>
> duty * refclk
> pwm_value = ----------------------------------------------
> NSEC_PER_SEC * prediv * (1 << prediv_exp)
>
> So in this specific scenario:
>
> (5000000 * 19200000) / (1000000000 * 5 * (1 << 64)) = 300
>
> With a resolution of 8 bits, this pwm value obviously goes over.
>
> Therefore, the max pwm value allowed needs to be 255.
>
> If not, the PMIC internal logic will only value that is under the set PWM
> size, resulting in a wrapped around PWM value.
>
> This has been observed on Lenovo Thinkpad T14s Gen6 (LCD panel version)
> which uses one of the PMK8550 to control the LCD backlight.
>
> Fix the value of the PWM by capping to a max based on the chosen
> resolution (PWM size).
>
I think you should be able to write this more succinct.
The important part of the problem description is that the requested
period will be rounded down to a possible hardware configuration, while
the duty cycle isn't. The calculated PWM_VALUE might therefor exceed the
calculated resolution, but the value is only capped to 15 bits for high
resolution channels.
Unless I'm misunderstanding Uwe's comment, this is how the API is
expected to work (although I was under the impression that we rounded
the period up, rather than down...)
Worth pointing out then is that, as there's a finite number of possible
periods that the hardware can operate at, you might want to tweak the
period given in the Devicetree to best facilitate the expected
brightness range.
And the same would go for my choice of NSEC_PER_MSEC for the non-PWM
code paths... I can't explain that choice...
> Cc: stable@xxxxxxxxxxxxxxx # 6.4
v6.4 is when b00d2ed37617 was introduced, so that's a given...
> Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> ---
> Note: This fix is blocking backlight support on Lenovo Thinkpad T14s
> Gen6 (LCD version), for which I have patches ready to send once this
> patch is agreed on (review) and merged.
> ---
> drivers/leds/rgb/leds-qcom-lpg.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
> unsigned int clk_rate;
>
> if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
> - max = LPG_RESOLUTION_15BIT - 1;
> + max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
This looks correct!
Regards,
Bjorn
> clk_rate = lpg_clk_rates_hi_res[chan->clk_sel];
> } else {
> max = LPG_RESOLUTION_9BIT - 1;
>
> ---
> base-commit: 50a0c754714aa3ea0b0e62f3765eb666a1579f24
> change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
>
> Best regards,
> --
> Abel Vesa <abel.vesa@xxxxxxxxxx>
>
