Re: [PATCH v2 2/3] leds: pca963x: add multicolor LED class support

On Thu, Jul 9, 2026 at 1:35 PM Lee Jones <[email protected]> wrote:
>
> /* Sashiko Automation: Reviewed (0 Findings) */
>
> On Wed, 01 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 | 126 ++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 110 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/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..f6f6bafcc2bd5bad51a3184c4cb08fc50693a0a5 100644
> > --- 'drivers/leds/leds-pca963x.c'
> > +++ '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;
> >  };
> > @@ -205,7 +209,7 @@ static int pca963x_power_state(struct pca963x_led *led)
> >       unsigned long *leds_on = &led->chip->leds_on;
> >       unsigned long cached_leds = *leds_on;
> >
> > -     if (led->led_cdev.brightness)
> > +     if (led->is_mc ? led->mc_cdev.led_cdev.brightness : led->led_cdev.brightness)
> >               set_bit(led->led_num, leds_on);
> >       else
> >               clear_bit(led->led_num, leds_on);
> > @@ -237,6 +241,28 @@ static int pca963x_led_set(struct led_classdev *led_cdev,
> >       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 int i;
>
> Remove this.
>
> > +     int ret;
> > +
> > +     led_mc_calc_color_components(mc_cdev, value);
> > +
> > +     guard(mutex)(&led->chip->mutex);
> > +
> > +     for (i = 0; i < mc_cdev->num_colors; i++) {
>
> for (int i = 0; ...

ack.

>
> > +             led->led_num = mc_cdev->subled_info[i].channel;
> > +             ret = pca963x_brightness(led, mc_cdev->subled_info[i].brightness);
> > +             if (ret < 0)
>
> Can (ret) work?

sure.

>
> > +                     return ret;
> > +     }
> > +
> > +     return pca963x_power_state(led);
> > +}
> > +
> >  static unsigned int pca963x_period_scale(struct pca963x_led *led,
> >                                        unsigned int val)
> >  {
> > @@ -300,6 +326,84 @@ static int pca963x_blink_set(struct led_classdev *led_cdev,
> >       return 0;
> >  }
> >
> > +static int pca963x_register_single_led(struct device *dev,
> > +                                     struct pca963x_led *led, u32 reg,
> > +                                     struct fwnode_handle *fwnode,
> > +                                     bool hw_blink)
> > +{
> > +     struct led_init_data init_data = {};
> > +     char default_label[32];
>
> How sure are you that this cannot overflow?

This is unchanged from the existing driver, and snprintf() is bounded
by sizeof(default_label), so it can only truncate, never overflow. In
practice it won't even truncate: nr is the I2C bus index, addr is a
7-bit I2C address (2 hex chars), and reg is a channel index limited to
0-15, so the label is always well within 32 bytes.

>
> > +     struct i2c_client *client = led->chip->client;
> > +
> > +     led->led_num = reg;
>
> There's quite a bit of duplication here.  How about calling a common
> function an only do the genuinely different things in these two new
> functions?

Yes, make sense I will do that in v5.

>
> > +     led->is_mc = false;
> > +     led->led_cdev.brightness_set_blocking = pca963x_led_set;
> > +     if (hw_blink)
> > +             led->led_cdev.blink_set = pca963x_blink_set;
> > +
> > +     init_data.fwnode = fwnode;
> > +     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;
> > +
> > +     return devm_led_classdev_register_ext(dev, &led->led_cdev, &init_data);
> > +}
> > +
> > +static int pca963x_register_mc_led(struct device *dev,
> > +                                struct pca963x_led *led, u32 reg,
> > +                                struct fwnode_handle *fwnode,
> > +                                const struct pca963x_chipdef *chipdef)
> > +{
> > +     struct mc_subled *subleds = led->subleds;
> > +     unsigned int num_colors = 0;
> > +     struct led_init_data init_data = {};
> > +     char default_label[32];
> > +     struct i2c_client *client = led->chip->client;
> > +     int ret;
> > +
> > +     fwnode_for_each_child_node_scoped(fwnode, sub) {
> > +             u32 color, subreg;
> > +
> > +             if (num_colors >= ARRAY_SIZE(led->subleds)) {
> > +                     dev_err(dev, "Too many sub-LEDs for node %pfw\n", fwnode);
>
> dev_err_probe()
>
> > +                     return -EINVAL;
> > +             }
> > +
> > +             ret = fwnode_property_read_u32(sub, "reg", &subreg);
> > +             if (ret || subreg >= chipdef->n_leds) {
> > +                     dev_err(dev, "Invalid 'reg' for sub-LED %pfw\n", sub);
> > +                     return -EINVAL;
> > +             }
> > +
> > +             ret = fwnode_property_read_u32(sub, "color", &color);
> > +             if (ret) {
> > +                     dev_err(dev, "Missing 'color' for sub-LED %pfw\n", sub);
> > +                     return ret;
> > +             }
> > +
> > +             subleds[num_colors].channel = subreg;
> > +             subleds[num_colors].color_index = color;
> > +             subleds[num_colors].intensity = LED_FULL;
> > +             num_colors++;
> > +     }
> > +
> > +     led->led_num = reg;
> > +     led->is_mc = true;
> > +     led->mc_cdev.subled_info = 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;
> > +
> > +     init_data.fwnode = fwnode;
> > +     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;
> > +
> > +     return devm_led_classdev_multicolor_register_ext(dev, &led->mc_cdev, &init_data);
> > +}
> > +
> >  static int pca963x_register_leds(struct i2c_client *client,
> >                                struct pca963x *chip)
> >  {
> > @@ -338,9 +442,6 @@ 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",
> > @@ -348,22 +449,13 @@ static int pca963x_register_leds(struct i2c_client *client,
> >                       return -EINVAL;
> >               }
> >
> > -             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 (fwnode_get_child_node_count(child) > 0)
> > +                     ret = pca963x_register_mc_led(dev, led, reg, child, chipdef);
> > +             else
> > +                     ret = pca963x_register_single_led(dev, led, reg, child, hw_blink);
> >               if (ret) {
> >                       dev_err(dev, "Failed to register LED for node %pfw\n",
> >                               child);
>
> dev_err_probe()

Ok,

Will submit a new version.

Thanks,
Loic



>
> --
> Lee Jones