Re: [PATCH v3 10/10] leds: pca995x: Add support for group brightness control

[Nora Schiffer]

On Tue, 2026-09-29 at 11:13 +0100, Lee Jones wrote:
> --- checkpatch.pl: clean (0 issues) ---
> 
> On Tue, 22 Sep 2026, Nora Schiffer wrote:
> 
> > When LEDs are set to PWM mode with group control enabled, their
> > brightness can control using a global "group PWM" setting, modulating
> > the individual LEDs' brightness with a second PWM running at a different
> > frequency. This lowers the minimum duty cycle from 1/256 to 1/65536
> > (averaged over the modulated signal). Group brightness control is
> > particularly useful to adjust for different levels of ambient light.
> > 
> > For simplicity, group PWM mode is always enabled, with the reset default
> > of 255 as group brightness. This reduces the effective duty cycle by
> > 1/256 at all individual brightness levels (the individual PWM signals
> > are modulated with the 255/256 duty cycle group PWM), which should be
> > imperceptible.
> > 
> > Signed-off-by: Nora Schiffer <nora.schiffer@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/leds/leds-pca995x.c | 46 ++++++++++++++++++++++++++++++++++---
> >  1 file changed, 43 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> > index 61d2581b976ce..d6438b576b186 100644
> > --- a/drivers/leds/leds-pca995x.c
> > +++ b/drivers/leds/leds-pca995x.c
> > @@ -35,6 +35,7 @@
> >  #define PCA995X_LED_OFF			0x0
> >  #define PCA995X_LED_ON			0x1
> >  #define PCA995X_LED_PWM_MODE		0x2
> > +#define PCA995X_LED_PWM_MODE_GRP	0x3
> >  #define PCA995X_LDRX_MASK		0x3
> >  #define PCA995X_LDRX_BITS		2
> > 
> > @@ -52,6 +53,7 @@
> >  struct pca995x_chipdef {
> >  	unsigned int num_leds;
> >  	u8 pwm_base;
> > +	u8 grppwm;
> >  	u8 irefall;
> >  	u8 eflag_base;
> >  };
> > @@ -59,6 +61,7 @@ struct pca995x_chipdef {
> >  static const struct pca995x_chipdef pca9952_chipdef = {
> >  	.num_leds	= 16,
> >  	.pwm_base	= 0x0a,
> > +	.grppwm		= 0x08,
> >  	.irefall	= 0x43,
> >  	.eflag_base	= 0x44,
> >  };
> > @@ -66,6 +69,7 @@ static const struct pca995x_chipdef pca9952_chipdef = {
> >  static const struct pca995x_chipdef pca9955b_chipdef = {
> >  	.num_leds	= 16,
> >  	.pwm_base	= 0x08,
> > +	.grppwm		= 0x06,
> >  	.irefall	= 0x45,
> >  	.eflag_base	= 0x46,
> >  };
> > @@ -73,6 +77,7 @@ static const struct pca995x_chipdef pca9955b_chipdef = {
> >  static const struct pca995x_chipdef pca9956b_chipdef = {
> >  	.num_leds	= 24,
> >  	.pwm_base	= 0x0a,
> > +	.grppwm		= 0x08,
> >  	.irefall	= 0x40,
> >  	.eflag_base	= 0x41,
> >  };
> > @@ -114,11 +119,10 @@ static int pca995x_brightness_set(struct led_classdev *led_cdev,
> > 
> >  	/*
> >  	 * Change LDRx configuration to individual brightness via PWM.
> > -	 * LED will stop blinking if it's doing so.
> >  	 */
> >  	return regmap_update_bits(chip->regmap, ledout_addr,
> >  				  PCA995X_LDRX_MASK << shift,
> > -				  PCA995X_LED_PWM_MODE << shift);
> > +				  PCA995X_LED_PWM_MODE_GRP << shift);
> 
> If we use group PWM mode to scale brightness, what happens when 'brightness' is
> 'LED_FULL'?  The 'switch' statement above sets 'PCA995X_LED_ON', which typically
> bypasses PWM entirely.  Won't this cause an LED set to 255 to ignore the group
> brightness and be fully on, while an LED at 254 is scaled down?

Patch 9/10 removes the use of PCA995X_LED_ON to avoid this issue, using
PCA995X_LED_PWM_MODE_GRP for all brightness values 1..255 instead.


> 
> >  }
> > 
> >  static ssize_t status_show(struct device *dev, struct device_attribute *attr, char *buf)
> > @@ -190,10 +194,41 @@ static ssize_t has_errors_store(struct device *dev, struct device_attribute *att
> >  	return ret ?: count;
> >  }
> > 
> > +static ssize_t group_brightness_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));
> 
> 'dev_get_drvdata(dev)' again?
> 
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(chip->regmap, chip->chipdef->grppwm, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sysfs_emit(buf, "%u\n", val);
> > +}
> > +
> > +static ssize_t group_brightness_store(struct device *dev, struct device_attribute *attr,
> > +				       const char *buf, size_t count)
> > +{
> > +	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));
> > +	u8 val;
> > +	int ret;
> > +
> > +	ret = kstrtou8(buf, 0, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(chip->regmap, chip->chipdef->grppwm, val);
> > +
> > +	return ret ?: count;
> > +}
> > +
> >  static DEVICE_ATTR_RW(has_errors);
> > +static DEVICE_ATTR_RW(group_brightness);
> > 
> >  static struct attribute *pca995x_attrs[] = {
> >  	&dev_attr_has_errors.attr,
> > +	&dev_attr_group_brightness.attr,
> >  	NULL,
> >  };
> >  ATTRIBUTE_GROUPS(pca995x);
> > @@ -270,11 +305,16 @@ static int pca995x_probe(struct i2c_client *client)
> >  	if (ret)
> >  		goto err_put_nodes;
> > 
> > -	/* Clear errors on probe */
> > +	/* Clear errors on probe, use GRPPWM register for group brightness control */
> >  	ret = regmap_write(chip->regmap, PCA995X_MODE2, PCA995X_MODE2_CFG);
> >  	if (ret)
> >  		goto err_put_nodes;
> > 
> > +	/* Full group brightness */
> > +	ret = regmap_write(chip->regmap, chipdef->grppwm, U8_MAX);
> > +	if (ret)
> > +		goto err_put_nodes;
> > +
> >  	/* IREF Output current value for all LEDn outputs */
> >  	ret = regmap_write(chip->regmap, chipdef->irefall, iref);
> >  	if (ret)
> > -- 
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > https://www.tq-group.com/
> > 
> > 
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/