Re: [PATCH v6 3/4] leds: pca963x: add multicolor LED class support

On Thu, Jul 23, 2026 at 4:19 PM Lee Jones <[email protected]> wrote:
>
> On Wed, 15 Jul 2026, Loic Poulain wrote:
>
> > Allow grouping of individual PCA963x PWM channels into a single
> > multicolor LED device by adding support for the LED multicolor class.
> >
> > A child node with sub-children is treated as a multicolor group,
> > others are treated as single leds, keeping full backwards compatibility.
> >
> > Signed-off-by: Loic Poulain <[email protected]>
> > ---
> >  drivers/leds/Kconfig        |   1 +
> >  drivers/leds/leds-pca963x.c | 162 ++++++++++++++++++++++++++++++++++----------
> >  2 files changed, 128 insertions(+), 35 deletions(-)
> >
> > diff --git dur/leds/Kconfig b/drivers/leds/Kconfig
> > index f4a0a3c8c8705e0f10ba26584277dbb2d5eac5b5..14df88f92b12bbe43908b67f9480cf23056e27e2 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -596,6 +596,7 @@ config LEDS_PCA963X
> >       tristate "LED support for PCA963x I2C chip"
> >       depends on LEDS_CLASS
> >       depends on I2C
> > +     select LEDS_CLASS_MULTICOLOR
> >       help
> >         This option enables support for LEDs connected to the PCA963x
> >         LED driver chip accessed via the I2C bus. Supported
> > diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
> > index e3a81c60ee27c96e5050a829523dfd43e1f0663f..f3e4d65e48b4c3eefa147a7fb5c9fe81ce569731 100644
> > --- a/drivers/leds/leds-pca963x.c
> > +++ b/drivers/leds/leds-pca963x.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/string.h>
> >  #include <linux/ctype.h>
> >  #include <linux/leds.h>
> > +#include <linux/led-class-multicolor.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> >  #include <linux/property.h>
> > @@ -101,8 +102,11 @@ struct pca963x;
> >  struct pca963x_led {
> >       struct pca963x *chip;
> >       struct led_classdev led_cdev;
> > +     struct led_classdev_mc mc_cdev;
> > +     struct mc_subled subleds[4];
> >       int led_num; /* 0 .. 15 potentially */
> >       bool blinking;
> > +     bool is_mc;
> >       u8 gdc;
> >       u8 gfrq;
> >  };
> > @@ -199,20 +203,24 @@ static void pca963x_blink(struct pca963x_led *led)
> >       led->blinking = true;
> >  }
> >
> > -static int pca963x_power_state(struct pca963x_led *led)
> > +static void pca963x_track_power_state(struct pca963x_led *led, unsigned int led_num,
> > +                                   enum led_brightness brightness)
> >  {
> > -     struct i2c_client *client = led->chip->client;
> >       unsigned long *leds_on = &led->chip->leds_on;
> > -     unsigned long cached_leds = *leds_on;
> >
> > -     if (led->led_cdev.brightness)
> > -             set_bit(led->led_num, leds_on);
> > +     if (brightness)
> > +             set_bit(led_num, leds_on);
> >       else
> > -             clear_bit(led->led_num, leds_on);
> > +             clear_bit(led_num, leds_on);
> > +}
> >
> > -     if (!(*leds_on) != !cached_leds)
> > +static int pca963x_sync_power_state(struct pca963x_led *led, unsigned long cached_leds)
> > +{
> > +     struct i2c_client *client = led->chip->client;
> > +
> > +     if (!led->chip->leds_on != !cached_leds)
> >               return i2c_smbus_write_byte_data(client, PCA963X_MODE1,
> > -                                              *leds_on ? 0 : BIT(4));
> > +                                              led->chip->leds_on ? 0 : BIT(4));
> >
> >       return 0;
> >  }
> > @@ -221,22 +229,54 @@ static int pca963x_led_set(struct led_classdev *led_cdev,
> >                          enum led_brightness value)
> >  {
> >       struct pca963x_led *led;
> > +     unsigned long cached_leds;
> >       int ret;
> >
> >       led = container_of(led_cdev, struct pca963x_led, led_cdev);
> >
> >       mutex_lock(&led->chip->mutex);
> >
> > +     cached_leds = led->chip->leds_on;
> >       ret = pca963x_brightness(led, value);
> >       if (ret < 0)
>
> Should we check 'if (ret)' here instead of 'if (ret < 0)'? It is generally
> preferred to only check for negative values if a positive return value has
> specific meaning that needs handling.

That code was already in place, but I can update it to align with the
other usages.

>
> >               goto unlock;
> > -     ret = pca963x_power_state(led);
> > +
> > +     pca963x_track_power_state(led, led->led_num, value);
> > +     ret = pca963x_sync_power_state(led, cached_leds);
> >
> >  unlock:
> >       mutex_unlock(&led->chip->mutex);
> >       return ret;
> >  }
> >
> > +static int pca963x_led_mc_set(struct led_classdev *led_cdev,
> > +                           enum led_brightness value)
> > +{
> > +     struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(led_cdev);
> > +     struct pca963x_led *led = container_of(mc_cdev, struct pca963x_led, mc_cdev);
> > +     unsigned long cached_leds;
> > +     int ret = 0, sync_ret;
> > +
> > +     led_mc_calc_color_components(mc_cdev, value);
> > +
> > +     guard(mutex)(&led->chip->mutex);
> > +
> > +     cached_leds = led->chip->leds_on;
> > +     for (unsigned int i = 0; i < mc_cdev->num_colors; i++) {
> > +             led->led_num = mc_cdev->subled_info[i].channel;
>
> Why does this get set twice?

Right, the first assignment is useless and has been forgotten during
the latest rework.

>
> Question from AI:
>
>   Would it be better to re-factor 'pca963x_brightness()' to accept the channel
>   number as an explicit parameter? Temporarily overwriting 'led->led_num' in the
>   shared structure feels a bit fragile.

That's not a bad idea tbh, so I will do this for v7.

>
> > +             ret = pca963x_brightness(led, mc_cdev->subled_info[i].brightness);
> > +             if (ret)
> > +                     break;
>
> Deserves a comment.  Why are we syncing power state on failure?

Some channels may already have been updated before the error, so we
still sync the global on/off state to reflect what actually changed.
I will add a comment.

>
> > +             pca963x_track_power_state(led, mc_cdev->subled_info[i].channel,
> > +                                       mc_cdev->subled_info[i].brightness);
> > +     }
> > +
> > +     sync_ret = pca963x_sync_power_state(led, cached_leds);
> > +
> > +     return ret ? : sync_ret;
> > +}
> > +
> >  static unsigned int pca963x_period_scale(struct pca963x_led *led,
> >                                        unsigned int val)
> >  {
> > @@ -300,6 +340,77 @@ static int pca963x_blink_set(struct led_classdev *led_cdev,
> >       return 0;
> >  }
> >
> > +static int pca963x_parse_mc_subleds(struct device *dev, struct pca963x_led *led,
> > +                                 struct fwnode_handle *fwnode,
> > +                                 const struct pca963x_chipdef *chipdef)
> > +{
> > +     unsigned int num_colors = 0;
> > +     int ret;
> > +
> > +     fwnode_for_each_child_node_scoped(fwnode, sub) {
> > +             u32 color, subreg;
> > +
> > +             if (num_colors >= ARRAY_SIZE(led->subleds))
> > +                     return dev_err_probe(dev, -EINVAL, "Too many LEDs for node %pfw\n", fwnode);
> > +
> > +             ret = fwnode_property_read_u32(sub, "reg", &subreg);
> > +             if (ret || subreg >= chipdef->n_leds)
> > +                     return dev_err_probe(dev, -EINVAL, "Invalid 'reg' for sub-LED %pfw\n", sub);
>
> Why are you masking the real error?

Because I try to handle two errors at a time, I will add proper
reporting for each in v7.

>
> > +             ret = fwnode_property_read_u32(sub, "color", &color);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "Missing 'color' for sub-LED %pfw\n", sub);
> > +
> > +             led->subleds[num_colors].channel = subreg;
> > +             led->subleds[num_colors].color_index = color;
> > +             led->subleds[num_colors].intensity = LED_FULL;
> > +             num_colors++;
> > +     }
> > +
> > +     led->mc_cdev.subled_info = led->subleds;
> > +     led->mc_cdev.num_colors = num_colors;
> > +     led->mc_cdev.led_cdev.max_brightness = LED_FULL;
> > +     led->mc_cdev.led_cdev.brightness_set_blocking = pca963x_led_mc_set;
> > +
> > +     return 0;
> > +}
> > +
> > +static int pca963x_register_led(struct device *dev, struct pca963x_led *led,
> > +                             u32 reg, struct fwnode_handle *fwnode,
> > +                             const struct pca963x_chipdef *chipdef,
> > +                             bool hw_blink)
> > +{
> > +     struct i2c_client *client = led->chip->client;
> > +     struct led_init_data init_data = {};
> > +     char label[32];
> > +     int ret;
> > +
> > +     led->led_num = reg;
> > +     led->is_mc = fwnode_get_child_node_count(fwnode) > 0;
>
> Deservers a comment.

Ack.

>
> > +     if (led->is_mc) {
> > +             ret = pca963x_parse_mc_subleds(dev, led, fwnode, chipdef);
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             led->led_cdev.brightness_set_blocking = pca963x_led_set;
> > +             if (hw_blink)
> > +                     led->led_cdev.blink_set = pca963x_blink_set;
> > +     }
> > +
> > +     init_data.fwnode = fwnode;
> > +     /* for backwards compatibility */
>
> Because ...
>
> Which part?
>
> Sentences start with an upper-case char.

This is a direct copy of the existing chunk in pca963x_register_leds
(subsequently removed below). That said, I'm happy to improve it.

>
> > +     init_data.devicename = "pca963x";
> > +     snprintf(label, sizeof(label), "%d:%.2x:%u", client->adapter->nr, client->addr, reg);
> > +     init_data.default_label = label;
> > +
> > +     if (led->is_mc)
> > +             return devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev,
> > +                                                              &init_data);
> > +
> > +     return devm_led_classdev_register_ext(dev, &led->led_cdev, &init_data);
> > +}
> > +
> >  static int pca963x_register_leds(struct i2c_client *client,
> >                                struct pca963x *chip)
> >  {
> > @@ -338,37 +449,18 @@ static int pca963x_register_leds(struct i2c_client *client,
> >               return ret;
> >
> >       device_for_each_child_node_scoped(dev, child) {
> > -             struct led_init_data init_data = {};
> > -             char default_label[32];
> > -
> >               ret = fwnode_property_read_u32(child, "reg", &reg);
> > -             if (ret || reg >= chipdef->n_leds) {
> > -                     dev_err(dev, "Invalid 'reg' property for node %pfw\n",
> > -                             child);
> > -                     return -EINVAL;
> > -             }
> > +             if (ret || reg >= chipdef->n_leds)
> > +                     return dev_err_probe(dev, -EINVAL,
> > +                                          "Invalid 'reg' property for node %pfw\n", child);
>
> We should be propagating the real error instead of investing our own.

Yes, same as above, will do.

Thanks for your review.

>
> > -             led->led_num = reg;
> >               led->chip = chip;
> > -             led->led_cdev.brightness_set_blocking = pca963x_led_set;
> > -             if (hw_blink)
> > -                     led->led_cdev.blink_set = pca963x_blink_set;
> >               led->blinking = false;
> >
> > -             init_data.fwnode = child;
> > -             /* for backwards compatibility */
> > -             init_data.devicename = "pca963x";
> > -             snprintf(default_label, sizeof(default_label), "%d:%.2x:%u",
> > -                      client->adapter->nr, client->addr, reg);
> > -             init_data.default_label = default_label;
> > -
> > -             ret = devm_led_classdev_register_ext(dev, &led->led_cdev,
> > -                                                  &init_data);
> > -             if (ret) {
> > -                     dev_err(dev, "Failed to register LED for node %pfw\n",
> > -                             child);
> > -                     return ret;
> > -             }
> > +             ret = pca963x_register_led(dev, led, reg, child, chipdef, hw_blink);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "Failed to register LED for node %pfw\n",
> > +                                          child);
> >
> >               ++led;
> >       }
> >
> > --
> > 2.34.1
> >
> >
>
> --
> Lee Jones