Re: [PATCH] leds: rgb: leds-qcom-lpg: Fix pwm resolution for Hi-Res PWMs

[Abel Vesa <abel.vesa@linaro.org>]

On 25-02-21 00:35:08, Sebastian Reichel wrote:
> Hello Abel,
> 
> 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,
> > 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).
> > 
> > Cc: stable@vger.kernel.org    # 6.4
> > Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> > 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.
> > ---
> 
> Do you know if the pwm duty cycle to pwm value calculation is
> correct otherwise?

Sorry for the late reply.

Here is my understanding of the calculation of the pwm value currently
implemented.

First, find the best pre_div, refclk, resolution and prediv_exp by looping
through all refclk, resolution and prediv possible values, for the
following formula:

                         period * refclk
prediv_exp = log2 -------------------------------------
                    NSEC_PER_SEC * pre_div * (1 << resolution)


So in DT we set the period to 50000000. For this, as I mentioned in the
commit message the best refclk is 19.2MHz, the best prediv is 5, the best
exponent is 6 and the best resolution is 255.

So if you use these to compute the period following this formula:


                NSEC_PER_SEC * prediv * (1 << resolution)
best_period = -------------------------------------------
                             refclk

So in our case:

(1000000000 * 5 * (1 << 8) * (1 << 6)) / 19200000 = 4266666.6666...

So here is where the things go wrong. Bjorn helped me figure this out today
(off-list). Basically, the pwm framework will allow values up to 5000000,
as specified in the DT, but for then pwm value will go over 255
when computing the actual pwm value by the following formula:

                            duty * refclk
pwm_value = ----------------------------------------------
                NSEC_PER_SEC * prediv * (1 << prediv_exp)


So here is how the value 300 is reached (I messed up this next formula in
the commit message):

(5000000 * 19200000) / (1000000000 * 5 * (1 << 8)) = 300

But if we were to use the best_period determined:

(4266666 * 19200000) / (1000000000 * 5 * (1 << 8)) = 255

So I guess the process of determining the best parameters is correct.
What I think is missing is we need to divide the requested period (5000000)
to the resolution (255) and make sure the duty cycle is a multiple of the
result.

Something like this:

step = period / (1 << resolution)

So: 

5000000 / ((1 << 8) - 1) = 19607

and then:

pwm_value = duty / step;

Hope this makes sense.

Will try this out and respin the patch.

> 
> I'm asking because the max value is only used for capping, so with
> this patch the maximum brightness will be reached at around 80% duty
> cycle (i.e. when the wrap over happens without this patch).
> 
> Locally I'm currently remapping the duty cycle range to the PWM
> value range, which means the display brightness increases
> step-by-step until reaching 100% "duty cycle":
> 
> 		val = (duty * 255) / chan->period;
> 		chan->pwm_value = min(val, 255);
> 
> But for the backlight control the absolute numbers do not really
> matter and I have zero knowledge about the chip. So it might be
> that the controller really can only go up to ~80% duty cycle at
> these settings?
> 
> Greetings,
> 
> -- Sebastian
> 
> >  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;
> >  		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@linaro.org>
> >