Dan, Some variable naming related nitpicking below. On 10/16/19 5:59 PM, Dan Murphy wrote: > Add multicolor framework support for the lp55xx family. > > Signed-off-by: Dan Murphy <dmurphy@xxxxxx> > --- > drivers/leds/Kconfig | 2 + > drivers/leds/leds-lp55xx-common.c | 175 +++++++++++++++++++--- > drivers/leds/leds-lp55xx-common.h | 9 ++ > include/linux/platform_data/leds-lp55xx.h | 7 + > 4 files changed, 170 insertions(+), 23 deletions(-) > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index fb614a6b9afa..a121a2855c06 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -377,8 +377,10 @@ config LEDS_LP50XX > config LEDS_LP55XX_COMMON > tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" > depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 > + depends on OF > select FW_LOADER > select FW_LOADER_USER_HELPER > + select LEDS_CLASS_MULTI_COLOR > help > This option supports common operations for LP5521/5523/55231/5562/8501 > devices. > diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c > index 824d1d73dde1..026ebc2f8e18 100644 > --- a/drivers/leds/leds-lp55xx-common.c > +++ b/drivers/leds/leds-lp55xx-common.c > @@ -131,14 +131,54 @@ static struct attribute *lp55xx_led_attrs[] = { > }; > ATTRIBUTE_GROUPS(lp55xx_led); > > +static int lp55xx_map_channel(struct lp55xx_led *led, int color_id, > + enum led_brightness brightness) > +{ > + int i; > + > + for (i = 0; i < led->mc_cdev.num_leds; i++) { > + if (led->color_component[i].color_id == color_id) { I'd use plural "color_components" for the property name. > + led->color_component[i].brightness = brightness; > + return 0; > + } > + } > + > + return -EINVAL; > +} > + > static int lp55xx_set_brightness(struct led_classdev *cdev, > enum led_brightness brightness) > { > + struct led_mc_color_conversion color_component[LP55XX_MAX_GROUPED_CHAN]; > struct lp55xx_led *led = cdev_to_lp55xx_led(cdev); > struct lp55xx_device_config *cfg = led->chip->cfg; > + int ret; > + int i; > > - led->brightness = (u8)brightness; > - return cfg->brightness_fn(led); > + if (led->mc_cdev.num_leds > 1) { > + if (!cfg->multicolor_brightness_fn) > + return -EINVAL; > + > + led_mc_calc_color_components(&led->mc_cdev, brightness, > + color_component); Similarly here - you calculate all components so it is weird to pass variable of singular color_component form. > + > + for (i = 0; i < led->mc_cdev.num_leds; i++) { > + ret = lp55xx_map_channel(led, > + color_component[i].color_id, > + color_component[i].brightness); > + if (ret) > + return ret; > + } > + > + ret = cfg->multicolor_brightness_fn(led); > + if (ret) > + return ret; > + } else { > + led->brightness = (u8)brightness; > + ret = cfg->brightness_fn(led); > + } > + > + return ret; [...] -- Best regards, Jacek Anaszewski