Re: [PATCH v6] leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode

[Fenglin Wu]

On 9/3/2026 7:27 PM, Konrad Dybcio wrote:
> On 7/17/26 6:41 AM, Fenglin Wu wrote:
>> Currently, when the LED is configured as a RGB LED or a multi-color
>> LED device, the same pattern is programmed for all LED channels
>> regardless of the sub-led intensities when triggered by HW pattern.
>> It results that the LED device is always working in a white-balanced
>> mode regardless of the intensity settings.
>>
>> To fix this, scale the pattern data according to the sub-led intensity
>> and program the HW pattern separately for each LPG channel.
>>
>> Fixes: 24e2d05d1b68 ("leds: Add driver for Qualcomm LPG")
>> Fixes: 6ab1f766a80a ("leds: rgb: leds-qcom-lpg: Add support for PPG through single SDAM")
>> Fixes: 5e9ff626861a ("leds: rgb: leds-qcom-lpg: Include support for PPG with dedicated LUT SDAM")
>> Assisted-by: Claude:claude-4-6-sonnet
>> Signed-off-by: Fenglin Wu <fenglin.wu@xxxxxxxxxxxxxxxx>
>> ---
> 
> GPT came up with the following fixes/suggestions:
> 
> 1] This one makes sense at a glance:
> 
>     leds: rgb: leds-qcom-lpg: Skip LUT allocation for off colors
>     
>     Multicolor hardware pattern setup programs a separate LUT range for every
>     component. A component with zero calculated brightness is disabled before
>     the pattern is applied, so its all-zero range is never used.
>     
>     Skip allocating it to preserve the shared LUT capacity.
>     
>     Fixes: 2882fa0dc1cf ("leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode")
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index 24b1f570f524..e32388a16537 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -1219,6 +1219,13 @@ static int lpg_pattern_mc_set(struct led_classdev *cdev,
>                 chan = led->channels[i];
>                 scale = mc->subled_info[i].brightness;
>  
> +               /* An off component neither needs nor uses a LUT range. */
> +               if (!scale) {
> +                       chan->pattern_lo_idx = 0;
> +                       chan->pattern_hi_idx = 0;
> +                       continue;
> +               }
> +

Thanks. I can add this one even though it is not related with function
correctness; it does help save the LUT with a simple check on the
sub-LED brightness. The sub-LED would just be turned off by gating the
current sink so the setting of pulse generator doesn't matter in such case.

>                 for (j = 0; j < pattern.len; j++) {
>                         scaled[j].brightness = DIV_ROUND_CLOSEST(
>                                 (u32)prep_data[j].brightness * scale, LED_FULL);
> 
> 
> 2] This one.. I'm not convinced..
> 
>     leds: rgb: leds-qcom-lpg: Reuse LUT patterns for equal colors
>     
>     Multicolor hardware patterns with equal nonzero component brightnesses
>     produce identical LUT data. Reuse their LUT range instead of allocating
>     and programming duplicate data.
>     
>     Free each shared range once when clearing a pattern or unwinding a failed
>     allocation.
> 
> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index e32388a16537..1c4d550c1ed0 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -1175,6 +1175,34 @@ static int lpg_pattern_single_set(struct led_classdev *cdev,
>  	return 0;
>  }
>  
> +static void lpg_pattern_free(struct lpg_led *led, unsigned int count)
> +{
> +	struct lpg_channel *chan;
> +	unsigned int i, j;
> +
> +	for (i = 0; i < count; i++) {
> +		chan = led->channels[i];
> +		if (chan->pattern_lo_idx == chan->pattern_hi_idx)
> +			continue;
> +
> +		for (j = 0; j < i; j++) {
> +			if (chan->pattern_lo_idx == led->channels[j]->pattern_lo_idx &&
> +			    chan->pattern_hi_idx == led->channels[j]->pattern_hi_idx)
> +				break;
> +		}
> +
> +		if (j == i)
> +			lpg_lut_free(chan->lpg, chan->pattern_lo_idx,
> +				     chan->pattern_hi_idx);
> +	}
> +
> +	for (i = 0; i < count; i++) {
> +		chan = led->channels[i];
> +		chan->pattern_lo_idx = 0;
> +		chan->pattern_hi_idx = 0;
> +	}
> +}
> +
>  static int lpg_pattern_mc_set(struct led_classdev *cdev,
>  			      struct led_pattern *led_pattern, u32 len,
>  			      int repeat)
> @@ -1226,6 +1254,17 @@ static int lpg_pattern_mc_set(struct led_classdev *cdev,
>  			continue;
>  		}
>  
> +		for (j = 0; j < i; j++) {
> +			if (scale == mc->subled_info[j].brightness) {
> +				chan->pattern_lo_idx = led->channels[j]->pattern_lo_idx;
> +				chan->pattern_hi_idx = led->channels[j]->pattern_hi_idx;
> +				break;
> +			}
> +		}
This is based on a wrong assumption that all sub-LEDs are using the same
pattern, so this is not correct.
> +
> +		if (j != i)
> +			continue;
> +
>  		for (j = 0; j < pattern.len; j++) {
>  			scaled[j].brightness = DIV_ROUND_CLOSEST(
>  				(u32)prep_data[j].brightness * scale, LED_FULL);
> @@ -1238,14 +1277,7 @@ static int lpg_pattern_mc_set(struct led_classdev *cdev,
>  			ret = lpg_lut_store_sdam(lpg, scaled, pattern.len, &lo_idx, &hi_idx);
>  
>  		if (ret < 0) {
> -			/* Free LUT slots already allocated for previous channels */
> -			while (i-- > 0) {
> -				chan = led->channels[i];
> -				lpg_lut_free(lpg, chan->pattern_lo_idx, chan->pattern_hi_idx);
> -				chan->pattern_lo_idx = 0;
> -				chan->pattern_hi_idx = 0;
> -			}
> -
> +			lpg_pattern_free(led, i);
>  			return ret;
>  		}
>  
> @@ -1271,13 +1303,12 @@ static int lpg_pattern_clear(struct lpg_led *led)
>  
>  	mutex_lock(&lpg->lock);
>  
> +	lpg_pattern_free(led, led->num_channels);
> +
>  	for (i = 0; i < led->num_channels; i++) {
>  		chan = led->channels[i];
> -		lpg_lut_free(lpg, chan->pattern_lo_idx, chan->pattern_hi_idx);
>  		lpg_sdam_configure_triggers(chan, 0);
>  		lpg_clear_pbs_trigger(chan->lpg, chan->lut_mask);
> -		chan->pattern_lo_idx = 0;
> -		chan->pattern_hi_idx = 0;
>  	}
>  
>  	mutex_unlock(&lpg->lock);