| Message ID | 20191001145620.11123-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | Multicolor Framework | expand |
Hello On 10/1/19 9:56 AM, Dan Murphy wrote: > Hello > > I have updated the code per v9 review comments > https://lore.kernel.org/patchwork/project/lkml/list/?series=411824 I have a v11 version ready to go would like to include any v10 comments So v11 changes include - Adding inline MC framework functions for devices that do not have a direct dependency on the MC framework (ie LP55xx) - Update and fixed the devm_* MC framework functions (fixed the struct in match and added the devm_*_ext functions) - Removed the hard dependency on the MC framework in the Kconfig for the LP55xx_common code. Dan > Some notable changes: > > MC framework 4/16 > - Added a color structure for device drivers to use to associate color IDs with > brightness values > - Name of structure may need some work but this is for proof of concept. > - Added back in the devm_* APIs > - Inlined function led_classdev_multicolor_register > > LP55xx > - Fixed binding doc to add "@" to nodes [8/16] > - Fixed associated DT file to add the "@" to each node [9,10,11/16] > - Separate the LP5523 code changes from the LP55xx common code change > - Added channel to color ID mapping for LP55xx [12/16] > > LP5523 > - Separated out this code from LP55xx > > LP5521 > - New patch adding multicolor framework support for this device > > Dan > > Dan Murphy (16): > dt: bindings: Add multicolor class dt bindings documention > dt-bindings: leds: Add multicolor ID to the color ID list > leds: Add multicolor ID to the color ID list > leds: multicolor: Introduce a multicolor class definition > dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers > leds: lp50xx: Add the LP50XX family of the RGB LED driver > dt: bindings: lp55xx: Be consistent in the document with LED acronym > dt: bindings: lp55xx: Update binding for Multicolor Framework > ARM: dts: n900: Add reg property to the LP5523 channel node > ARM: dts: imx6dl-yapp4: Add reg property to the lp5562 channel node > ARM: dts: ste-href: Add reg property to the LP5521 channel nodes > leds: lp55xx: Add multicolor framework support to lp55xx > leds: lp5523: Update the lp5523 code to add intensity function > leds: lp5521: Add multicolor framework intensity support > leds: lp55xx: Fix checkpatch file permissions issues > leds: lp5523: Fix checkpatch issues in the code > > .../ABI/testing/sysfs-class-led-multicolor | 35 + > .../bindings/leds/leds-class-multicolor.txt | 98 +++ > .../devicetree/bindings/leds/leds-lp50xx.txt | 148 ++++ > .../devicetree/bindings/leds/leds-lp55xx.txt | 155 +++- > Documentation/leds/index.rst | 1 + > Documentation/leds/leds-class-multicolor.rst | 96 +++ > arch/arm/boot/dts/imx6dl-yapp4-common.dtsi | 14 +- > arch/arm/boot/dts/omap3-n900.dts | 29 +- > arch/arm/boot/dts/ste-href.dtsi | 22 +- > drivers/leds/Kconfig | 22 + > drivers/leds/Makefile | 2 + > drivers/leds/led-class-multicolor.c | 268 ++++++ > drivers/leds/led-core.c | 1 + > drivers/leds/leds-lp50xx.c | 784 ++++++++++++++++++ > drivers/leds/leds-lp5521.c | 14 + > drivers/leds/leds-lp5523.c | 33 +- > drivers/leds/leds-lp55xx-common.c | 191 ++++- > drivers/leds/leds-lp55xx-common.h | 11 + > include/dt-bindings/leds/common.h | 3 +- > include/linux/led-class-multicolor.h | 88 ++ > include/linux/platform_data/leds-lp55xx.h | 6 + > 21 files changed, 1928 insertions(+), 93 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor > create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt > create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.txt > create mode 100644 Documentation/leds/leds-class-multicolor.rst > create mode 100644 drivers/leds/led-class-multicolor.c > create mode 100644 drivers/leds/leds-lp50xx.c > create mode 100644 include/linux/led-class-multicolor.h >
Dan, On 10/1/19 4:56 PM, Dan Murphy wrote: > Introduce the LP5036/30/24/18/12/9 RGB LED driver. > The difference in these parts are the number of > LED outputs where the: > > LP5036 can control 36 LEDs > LP5030 can control 30 LEDs > LP5024 can control 24 LEDs > LP5018 can control 18 LEDs > LP5012 can control 12 LEDs > LP5009 can control 9 LEDs > > The device has the ability to group LED output into control banks > so that multiple LED banks can be controlled with the same mixing and > brightness. Inversely the LEDs can also be controlled independently. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/Kconfig | 11 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-lp50xx.c | 784 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 796 insertions(+) > create mode 100644 drivers/leds/leds-lp50xx.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index cfb1ebb6517f..84f60e35c5ee 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -363,6 +363,17 @@ config LEDS_LP3952 > To compile this driver as a module, choose M here: the > module will be called leds-lp3952. > > +config LEDS_LP50XX > + tristate "LED Support for TI LP5036/30/24/18/12/9 LED driver chip" > + depends on LEDS_CLASS && REGMAP_I2C > + depends on LEDS_CLASS_MULTI_COLOR > + help > + If you say yes here you get support for the Texas Instruments > + LP5036, LP5030, LP5024, LP5018, LP5012 and LP5009 LED driver. > + > + To compile this driver as a module, choose M here: the > + module will be called 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 > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 841038cfe35b..7a208a0f7b84 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o > obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o > obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o > obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o > +obj-$(CONFIG_LEDS_LP50XX) += leds-lp50xx.o > obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o > obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o > obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o > diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c [...] > +static int lp50xx_probe_dt(struct lp50xx *priv) > +{ > + u32 led_banks[LP5036_MAX_LED_MODULES]; > + struct fwnode_handle *child = NULL; > + struct fwnode_handle *led_node = NULL; > + struct led_init_data init_data; > + struct lp50xx_led *led; > + int num_colors; > + u32 color_id; > + int led_number; > + size_t i = 0; > + int ret; > + > + priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev, > + "enable", GPIOD_OUT_LOW); > + if (IS_ERR(priv->enable_gpio)) { > + ret = PTR_ERR(priv->enable_gpio); > + dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n", > + ret); > + return ret; > + } > + > + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); > + if (IS_ERR(priv->regulator)) > + priv->regulator = NULL; > + > + device_for_each_child_node(&priv->client->dev, child) { > + led = &priv->leds[i]; > + if (fwnode_property_present(child, "ti,led-bank")) { > + ret = fwnode_property_read_u32_array(child, > + "ti,led-bank", > + NULL, 0); > + ret = fwnode_property_read_u32_array(child, > + "ti,led-bank", > + led_banks, > + ret); You could check if bank numbers are within a range. > + if (ret) { > + dev_err(&priv->client->dev, > + "led-bank property is missing\n"); > + fwnode_handle_put(child); > + goto child_out; > + } > + > + priv->num_of_banked_leds = ARRAY_SIZE(led_banks); > + > + ret = lp50xx_set_banks(priv, led_banks); > + if (ret) { > + dev_err(&priv->client->dev, > + "Cannot setup banked LEDs\n"); > + fwnode_handle_put(child); > + goto child_out; > + } > + led->ctrl_bank_enabled = 1; > + > + } else { > + ret = fwnode_property_read_u32(child, "reg", > + &led_number); The same applies to the led_number. > + if (ret) { > + dev_err(&priv->client->dev, > + "led reg property missing\n"); > + fwnode_handle_put(child); > + goto child_out; > + } > + > + led->led_number = led_number; > + } -- Best regards, Jacek Anaszewski
Jacek Thanks for the review On 10/6/19 10:23 AM, Jacek Anaszewski wrote: > Dan, > > Thank you for the update. > > On 10/1/19 4:56 PM, Dan Murphy wrote: >> Introduce a multicolor class that groups colored LEDs >> within a LED node. >> >> The multi color class groups monochrome LEDs and allows controlling two >> aspects of the final combined color: hue and lightness. The former is >> controlled via <color>_intensity files and the latter is controlled >> via brightness file. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> .../ABI/testing/sysfs-class-led-multicolor | 35 +++ >> Documentation/leds/index.rst | 1 + >> Documentation/leds/leds-class-multicolor.rst | 96 +++++++ >> drivers/leds/Kconfig | 10 + >> drivers/leds/Makefile | 1 + >> drivers/leds/led-class-multicolor.c | 268 ++++++++++++++++++ >> include/linux/led-class-multicolor.h | 88 ++++++ >> 7 files changed, 499 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor >> create mode 100644 Documentation/leds/leds-class-multicolor.rst >> create mode 100644 drivers/leds/led-class-multicolor.c >> create mode 100644 include/linux/led-class-multicolor.h >> >> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor >> new file mode 100644 >> index 000000000000..65cb43de26e6 >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor >> @@ -0,0 +1,35 @@ >> +What: /sys/class/leds/<led>/brightness >> +Date: Sept 2019 >> +KernelVersion: 5.5 >> +Contact: Dan Murphy <dmurphy@ti.com> >> +Description: read/write >> + Writing to this file will update all LEDs within the group to a >> + calculated percentage of what each color LED intensity is set >> + to. The percentage is calculated via the equation below: >> + >> + led_brightness = brightness * <color>_intensity/<color>_max_intensity >> + >> + For additional details please refer to >> + Documentation/leds/leds-class-multicolor.rst. >> + >> + The value of the color is from 0 to >> + /sys/class/leds/<led>/max_brightness. >> + >> +What: /sys/class/leds/<led>/colors/<color>_intensity >> +Date: Sept 2019 >> +KernelVersion: 5.5 >> +Contact: Dan Murphy <dmurphy@ti.com> >> +Description: read/write >> + The <color>_intensity file is created based on the color >> + defined by the registrar of the class. >> + There is one file per color presented. >> + >> + The value of the color is from 0 to >> + /sys/class/leds/<led>/colors/<color>_max_intensity. >> + >> +What: /sys/class/leds/<led>/colors/<color>_max_intensity >> +Date: Sept 2019 >> +KernelVersion: 5.5 >> +Contact: Dan Murphy <dmurphy@ti.com> >> +Description: read only >> + Maximum intensity level for the LED color. >> diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst >> index 060f4e485897..bc70c6aa7138 100644 >> --- a/Documentation/leds/index.rst >> +++ b/Documentation/leds/index.rst >> @@ -9,6 +9,7 @@ LEDs >> >> leds-class >> leds-class-flash >> + leds-class-multicolor >> ledtrig-oneshot >> ledtrig-transient >> ledtrig-usbport >> diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst >> new file mode 100644 >> index 000000000000..87a1588d7619 >> --- /dev/null >> +++ b/Documentation/leds/leds-class-multicolor.rst >> @@ -0,0 +1,96 @@ >> +==================================== >> +Multi Color LED handling under Linux >> +==================================== >> + >> +Description >> +=========== >> +The multi color class groups monochrome LEDs and allows controlling two >> +aspects of the final combined color: hue and lightness. The former is >> +controlled via <color>_intensity files and the latter is controlled >> +via brightness file. >> + >> +For more details on hue and lightness notions please refer to >> +https://en.wikipedia.org/wiki/CIECAM02. >> + >> +Note that intensity files only cache the written value and the actual >> +change of hardware state occurs upon writing brightness file. This >> +allows for changing many factors of the perceived color in a virtually >> +unnoticeable way for the human observer. >> + >> +Multicolor Class Control >> +======================== >> +The multicolor class presents the LED groups under a directory called "colors". >> +This directory is a child under the LED parent node created by the led_class >> +framework. The led_class framework is documented in led-class.rst within this >> +documentation directory. >> + >> +Each colored LED will have two files created under the colors directory >> +<color>_intensity and <color>_max_intensity. These files will contain >> +one of LED_COLOR_ID_* definitions from the header >> +include/dt-bindings/leds/common.h. >> + >> +Directory Layout Example >> +======================== >> +root:/sys/class/leds/rgb:grouped_leds# ls -lR colors/ >> +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 red_max_intensity >> +--w--wx-w- 1 root root 4096 Jul 7 03:10 red_intensity >> +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 green_max_intensity >> +--w--wx-w- 1 root root 4096 Jul 7 03:10 green_intensity >> +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 blue_max_intensity >> +--w--wx-w- 1 root root 4096 Jul 7 03:10 blue_intensity > Now when you have fixed file permissions in LED mc core the > 'ls -l' should report them correctly, so this needs to be > updated. ACK > [...] >> +int devm_led_classdev_multicolor_register(struct device *parent, >> + struct led_classdev_mc *mcled_cdev) >> +{ >> + struct led_classdev_mc **dr; >> + int ret; >> + >> + dr = devres_alloc(devm_led_classdev_multicolor_release, >> + sizeof(*dr), GFP_KERNEL); >> + if (!dr) >> + return -ENOMEM; >> + >> + ret = led_classdev_multicolor_register(parent, mcled_cdev); >> + if (ret) { >> + devres_free(dr); >> + return ret; >> + } >> + >> + *dr = mcled_cdev; >> + devres_add(parent, dr); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_register); >> + >> +static int devm_led_classdev_multicolor_match(struct device *dev, >> + void *res, void *data) >> +{ >> + struct mcled_cdev **p = res; > s/mcled_cdev/led_classdev_mc/ ACK. Fixed in v11 Dan >> + >> + if (WARN_ON(!p || !*p)) >> + return 0; >> + >> + return *p == data; >> +} >> + > [...]
Jacek On 10/6/19 11:12 AM, Jacek Anaszewski wrote: > Dan, > > On 10/1/19 4:56 PM, Dan Murphy wrote: >> Introduce the LP5036/30/24/18/12/9 RGB LED driver. >> The difference in these parts are the number of >> LED outputs where the: >> >> LP5036 can control 36 LEDs >> LP5030 can control 30 LEDs >> LP5024 can control 24 LEDs >> LP5018 can control 18 LEDs >> LP5012 can control 12 LEDs >> LP5009 can control 9 LEDs >> >> The device has the ability to group LED output into control banks >> so that multiple LED banks can be controlled with the same mixing and >> brightness. Inversely the LEDs can also be controlled independently. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/leds/Kconfig | 11 + >> drivers/leds/Makefile | 1 + >> drivers/leds/leds-lp50xx.c | 784 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 796 insertions(+) >> create mode 100644 drivers/leds/leds-lp50xx.c >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index cfb1ebb6517f..84f60e35c5ee 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -363,6 +363,17 @@ config LEDS_LP3952 >> To compile this driver as a module, choose M here: the >> module will be called leds-lp3952. >> >> +config LEDS_LP50XX >> + tristate "LED Support for TI LP5036/30/24/18/12/9 LED driver chip" >> + depends on LEDS_CLASS && REGMAP_I2C >> + depends on LEDS_CLASS_MULTI_COLOR >> + help >> + If you say yes here you get support for the Texas Instruments >> + LP5036, LP5030, LP5024, LP5018, LP5012 and LP5009 LED driver. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called 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 >> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >> index 841038cfe35b..7a208a0f7b84 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -34,6 +34,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o >> obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o >> obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o >> obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o >> +obj-$(CONFIG_LEDS_LP50XX) += leds-lp50xx.o >> obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o >> obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o >> obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o >> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c > [...] >> +static int lp50xx_probe_dt(struct lp50xx *priv) >> +{ >> + u32 led_banks[LP5036_MAX_LED_MODULES]; >> + struct fwnode_handle *child = NULL; >> + struct fwnode_handle *led_node = NULL; >> + struct led_init_data init_data; >> + struct lp50xx_led *led; >> + int num_colors; >> + u32 color_id; >> + int led_number; >> + size_t i = 0; >> + int ret; >> + >> + priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev, >> + "enable", GPIOD_OUT_LOW); >> + if (IS_ERR(priv->enable_gpio)) { >> + ret = PTR_ERR(priv->enable_gpio); >> + dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n", >> + ret); >> + return ret; >> + } >> + >> + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); >> + if (IS_ERR(priv->regulator)) >> + priv->regulator = NULL; >> + >> + device_for_each_child_node(&priv->client->dev, child) { >> + led = &priv->leds[i]; >> + if (fwnode_property_present(child, "ti,led-bank")) { >> + ret = fwnode_property_read_u32_array(child, >> + "ti,led-bank", >> + NULL, 0); >> + ret = fwnode_property_read_u32_array(child, >> + "ti,led-bank", >> + led_banks, >> + ret); > You could check if bank numbers are within a range. Ack > >> + if (ret) { >> + dev_err(&priv->client->dev, >> + "led-bank property is missing\n"); >> + fwnode_handle_put(child); >> + goto child_out; >> + } >> + >> + priv->num_of_banked_leds = ARRAY_SIZE(led_banks); >> + >> + ret = lp50xx_set_banks(priv, led_banks); >> + if (ret) { >> + dev_err(&priv->client->dev, >> + "Cannot setup banked LEDs\n"); >> + fwnode_handle_put(child); >> + goto child_out; >> + } >> + led->ctrl_bank_enabled = 1; >> + >> + } else { >> + ret = fwnode_property_read_u32(child, "reg", >> + &led_number); > The same applies to the led_number. NACK to this. This is checked below. But I should probably move that check up into the else case. Dan > >> + if (ret) { >> + dev_err(&priv->client->dev, >> + "led reg property missing\n"); >> + fwnode_handle_put(child); >> + goto child_out; >> + } >> + >> + led->led_number = led_number; >> + }
Jacek On 10/6/19 2:52 PM, Jacek Anaszewski wrote: > Dan, > > On 10/1/19 4:56 PM, Dan Murphy wrote: >> Add multicolor framework support for the lp55xx family. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/leds/Kconfig | 1 + >> drivers/leds/leds-lp55xx-common.c | 169 +++++++++++++++++++--- >> drivers/leds/leds-lp55xx-common.h | 11 ++ >> include/linux/platform_data/leds-lp55xx.h | 6 + >> 4 files changed, 163 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index 84f60e35c5ee..dc3d9f2194cd 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -377,6 +377,7 @@ 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 LEDS_CLASS_MULTI_COLOR && OF >> select FW_LOADER >> select FW_LOADER_USER_HELPER >> help >> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c >> index 44ced02b49f9..5de4f1789a44 100644 >> --- a/drivers/leds/leds-lp55xx-common.c >> +++ b/drivers/leds/leds-lp55xx-common.c >> @@ -131,14 +131,50 @@ static struct attribute *lp55xx_led_attrs[] = { >> }; >> ATTRIBUTE_GROUPS(lp55xx_led); >> >> +struct led_mc_color_conversion color_component[LP55XX_MAX_GROUPED_CHAN]; > Why is this global? Please move it to lp55xx_set_brightness(). ACK > >> + >> +static int lp55xx_get_channel(struct lp55xx_led *led, int color_id) >> +{ >> + int i; >> + >> + for (i = 0; i < led->mc_cdev.num_leds; i++) { >> + if (led->channel_color[i] == color_id) >> + return led->grouped_channels[i]; >> + } >> + >> + return -EINVAL; >> +} >> + >> static int lp55xx_set_brightness(struct led_classdev *cdev, >> enum led_brightness brightness) >> { >> struct lp55xx_led *led = cdev_to_lp55xx_led(cdev); >> struct lp55xx_device_config *cfg = led->chip->cfg; >> + int channel_num; >> + int ret; >> + int i; >> >> - led->brightness = (u8)brightness; >> - return cfg->brightness_fn(led); >> + if (led->mc_cdev.num_leds > 1) { >> + led_mc_calc_brightness(&led->mc_cdev, brightness, >> + &color_component[0]); > s/&color_component[0]/color_component/ ACK >> + >> + for (i = 0; i < led->mc_cdev.num_leds; i++) { >> + channel_num = lp55xx_get_channel(led, >> + color_component[i].color_id); >> + if (channel_num < 0) >> + return channel_num; >> + >> + ret = cfg->color_intensity_fn(led, channel_num, >> + color_component[i].brightness); > If you passed struct led_mc_color_conversion instead of brightness, > then in the color_intensity_fn op you could obtain channel numbers > by calling lp55xx_get_channel in a loop. And you could setup the whole > cluster in a single call. Hmm. How would that be an improvement? Maybe the answer lies down below in my response and we will not have to get the channel_num as we can make the output part of the mc_color_conversion struct. As I pointed out in v9 "Beyond that in coding this and thinking about the design it is better to have the lp55xx_common code to do all the heavy lifting and the children to just perform the action on the device itself" https://lore.kernel.org/linux-leds/4186e454-48fd-1578-cd26-083b54c707ab@gmail.com/T/#u The children should not have to know if the LED is registered to the LED class, MC Class or Flash class only the common code should know this information. Just need to keep the child code simple. This is why I pass in the values as opposed to having the child figure it out. >> + if (ret) >> + return ret; >> + } >> + } else { >> + led->brightness = (u8)brightness; >> + ret = cfg->brightness_fn(led); >> + } >> + >> + return ret; >> } >> >> static int lp55xx_init_led(struct lp55xx_led *led, >> @@ -147,9 +183,9 @@ static int lp55xx_init_led(struct lp55xx_led *led, >> struct lp55xx_platform_data *pdata = chip->pdata; >> struct lp55xx_device_config *cfg = chip->cfg; >> struct device *dev = &chip->cl->dev; >> + int max_channel = cfg->max_channel; >> char name[32]; >> int ret; >> - int max_channel = cfg->max_channel; >> >> if (chan >= max_channel) { >> dev_err(dev, "invalid channel: %d / %d\n", chan, max_channel); >> @@ -159,10 +195,37 @@ static int lp55xx_init_led(struct lp55xx_led *led, >> if (pdata->led_config[chan].led_current == 0) >> return 0; >> >> + if (pdata->led_config[chan].name) { >> + led->cdev.name = pdata->led_config[chan].name; >> + } else { >> + snprintf(name, sizeof(name), "%s:channel%d", >> + pdata->label ? : chip->cl->name, chan); >> + led->cdev.name = name; >> + } >> + >> + if (pdata->led_config[chan].num_colors > 1) { >> + led->mc_cdev.led_cdev = &led->cdev; >> + led->cdev.brightness_set_blocking = lp55xx_set_brightness; >> + led->cdev.groups = lp55xx_led_groups; >> + led->mc_cdev.num_leds = pdata->led_config[chan].num_colors; >> + led->mc_cdev.available_colors = >> + pdata->led_config[chan].available_colors; >> + memcpy(led->channel_color, >> + pdata->led_config[chan].channel_color, >> + sizeof(led->channel_color)); >> + memcpy(led->grouped_channels, >> + pdata->led_config[chan].grouped_channels, >> + sizeof(led->grouped_channels)); >> + } else { >> + >> + led->cdev.default_trigger = >> + pdata->led_config[chan].default_trigger; >> + led->cdev.brightness_set_blocking = lp55xx_set_brightness; >> + } led->cdev.groups = lp55xx_led_groups; >> + >> led->led_current = pdata->led_config[chan].led_current; >> led->max_current = pdata->led_config[chan].max_current; >> led->chan_nr = pdata->led_config[chan].chan_nr; >> - led->cdev.default_trigger = pdata->led_config[chan].default_trigger; >> >> if (led->chan_nr >= max_channel) { >> dev_err(dev, "Use channel numbers between 0 and %d\n", >> @@ -170,18 +233,11 @@ static int lp55xx_init_led(struct lp55xx_led *led, >> return -EINVAL; >> } >> >> - led->cdev.brightness_set_blocking = lp55xx_set_brightness; >> - led->cdev.groups = lp55xx_led_groups; >> + if (pdata->led_config[chan].num_colors > 1) >> + ret = led_classdev_multicolor_register(dev, &led->mc_cdev); >> + else >> + ret = led_classdev_register(dev, &led->cdev); >> >> - if (pdata->led_config[chan].name) { >> - led->cdev.name = pdata->led_config[chan].name; >> - } else { >> - snprintf(name, sizeof(name), "%s:channel%d", >> - pdata->label ? : chip->cl->name, chan); >> - led->cdev.name = name; >> - } >> - >> - ret = led_classdev_register(dev, &led->cdev); >> if (ret) { >> dev_err(dev, "led register err: %d\n", ret); >> return ret; >> @@ -466,7 +522,6 @@ int lp55xx_register_leds(struct lp55xx_led *led, struct lp55xx_chip *chip) >> dev_err(&chip->cl->dev, "empty brightness configuration\n"); >> return -EINVAL; >> } >> - >> for (i = 0; i < num_channels; i++) { >> >> /* do not initialize channels that are not connected */ >> @@ -538,6 +593,76 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip) >> } >> EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs); >> >> +static int lp5xx_parse_common_child(struct device_node *np, >> + struct lp55xx_led_config *cfg, >> + int chan_num, bool is_multicolor) >> +{ >> + u32 led_number; >> + int ret; >> + >> + of_property_read_string(np, "chan-name", >> + &cfg[chan_num].name); >> + of_property_read_u8(np, "led-cur", >> + &cfg[chan_num].led_current); >> + of_property_read_u8(np, "max-cur", >> + &cfg[chan_num].max_current); >> + >> + ret = of_property_read_u32(np, "reg", &led_number); >> + if (ret) >> + return ret; >> + >> + if (led_number < 0 || led_number > 6) >> + return -EINVAL; >> + >> + if (is_multicolor) >> + cfg[chan_num].grouped_channels[cfg[chan_num].num_colors] > Please pass the index for grouped channels as an argument to this > function. Referring here directly to a temporary state of num_colors > that is incremented in the loop from which this function is called > is ugly IMO. Ack >> + = led_number; >> + else >> + cfg[chan_num].chan_nr = led_number; >> + >> + return 0; >> +} >> + >> +static int lp5xx_parse_channel_child(struct device_node *np, >> + struct lp55xx_led_config *cfg, >> + int child_number) >> +{ >> + struct device_node *child; >> + int channel_color; >> + u32 color_id; >> + int ret; >> + >> + cfg[child_number].default_trigger = >> + of_get_property(np, "linux,default-trigger", NULL); >> + >> + ret = of_property_read_u32(np, "color", &channel_color); >> + if (ret) >> + channel_color = ret; >> + >> + >> + if (channel_color == LED_COLOR_ID_MULTI) { >> + for_each_child_of_node(np, child) { >> + ret = lp5xx_parse_common_child(child, cfg, >> + child_number, true); >> + if (ret) >> + return ret; >> + ret = of_property_read_u32(child, "color", &color_id); >> + if (ret) >> + return ret; >> + >> + cfg[child_number].channel_color[cfg[child_number].num_colors] = >> + color_id; >> + set_bit(color_id, &cfg[child_number].available_colors); >> + >> + cfg[child_number].num_colors++; >> + } >> + } else { >> + return lp5xx_parse_common_child(np, cfg, child_number, false); >> + } >> + >> + return 0; >> +} >> + >> struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev, >> struct device_node *np) >> { >> @@ -546,6 +671,7 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev, >> struct lp55xx_led_config *cfg; >> int num_channels; >> int i = 0; >> + int ret; >> >> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >> if (!pdata) >> @@ -565,14 +691,9 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev, >> pdata->num_channels = num_channels; >> >> for_each_child_of_node(np, child) { >> - cfg[i].chan_nr = i; >> - >> - of_property_read_string(child, "chan-name", &cfg[i].name); >> - of_property_read_u8(child, "led-cur", &cfg[i].led_current); >> - of_property_read_u8(child, "max-cur", &cfg[i].max_current); >> - cfg[i].default_trigger = >> - of_get_property(child, "linux,default-trigger", NULL); >> - >> + ret = lp5xx_parse_channel_child(child, cfg, i); >> + if (ret) >> + return ERR_PTR(-EINVAL); >> i++; >> } >> >> diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h >> index 783ed5103ce5..5ea2a292a97e 100644 >> --- a/drivers/leds/leds-lp55xx-common.h >> +++ b/drivers/leds/leds-lp55xx-common.h >> @@ -12,6 +12,10 @@ >> #ifndef _LEDS_LP55XX_COMMON_H >> #define _LEDS_LP55XX_COMMON_H >> >> +#include <linux/led-class-multicolor.h> >> + >> +#define LP55XX_MAX_GROUPED_CHAN 4 >> + >> enum lp55xx_engine_index { >> LP55XX_ENGINE_INVALID, >> LP55XX_ENGINE_1, >> @@ -109,6 +113,9 @@ struct lp55xx_device_config { >> /* access brightness register */ >> int (*brightness_fn)(struct lp55xx_led *led); >> >> + /* access specific brightness register */ >> + int (*color_intensity_fn)(struct lp55xx_led *led, int chan_num, int brightness); >> + >> /* current setting function */ >> void (*set_led_current) (struct lp55xx_led *led, u8 led_current); >> >> @@ -159,6 +166,7 @@ struct lp55xx_chip { >> * struct lp55xx_led >> * @chan_nr : Channel number >> * @cdev : LED class device >> + * @mc_cdev : Multi color class device >> * @led_current : Current setting at each led channel >> * @max_current : Maximun current at each led channel >> * @brightness : Brightness value > Documentation for channel_color and grouped_channels is missing. > ACK >> @@ -167,9 +175,12 @@ struct lp55xx_chip { >> struct lp55xx_led { >> int chan_nr; >> struct led_classdev cdev; >> + struct led_classdev_mc mc_cdev; >> u8 led_current; >> u8 max_current; >> u8 brightness; >> + int channel_color[LP55XX_MAX_GROUPED_CHAN]; >> + int grouped_channels[LP55XX_MAX_GROUPED_CHAN]; > I propose to create structure: > > struct lp55xx_mc_cluster { > int channel_color; > int channel_id; > }; > > and instead of the above two arrays create one > > struct lp55xx_mc_cluster mc_cluster[LP55XX_MAX_GROUPED_CHAN]; Maybe we can extend the mc_color_converion struct to add output_num. Now I did try to do this but the design of the code made it a bit wonky. I will look at it again. If the output_num information is contain in a single struct as opposed to having each driver create their own struct. struct led_mc_color_conversion { int color_id; int brightness; int output_num; }; struct led_mc_color_conversion mc_cluster[LP55XX_MAX_GROUPED_CHAN]; If other drivers do not need that information then they do not need to populate it > >> struct lp55xx_chip *chip; >> }; >> >> diff --git a/include/linux/platform_data/leds-lp55xx.h b/include/linux/platform_data/leds-lp55xx.h >> index 96a787100fda..0ac29f537ab6 100644 >> --- a/include/linux/platform_data/leds-lp55xx.h >> +++ b/include/linux/platform_data/leds-lp55xx.h >> @@ -12,6 +12,8 @@ >> #ifndef _LEDS_LP55XX_H >> #define _LEDS_LP55XX_H >> >> +#include <linux/led-class-multicolor.h> >> + >> /* Clock configuration */ >> #define LP55XX_CLOCK_AUTO 0 >> #define LP55XX_CLOCK_INT 1 >> @@ -23,6 +25,10 @@ struct lp55xx_led_config { >> u8 chan_nr; >> u8 led_current; /* mA x10, 0 if led is not connected */ >> u8 max_current; >> + int num_colors; >> + unsigned long available_colors; >> + u32 channel_color[LED_COLOR_ID_MAX]; > channel_color array is redundant if you have available_colors flags. I will look this again. >> + int grouped_channels[LED_COLOR_ID_MAX]; >> }; >> >> struct lp55xx_predef_pattern { >>
Dan, On 10/7/19 7:08 PM, Dan Murphy wrote: > Jacek > > On 10/6/19 2:52 PM, Jacek Anaszewski wrote: >> Dan, >> >> On 10/1/19 4:56 PM, Dan Murphy wrote: >>> Add multicolor framework support for the lp55xx family. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> --- >>> drivers/leds/Kconfig | 1 + >>> drivers/leds/leds-lp55xx-common.c | 169 +++++++++++++++++++--- >>> drivers/leds/leds-lp55xx-common.h | 11 ++ >>> include/linux/platform_data/leds-lp55xx.h | 6 + >>> 4 files changed, 163 insertions(+), 24 deletions(-) >>> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 84f60e35c5ee..dc3d9f2194cd 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -377,6 +377,7 @@ 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 LEDS_CLASS_MULTI_COLOR && OF >>> select FW_LOADER >>> select FW_LOADER_USER_HELPER >>> help >>> diff --git a/drivers/leds/leds-lp55xx-common.c >>> b/drivers/leds/leds-lp55xx-common.c >>> index 44ced02b49f9..5de4f1789a44 100644 >>> --- a/drivers/leds/leds-lp55xx-common.c >>> +++ b/drivers/leds/leds-lp55xx-common.c >>> @@ -131,14 +131,50 @@ static struct attribute *lp55xx_led_attrs[] = { >>> }; >>> ATTRIBUTE_GROUPS(lp55xx_led); >>> +struct led_mc_color_conversion >>> color_component[LP55XX_MAX_GROUPED_CHAN]; >> Why is this global? Please move it to lp55xx_set_brightness(). > ACK >> >>> + >>> +static int lp55xx_get_channel(struct lp55xx_led *led, int color_id) >>> +{ >>> + int i; >>> + >>> + for (i = 0; i < led->mc_cdev.num_leds; i++) { >>> + if (led->channel_color[i] == color_id) >>> + return led->grouped_channels[i]; >>> + } >>> + >>> + return -EINVAL; >>> +} >>> + >>> static int lp55xx_set_brightness(struct led_classdev *cdev, >>> enum led_brightness brightness) >>> { >>> struct lp55xx_led *led = cdev_to_lp55xx_led(cdev); >>> struct lp55xx_device_config *cfg = led->chip->cfg; >>> + int channel_num; >>> + int ret; >>> + int i; >>> - led->brightness = (u8)brightness; >>> - return cfg->brightness_fn(led); >>> + if (led->mc_cdev.num_leds > 1) { >>> + led_mc_calc_brightness(&led->mc_cdev, brightness, >>> + &color_component[0]); >> s/&color_component[0]/color_component/ > > ACK > > >>> + >>> + for (i = 0; i < led->mc_cdev.num_leds; i++) { >>> + channel_num = lp55xx_get_channel(led, >>> + color_component[i].color_id); >>> + if (channel_num < 0) >>> + return channel_num; >>> + >>> + ret = cfg->color_intensity_fn(led, channel_num, >>> + color_component[i].brightness); >> If you passed struct led_mc_color_conversion instead of brightness, >> then in the color_intensity_fn op you could obtain channel numbers >> by calling lp55xx_get_channel in a loop. And you could setup the whole >> cluster in a single call. > > Hmm. How would that be an improvement? Maybe the answer lies down I mentioned that before - think of sleeping on mutex on contention (keep it mind that brightness will be also set from triggers and from the workqueue in effect, and userspace can interfere that). That could add unpleasant delays between setting color components. It could be worked around by executing the for loop here under mutex, but would be non-uniform with regard to handling monochrome brightness setting via cfg->brightness_fn, which relies on chip->lock taken by it. You need one of two options: either the whole for loop here under mutex or on the driver side. > below in my response and we will not have to get the channel_num as we > can make the output part of the mc_color_conversion struct. > > As I pointed out in v9 "Beyond that in coding this and thinking about > the design it is better to have the lp55xx_common code to do all the > heavy lifting and the children to just perform the action on the device > itself" > > https://lore.kernel.org/linux-leds/4186e454-48fd-1578-cd26-083b54c707ab@gmail.com/T/#u > > > The children should not have to know if the LED is registered to the LED > class, MC Class or Flash class only the common code should know this > information. Just need to keep the child code simple. This is why I > pass in the values as opposed to having the child figure it out. With mc class we have a bit different perspective - the children are not standalone LED class devices. Of course it can be modeled this way as well, but it can result in a misleading impression that the LEDs are independent. If we set the all colors in one op for mc class case then it would nicely highlight that use case on the driver side. > >>> + if (ret) >>> + return ret; >>> + } >>> + } else { >>> + led->brightness = (u8)brightness; >>> + ret = cfg->brightness_fn(led); >>> + } >>> + >>> + return ret; >>> } >>> static int lp55xx_init_led(struct lp55xx_led *led, >>> @@ -147,9 +183,9 @@ static int lp55xx_init_led(struct lp55xx_led *led, >>> struct lp55xx_platform_data *pdata = chip->pdata; >>> struct lp55xx_device_config *cfg = chip->cfg; >>> struct device *dev = &chip->cl->dev; >>> + int max_channel = cfg->max_channel; >>> char name[32]; >>> int ret; >>> - int max_channel = cfg->max_channel; >>> if (chan >= max_channel) { >>> dev_err(dev, "invalid channel: %d / %d\n", chan, max_channel); >>> @@ -159,10 +195,37 @@ static int lp55xx_init_led(struct lp55xx_led *led, >>> if (pdata->led_config[chan].led_current == 0) >>> return 0; >>> + if (pdata->led_config[chan].name) { >>> + led->cdev.name = pdata->led_config[chan].name; >>> + } else { >>> + snprintf(name, sizeof(name), "%s:channel%d", >>> + pdata->label ? : chip->cl->name, chan); >>> + led->cdev.name = name; >>> + } >>> + >>> + if (pdata->led_config[chan].num_colors > 1) { >>> + led->mc_cdev.led_cdev = &led->cdev; >>> + led->cdev.brightness_set_blocking = lp55xx_set_brightness; >>> + led->cdev.groups = lp55xx_led_groups; >>> + led->mc_cdev.num_leds = pdata->led_config[chan].num_colors; >>> + led->mc_cdev.available_colors = >>> + pdata->led_config[chan].available_colors; >>> + memcpy(led->channel_color, >>> + pdata->led_config[chan].channel_color, >>> + sizeof(led->channel_color)); >>> + memcpy(led->grouped_channels, >>> + pdata->led_config[chan].grouped_channels, >>> + sizeof(led->grouped_channels)); >>> + } else { >>> + >>> + led->cdev.default_trigger = >>> + pdata->led_config[chan].default_trigger; >>> + led->cdev.brightness_set_blocking = lp55xx_set_brightness; >>> + } led->cdev.groups = lp55xx_led_groups; >>> + >>> led->led_current = pdata->led_config[chan].led_current; >>> led->max_current = pdata->led_config[chan].max_current; >>> led->chan_nr = pdata->led_config[chan].chan_nr; >>> - led->cdev.default_trigger = >>> pdata->led_config[chan].default_trigger; >>> if (led->chan_nr >= max_channel) { >>> dev_err(dev, "Use channel numbers between 0 and %d\n", >>> @@ -170,18 +233,11 @@ static int lp55xx_init_led(struct lp55xx_led *led, >>> return -EINVAL; >>> } >>> - led->cdev.brightness_set_blocking = lp55xx_set_brightness; >>> - led->cdev.groups = lp55xx_led_groups; >>> + if (pdata->led_config[chan].num_colors > 1) >>> + ret = led_classdev_multicolor_register(dev, &led->mc_cdev); >>> + else >>> + ret = led_classdev_register(dev, &led->cdev); >>> - if (pdata->led_config[chan].name) { >>> - led->cdev.name = pdata->led_config[chan].name; >>> - } else { >>> - snprintf(name, sizeof(name), "%s:channel%d", >>> - pdata->label ? : chip->cl->name, chan); >>> - led->cdev.name = name; >>> - } >>> - >>> - ret = led_classdev_register(dev, &led->cdev); >>> if (ret) { >>> dev_err(dev, "led register err: %d\n", ret); >>> return ret; >>> @@ -466,7 +522,6 @@ int lp55xx_register_leds(struct lp55xx_led *led, >>> struct lp55xx_chip *chip) >>> dev_err(&chip->cl->dev, "empty brightness configuration\n"); >>> return -EINVAL; >>> } >>> - >>> for (i = 0; i < num_channels; i++) { >>> /* do not initialize channels that are not connected */ >>> @@ -538,6 +593,76 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip >>> *chip) >>> } >>> EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs); >>> +static int lp5xx_parse_common_child(struct device_node *np, >>> + struct lp55xx_led_config *cfg, >>> + int chan_num, bool is_multicolor) >>> +{ >>> + u32 led_number; >>> + int ret; >>> + >>> + of_property_read_string(np, "chan-name", >>> + &cfg[chan_num].name); >>> + of_property_read_u8(np, "led-cur", >>> + &cfg[chan_num].led_current); >>> + of_property_read_u8(np, "max-cur", >>> + &cfg[chan_num].max_current); >>> + >>> + ret = of_property_read_u32(np, "reg", &led_number); >>> + if (ret) >>> + return ret; >>> + >>> + if (led_number < 0 || led_number > 6) >>> + return -EINVAL; >>> + >>> + if (is_multicolor) >>> + cfg[chan_num].grouped_channels[cfg[chan_num].num_colors] >> Please pass the index for grouped channels as an argument to this >> function. Referring here directly to a temporary state of num_colors >> that is incremented in the loop from which this function is called >> is ugly IMO. > > Ack > > >>> + = led_number; >>> + else >>> + cfg[chan_num].chan_nr = led_number; >>> + >>> + return 0; >>> +} >>> + >>> +static int lp5xx_parse_channel_child(struct device_node *np, >>> + struct lp55xx_led_config *cfg, >>> + int child_number) >>> +{ >>> + struct device_node *child; >>> + int channel_color; >>> + u32 color_id; >>> + int ret; >>> + >>> + cfg[child_number].default_trigger = >>> + of_get_property(np, "linux,default-trigger", NULL); >>> + >>> + ret = of_property_read_u32(np, "color", &channel_color); >>> + if (ret) >>> + channel_color = ret; >>> + >>> + >>> + if (channel_color == LED_COLOR_ID_MULTI) { >>> + for_each_child_of_node(np, child) { >>> + ret = lp5xx_parse_common_child(child, cfg, >>> + child_number, true); >>> + if (ret) >>> + return ret; >>> + ret = of_property_read_u32(child, "color", &color_id); >>> + if (ret) >>> + return ret; >>> + >>> + >>> cfg[child_number].channel_color[cfg[child_number].num_colors] = >>> + color_id; >>> + set_bit(color_id, &cfg[child_number].available_colors); >>> + >>> + cfg[child_number].num_colors++; >>> + } >>> + } else { >>> + return lp5xx_parse_common_child(np, cfg, child_number, false); >>> + } >>> + >>> + return 0; >>> +} >>> + >>> struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device >>> *dev, >>> struct device_node *np) >>> { >>> @@ -546,6 +671,7 @@ struct lp55xx_platform_data >>> *lp55xx_of_populate_pdata(struct device *dev, >>> struct lp55xx_led_config *cfg; >>> int num_channels; >>> int i = 0; >>> + int ret; >>> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >>> if (!pdata) >>> @@ -565,14 +691,9 @@ struct lp55xx_platform_data >>> *lp55xx_of_populate_pdata(struct device *dev, >>> pdata->num_channels = num_channels; >>> for_each_child_of_node(np, child) { >>> - cfg[i].chan_nr = i; >>> - >>> - of_property_read_string(child, "chan-name", &cfg[i].name); >>> - of_property_read_u8(child, "led-cur", &cfg[i].led_current); >>> - of_property_read_u8(child, "max-cur", &cfg[i].max_current); >>> - cfg[i].default_trigger = >>> - of_get_property(child, "linux,default-trigger", NULL); >>> - >>> + ret = lp5xx_parse_channel_child(child, cfg, i); >>> + if (ret) >>> + return ERR_PTR(-EINVAL); >>> i++; >>> } >>> diff --git a/drivers/leds/leds-lp55xx-common.h >>> b/drivers/leds/leds-lp55xx-common.h >>> index 783ed5103ce5..5ea2a292a97e 100644 >>> --- a/drivers/leds/leds-lp55xx-common.h >>> +++ b/drivers/leds/leds-lp55xx-common.h >>> @@ -12,6 +12,10 @@ >>> #ifndef _LEDS_LP55XX_COMMON_H >>> #define _LEDS_LP55XX_COMMON_H >>> +#include <linux/led-class-multicolor.h> >>> + >>> +#define LP55XX_MAX_GROUPED_CHAN 4 >>> + >>> enum lp55xx_engine_index { >>> LP55XX_ENGINE_INVALID, >>> LP55XX_ENGINE_1, >>> @@ -109,6 +113,9 @@ struct lp55xx_device_config { >>> /* access brightness register */ >>> int (*brightness_fn)(struct lp55xx_led *led); >>> + /* access specific brightness register */ >>> + int (*color_intensity_fn)(struct lp55xx_led *led, int chan_num, >>> int brightness); >>> + >>> /* current setting function */ >>> void (*set_led_current) (struct lp55xx_led *led, u8 led_current); >>> @@ -159,6 +166,7 @@ struct lp55xx_chip { >>> * struct lp55xx_led >>> * @chan_nr : Channel number >>> * @cdev : LED class device >>> + * @mc_cdev : Multi color class device >>> * @led_current : Current setting at each led channel >>> * @max_current : Maximun current at each led channel >>> * @brightness : Brightness value >> Documentation for channel_color and grouped_channels is missing. >> > ACK > > >>> @@ -167,9 +175,12 @@ struct lp55xx_chip { >>> struct lp55xx_led { >>> int chan_nr; >>> struct led_classdev cdev; >>> + struct led_classdev_mc mc_cdev; >>> u8 led_current; >>> u8 max_current; >>> u8 brightness; >>> + int channel_color[LP55XX_MAX_GROUPED_CHAN]; >>> + int grouped_channels[LP55XX_MAX_GROUPED_CHAN]; >> I propose to create structure: >> >> struct lp55xx_mc_cluster { >> int channel_color; >> int channel_id; >> }; >> >> and instead of the above two arrays create one >> >> struct lp55xx_mc_cluster mc_cluster[LP55XX_MAX_GROUPED_CHAN]; > > Maybe we can extend the mc_color_converion struct to add output_num. > > Now I did try to do this but the design of the code made it a bit > wonky. I will look at it again. > > If the output_num information is contain in a single struct as opposed > to having each driver create their own struct. > > struct led_mc_color_conversion { > int color_id; > int brightness; > > int output_num; > > }; > > struct led_mc_color_conversion mc_cluster[LP55XX_MAX_GROUPED_CHAN]; > > If other drivers do not need that information then they do not need to > populate it Maybe, feel free to give it a try. struct lp55xx_chip *chip; >>> }; >>> diff --git a/include/linux/platform_data/leds-lp55xx.h >>> b/include/linux/platform_data/leds-lp55xx.h >>> index 96a787100fda..0ac29f537ab6 100644 >>> --- a/include/linux/platform_data/leds-lp55xx.h >>> +++ b/include/linux/platform_data/leds-lp55xx.h >>> @@ -12,6 +12,8 @@ >>> #ifndef _LEDS_LP55XX_H >>> #define _LEDS_LP55XX_H >>> +#include <linux/led-class-multicolor.h> >>> + >>> /* Clock configuration */ >>> #define LP55XX_CLOCK_AUTO 0 >>> #define LP55XX_CLOCK_INT 1 >>> @@ -23,6 +25,10 @@ struct lp55xx_led_config { >>> u8 chan_nr; >>> u8 led_current; /* mA x10, 0 if led is not connected */ >>> u8 max_current; >>> + int num_colors; >>> + unsigned long available_colors; >>> + u32 channel_color[LED_COLOR_ID_MAX]; >> channel_color array is redundant if you have available_colors flags. > > I will look this again. > > >>> + int grouped_channels[LED_COLOR_ID_MAX]; >>> }; >>> struct lp55xx_predef_pattern { >>> > -- Best regards, Jacek Anaszewski