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

[Fenglin Wu]

On 6/8/2026 4:24 AM, Dmitry Baryshkov wrote:
> On Fri, Jun 05, 2026 at 01:18:24AM -0700, 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>
> > ---
> >   drivers/leds/rgb/leds-qcom-lpg.c | 174 +++++++++++++++++++++++++++++++--------
> >   1 file changed, 141 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > index d7d6518de30f..ca84da563e09 100644
> > --- a/drivers/leds/rgb/leds-qcom-lpg.c
> > +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > @@ -148,6 +148,24 @@ struct lpg_channel {
> >   	unsigned int pattern_hi_idx;
> >   };
> >   
> > +/**
> > + * struct lpg_pattern - The LPG pattern normalized from the LED pattern
> > + * @data:       The pattern data array (caller must kfree)
> > + * @len:	number of entries to write to the LUT
> > + * @delta_t:    common step duration in ms
> > + * @lo_pause:   low-pause duration in ms
> > + * @hi_pause:   high-pause duration in ms
> > + * @ping_pong:  true if the pattern support reverse
> > + */
> > +struct lpg_pattern {
> > +	struct led_pattern *data;
> This looks like an overkill. Can you embed the struct here instead of
> embedding a pointer?

Are you suggesting only embedding an array of "struct led_pattern" here?

The patter data array consists of a variable number of "led_pattern" 
tuples that set by the HW pattern trigger, and a "led_pattern" tuple 
consists a "brightness" and "delta_t" pair that represents a single data 
point. A pointer is needed here as the memory for the pattern needs to 
be allocated dynamically according to the pattern length, hence it needs 
to be freed after the pattern is used.

> > +	unsigned int len;
> > +	unsigned int delta_t;
> > +	unsigned int lo_pause;
> > +	unsigned int hi_pause;
> > +	bool ping_pong;
> > +};
> > +
> >   /**
> >    * struct lpg_led - logical LED object
> >    * @lpg:		lpg context reference