Re: [PATCH RFC] leds: rgb: leds-qcom-lpg: Compute PWM value based on period instead

[Sebastian Reichel]

Hi,

On Mon, Mar 03, 2025 at 06:14:36PM +0200, Abel Vesa wrote:
> Currently, the implementation computes the best matched period based
> on the requested one, by looping through all possible register
> configurations. The best matched period is below the requested period.
> This means the PWM consumer could request duty cycle values between
> the best matched period and the requested period, which with the current
> implementation for computing the PWM value, it will result in values out
> of range with respect to the selected resolution.
> 
> So change the way the PWM value is determined as a ratio between the
> requested period and duty cycle, mapped on the resolution interval.
> Do this in contrast to using the register configuration selected when
> the best matching period was determined.
> 
> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> ---
> This patch is based on the following patchset:
> https://lore.kernel.org/all/20250303-leds-qcom-lpg-fix-max-pwm-on-hi-res-v3-0-62703c0ab76a@xxxxxxxxxx/
> ---

Tested-by: Sebastian Reichel <sre@xxxxxxxxxx>

Greetings,

-- Sebastian

>  drivers/leds/rgb/leds-qcom-lpg.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 4f2a178e3d265a2cc88e651d3e2ca6ae3dfac2e2..80130c205dce7c6ae1c356b7a7c5f6460a2b0bb0 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -523,8 +523,10 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period)
>  	return 0;
>  }
>  
> -static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
> +static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty, uint64_t period)
>  {
> +	const unsigned int *pwm_resolution_arr;
> +	unsigned int step;
>  	unsigned int max;
>  	unsigned int val;
>  	unsigned int clk_rate;
> @@ -532,13 +534,15 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
>  	if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
>  		max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
>  		clk_rate = lpg_clk_rates_hi_res[chan->clk_sel];
> +		pwm_resolution_arr = lpg_pwm_resolution_hi_res;
>  	} else {
>  		max = BIT(lpg_pwm_resolution[chan->pwm_resolution_sel]) - 1;
>  		clk_rate = lpg_clk_rates[chan->clk_sel];
> +		pwm_resolution_arr = lpg_pwm_resolution;
>  	}
>  
> -	val = div64_u64(duty * clk_rate,
> -			(u64)NSEC_PER_SEC * lpg_pre_divs[chan->pre_div_sel] * (1 << chan->pre_div_exp));
> +	step = div64_u64(period, max);
> +	val = div64_u64(duty, step);
>  
>  	chan->pwm_value = min(val, max);
>  }
> @@ -829,7 +833,7 @@ static void lpg_brightness_set(struct lpg_led *led, struct led_classdev *cdev,
>  			lpg_calc_freq(chan, NSEC_PER_MSEC);
>  
>  			duty = div_u64(brightness * chan->period, cdev->max_brightness);
> -			lpg_calc_duty(chan, duty);
> +			lpg_calc_duty(chan, duty, NSEC_PER_MSEC);
>  			chan->enabled = true;
>  			chan->ramp_enabled = false;
>  
> @@ -906,7 +910,7 @@ static int lpg_blink_set(struct lpg_led *led,
>  		chan = led->channels[i];
>  
>  		lpg_calc_freq(chan, period);
> -		lpg_calc_duty(chan, duty);
> +		lpg_calc_duty(chan, duty, period);
>  
>  		chan->enabled = true;
>  		chan->ramp_enabled = false;
> @@ -1241,7 +1245,7 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		if (ret < 0)
>  			goto out_unlock;
>  
> -		lpg_calc_duty(chan, state->duty_cycle);
> +		lpg_calc_duty(chan, state->duty_cycle, state->period);
>  	}
>  	chan->enabled = state->enabled;
>  
> 
> ---
> base-commit: 0067a4b21c9ab441bbe6bf3635b3ddd21f6ca7c3
> change-id: 20250303-leds-qcom-lpg-compute-pwm-value-using-period-0823ceb7542a
> 
> Best regards,
> -- 
> Abel Vesa <abel.vesa@xxxxxxxxxx>
>

Attachment: signature.asc
Description: PGP signature