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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver permanently leaks a hardware LUT slot and fails to trigger patterns when a 2-step palindrome pattern evaluates to an `actual_len` of 1.
--

--- Patch [1]: [PATCH v6] leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode ---
commit c5b003f7bc9326bf3784a70ad883c39b215ae496
Author: Fenglin Wu <[email protected]>
Subject: leds: rgb: leds-qcom-lpg: Fix LED color balancing in HW pattern mode

This commit splits out pattern preparation into lpg_prepare_pattern() and
adds support for per-channel scaling so the hardware pattern respects
multi-color LED balances. It also updates the LUT storage paths to handle
the dynamically allocated scaled patterns.

> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index d7d6518de30f2..24b1f570f5244 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c

[ ... ]

> @@ -1087,43 +1098,64 @@ static int lpg_pattern_set(struct lpg_led *led, struct led_pattern *led_pattern,
>  	 * duration of all steps.
>  	 */
>  	if (lpg->lut_base || lpg->lut_sdam) {
> -		lo_pause = pattern[0].delta_t;
> -		hi_pause = pattern[actual_len - 1].delta_t;
> +		prep->lo_pause = pattern[0].delta_t;
> +		prep->hi_pause = pattern[actual_len - 1].delta_t;
>  	} else {
>  		if (delta_t != pattern[0].delta_t || delta_t != pattern[actual_len - 1].delta_t)
> -			goto out_free_pattern;
> +			return -EINVAL;
> +		prep->lo_pause = 0;
> +		prep->hi_pause = 0;
>  	}
>  
> +	prep->data = no_free_ptr(pattern);
> +	prep->len = actual_len;

[Severity: High]
This is a pre-existing issue, but does this logic permanently leak a
hardware LUT slot if a 2-step palindrome pattern is provided?

If a user-space process provides a 2-step pattern where both steps have the
same brightness, the ping_pong logic earlier in lpg_prepare_pattern() will
calculate actual_len = (2 + 1) / 2, which evaluates to 1.

When this length-1 pattern is subsequently stored, it allocates 1 slot where
lo_idx and hi_idx are equal. Because they are equal, lpg_brightness_set()
treats the configuration as a static brightness instead of playing the
pattern.

Later, when the pattern is cleared and lpg_lut_free() is called with this
lo_idx and hi_idx, it hits this early return:

static void lpg_lut_free(struct lpg *lpg, unsigned int lo_idx, unsigned int hi_idx)
{
	int len;

	len = hi_idx - lo_idx + 1;
	if (len == 1)
		return;

	bitmap_clear(lpg->lut_bitmap, lo_idx, len);
}

Does this early return bypass bitmap_clear() and permanently leak the LUT
slot for the remainder of the device lifecycle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-lpg-rgb-color-balance-fix-v6-1-b49d51528f61@oss.qualcomm.com?part=1