Multicolor Framework v12

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 <[email protected]>

---
 .../ABI/testing/sysfs-class-led-multicolor    |  36 +++
 .../bindings/leds/leds-class-multicolor.txt   |   4 +-
 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           | 271 ++++++++++++++++++
 include/linux/led-class-multicolor.h          | 143 +++++++++
 8 files changed, 560 insertions(+), 2 deletions(-)
 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

Documentation/ABI/testing/sysfs-class-led-multicolor new file +36 -0

new file mode 100644
index 000000000000..3d1f9d726c70
@@ -0,0 +1,36 @@
+What:		/sys/class/leds/<led>/brightness
+Date:		Sept 2019
+KernelVersion:	5.5
+Contact:	Dan Murphy <[email protected]>
+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 for each grouped LED 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 <[email protected]>
+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 <[email protected]>
+Description:	read only
+		Maximum intensity level for the LED color.

Documentation/devicetree/bindings/leds/leds-class-multicolor.txt +1 -1

index 8619c9bf1811..4b1bd82f2a42 100644
@@ -10,7 +10,7 @@ The nodes and properties defined in this document are unique to the multicolor
 LED class.  Common LED nodes and properties are inherited from the common.txt
 within this documentation directory.
 
-Required LED Child properties:
+Required LED child properties:
 	- color : For multicolor LED support this property should be defined as
 		  LED_COLOR_ID_MULTI and further definition can be found in
 		  include/linux/leds/common.h.

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 <[email protected]>

---
 drivers/leds/Kconfig       |  11 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-lp50xx.c | 799 +++++++++++++++++++++++++++++++++++++
 3 files changed, 811 insertions(+)
 create mode 100644 drivers/leds/leds-lp50xx.c

drivers/leds/Kconfig +11 -0

index a1ede89afc9e..fb614a6b9afa 100644
@@ -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

drivers/leds/Makefile +1 -0

index 841038cfe35b..7a208a0f7b84 100644
@@ -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

drivers/leds/leds-lp50xx.c +0 -0

drivers/leds/leds-lp50xx.c +10 -0

+		fwnode_for_each_child_node(child, led_node) {
+			ret = fwnode_property_read_u32(led_node, "color",
+						       &color_id);
+			if (ret)
+				dev_err(&priv->client->dev,
+				"Cannot read color\n");
+
+			set_bit(color_id, &led->mc_cdev.available_colors);
+			num_colors++;
+

drivers/leds/leds-lp50xx.c +13 -0

+		if (ret) {
+			dev_err(&priv->client->dev, "led register err: %d\n",
+				ret);
+			fwnode_handle_put(child);
+			goto child_out;
+		}
+		i++;
+	}
+
+child_out:
+	return ret;
+}
+

Hi Dan,

I love your patch! Yet something to improve:

[auto build test ERROR on j.anaszewski-leds/for-next]
[cannot apply to v5.4-rc3 next-20191011]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/Multicolor-Framework-v12/20191014-103058
base:   https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git for-next
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 7.4.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sparc64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All errors (new ones prefixed by >>):

Quoted 1 line

Quoted 1 line

drivers//leds/leds-lp50xx.c:360:27: error: initializer element is not constant

      .lp50xx_regmap_config = lp5012_regmap_config,
                              ^~~~~~~~~~~~~~~~~~~~
   drivers//leds/leds-lp50xx.c:360:27: note: (near initialization for 'lp50xx_chip_info_tbl[0].lp50xx_regmap_config')
   drivers//leds/leds-lp50xx.c:371:27: error: initializer element is not constant
      .lp50xx_regmap_config = lp5012_regmap_config,
                              ^~~~~~~~~~~~~~~~~~~~
   drivers//leds/leds-lp50xx.c:371:27: note: (near initialization for 'lp50xx_chip_info_tbl[1].lp50xx_regmap_config')
   drivers//leds/leds-lp50xx.c:382:27: error: initializer element is not constant
      .lp50xx_regmap_config = lp5024_regmap_config,
                              ^~~~~~~~~~~~~~~~~~~~
   drivers//leds/leds-lp50xx.c:382:27: note: (near initialization for 'lp50xx_chip_info_tbl[2].lp50xx_regmap_config')
   drivers//leds/leds-lp50xx.c:393:27: error: initializer element is not constant
      .lp50xx_regmap_config = lp5024_regmap_config,
                              ^~~~~~~~~~~~~~~~~~~~
   drivers//leds/leds-lp50xx.c:393:27: note: (near initialization for 'lp50xx_chip_info_tbl[3].lp50xx_regmap_config')
   drivers//leds/leds-lp50xx.c:404:27: error: initializer element is not constant
      .lp50xx_regmap_config = lp5036_regmap_config,
                              ^~~~~~~~~~~~~~~~~~~~
   drivers//leds/leds-lp50xx.c:404:27: note: (near initialization for 'lp50xx_chip_info_tbl[4].lp50xx_regmap_config')
   drivers//leds/leds-lp50xx.c:415:27: error: initializer element is not constant
      .lp50xx_regmap_config = lp5036_regmap_config,
                              ^~~~~~~~~~~~~~~~~~~~

drivers//leds/leds-lp50xx.c:415:27: note: (near initialization for 'lp50xx_chip_info_tbl[5].lp50xx_regmap_config')

Dan,

Below compilation errors are occurring also for v14. Did you compile-test it at least?

Update the DT binding to include the properties to use the multicolor framework for the devices that use the LP55xx framework.

Signed-off-by Dan Murphy <[email protected]>

CC Tony Lindgren <[email protected]>

CC "Benoît Cousson" <[email protected]>

CC Linus Walleij <[email protected]>

CC Shawn Guo <[email protected]>

CC Sascha Hauer <[email protected]>

CC Pengutronix Kernel Team <[email protected]>

CC Fabio Estevam <[email protected]>

CC NXP Linux Team <[email protected]>

---
 .../devicetree/bindings/leds/leds-lp55xx.txt  | 149 +++++++++++++++---
 1 file changed, 124 insertions(+), 25 deletions(-)

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +14 -0

index bfe2805c5534..736a2e1538be 100644
@@ -1,6 +1,8 @@
 Binding for TI/National Semiconductor LP55xx LED Drivers
 
 Required properties:
+- #address-cells: 1
+- #size-cells: 0
 - compatible: one of
 	national,lp5521
 	national,lp5523

@@ -14,6 +16,18 @@ Required properties:
 Each child has own specific current settings
 - led-cur: Current setting at each LED channel (mA x10, 0 if LED is not connected)
 - max-cur: Maximun current at each LED channel.
+- reg: Output channel for the LED.  This is zero based channel identifier and
+	the data sheet is a one based channel identifier.
+	reg value to output to LED output number
+	D1 = reg value is 0
+	D2 = reg value is 1
+	D3 = reg value is 2
+	D4 = reg value is 3
+	D5 = reg value is 4
+	D6 = reg value is 5
+	D7 = reg value is 6
+	D8 = reg value is 7
+	D9 = reg value is 8
 
 Optional properties:
 - enable-gpio: GPIO attached to the chip's enable pin

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +5 -2

+		reg = <0>;
 		led-cur = /bits/ 8 <0x2f>;
 		max-cur = /bits/ 8 <0x5f>;
 		linux,default-trigger = "heartbeat";
 	};
 
-	chan1 {
+	chan@1 {
+		reg = <1>;
 		led-cur = /bits/ 8 <0x2f>;
 		max-cur = /bits/ 8 <0x5f>;
 	};
 
-	chan2 {
+	chan@2 {
+		reg = <2>;
 		led-cur = /bits/ 8 <0x2f>;
 		max-cur = /bits/ 8 <0x5f>;
 	};

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +20 -9

+	#address-cells = <1>;
+	#size-cells = <0>;
 	compatible = "national,lp5523";
 	reg = <0x32>;
 	clock-mode = /bits/ 8 <1>;
 
-	chan0 {
+	chan@0 {
+		reg = <0>;
 		chan-name = "d1";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan1 {
+	chan@1 {
+		reg = <1>;
 		chan-name = "d2";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan2 {
+	chan@2 {
+		reg = <2>;
 		chan-name = "d3";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan3 {
+	chan@3 {
+		reg = <3>;
 		chan-name = "d4";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan4 {
+	chan@4 {
+		reg = <4>;
 		chan-name = "d5";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan5 {
+	chan@5 {
+		reg = <5>;
 		chan-name = "d6";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan6 {
+	chan@6 {
+		reg = <6>;
 		chan-name = "d7";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan7 {
+	chan@7 {
+		reg = <7>;
 		chan-name = "d8";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan8 {
+	chan@8 {
+		reg = <8>;
 		chan-name = "d9";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +10 -4

+	#address-cells = <1>;
+	#size-cells = <0>;
 	compatible = "ti,lp5562";
 	reg = <0x30>;
 	clock-mode = /bits/8 <2>;
 
-	chan0 {
+	chan@0 {
+		reg = <0>;
 		chan-name = "R";
 		led-cur = /bits/ 8 <0x20>;
 		max-cur = /bits/ 8 <0x60>;
 	};
 
-	chan1 {
+	chan@1 {
+		reg = <1>;
 		chan-name = "G";
 		led-cur = /bits/ 8 <0x20>;
 		max-cur = /bits/ 8 <0x60>;
 	};
 
-	chan2 {
+	chan@2 {
+		reg = <2>;
 		chan-name = "B";
 		led-cur = /bits/ 8 <0x20>;
 		max-cur = /bits/ 8 <0x60>;
 	};
 
-	chan3 {
+	chan@3 {
+		reg = <3>;
 		chan-name = "W";
 		led-cur = /bits/ 8 <0x20>;
 		max-cur = /bits/ 8 <0x60>;

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +26 -9

+	#address-cells = <1>;
+	#size-cells = <0>;
 	compatible = "ti,lp8501";
 	reg = <0x32>;
 	clock-mode = /bits/ 8 <2>;
 	pwr-sel = /bits/ 8 <3>;	/* D1~9 connected to VOUT */
 
-	chan0 {
+	chan@0 {
+		reg = <0>;
 		chan-name = "d1";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan1 {
+	chan@1 {
+		reg = <1>;
 		chan-name = "d2";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan2 {
+	chan@2 {
+		reg = <2>;
 		chan-name = "d3";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan3 {
+	chan@3 {
+		reg = <3>;
 		chan-name = "d4";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan4 {
+	chan@4 {
+		reg = <4>;
 		chan-name = "d5";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan5 {
+	chan@5 {
+		reg = <5>;
 		chan-name = "d6";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan6 {
+	chan@6 {
+		reg = <6>;
 		chan-name = "d7";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan7 {
+	chan@7 {
+		reg = <7>;
 		chan-name = "d8";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 
-	chan8 {
+	chan@8 {
+		reg = <8>;
 		chan-name = "d9";
 		led-cur = /bits/ 8 <0x14>;
 		max-cur = /bits/ 8 <0x20>;
 	};
 };
+
+Multicolor Framework Support
+In addition to the nodes and properties defined above for device support the
+properties below are needed for multicolor framework support as defined in
+Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
+

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +36 -0

+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "national,lp5523";
+	reg = <0x32>;
+	clock-mode = /bits/ 8 <0>; /* LP55XX_CLOCK_AUTO */
+
+	multi-led@2 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <2>;
+		color = <LED_COLOR_ID_MULTI>;
+		function = LED_FUNCTION_STANDBY;
+		linux,default-trigger = "heartbeat";
+
+		led@0 {
+			led-cur = /bits/ 8 <50>;
+			max-cur = /bits/ 8 <100>;
+			reg = <0x0>;
+			color = <LED_COLOR_ID_GREEN>;
+		};
+
+		led@1 {
+			led-cur = /bits/ 8 <50>;
+			max-cur = /bits/ 8 <100>;
+			reg = <0x1>;
+			color = <LED_COLOR_ID_BLUE>;
+		};
+
+		led@6 {
+			led-cur = /bits/ 8 <50>;
+			max-cur = /bits/ 8 <100>;
+			reg = <0x6>;
+			color = <LED_COLOR_ID_RED>;
+		};
+	};
+};

Dan,

Quoted 52 lines · Dan Murphy

Update the DT binding to include the properties to use the multicolor framework for the devices that use the LP55xx framework.

Signed-off-by Dan Murphy <[email protected]>

CC Tony Lindgren <[email protected]>

CC "Benoît Cousson" <[email protected]>

CC Linus Walleij <[email protected]>

CC Shawn Guo <[email protected]>

CC Sascha Hauer <[email protected]>

CC Pengutronix Kernel Team <[email protected]>

CC Fabio Estevam <[email protected]>

CC NXP Linux Team <[email protected]>

---
  .../devicetree/bindings/leds/leds-lp55xx.txt  | 149 +++++++++++++++---
  1 file changed, 124 insertions(+), 25 deletions(-)

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +14 -0

index bfe2805c5534..736a2e1538be 100644
@@ -1,6 +1,8 @@
  Binding for TI/National Semiconductor LP55xx LED Drivers
 
  Required properties:
+- #address-cells: 1
+- #size-cells: 0
  - compatible: one of
  	national,lp5521
  	national,lp5523

@@ -14,6 +16,18 @@ Required properties:
  Each child has own specific current settings
  - led-cur: Current setting at each LED channel (mA x10, 0 if LED is not connected)
  - max-cur: Maximun current at each LED channel.
+- reg: Output channel for the LED.  This is zero based channel identifier and
+	the data sheet is a one based channel identifier.
+	reg value to output to LED output number
+	D1 = reg value is 0
+	D2 = reg value is 1
+	D3 = reg value is 2
+	D4 = reg value is 3
+	D5 = reg value is 4
+	D6 = reg value is 5
+	D7 = reg value is 6
+	D8 = reg value is 7
+	D9 = reg value is 8
 
  Optional properties:
  - enable-gpio: GPIO attached to the chip's enable pin

@@ -35,23 +49,28 @@ example 1) LP5521
  on channel 0.
 

Documentation/devicetree/bindings/leds/leds-lp55xx.txt example 1) LP5521 · line 51

Quoted 1 line

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +0 -0

  lp5521@32 {

While at it let's fix node prefix to led-controller. It will not break anyone and yet it will adapt the node naming to what is now recommended.

Documentation/devicetree/bindings/leds/leds-lp55xx.txt example 1) LP5521 · line 59

Quoted 10 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +3 -1

+	#address-cells = <1>;
+	#size-cells = <0>;
  	compatible = "national,lp5521";
  	reg = <0x32>;
  	label = "lp5521_pri";
  	clock-mode = /bits/ 8 <2>;
 
-	chan0 {
+	chan@0 {

Similarly in case of every child node:

s/chan/led/

Quoted 20 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +5 -2

+		reg = <0>;
  		led-cur = /bits/ 8 <0x2f>;
  		max-cur = /bits/ 8 <0x5f>;
  		linux,default-trigger = "heartbeat";
  	};
 
-	chan1 {
+	chan@1 {
+		reg = <1>;
  		led-cur = /bits/ 8 <0x2f>;
  		max-cur = /bits/ 8 <0x5f>;
  	};
 
-	chan2 {
+	chan@2 {
+		reg = <2>;
  		led-cur = /bits/ 8 <0x2f>;
  		max-cur = /bits/ 8 <0x5f>;
  	};

Documentation/devicetree/bindings/leds/leds-lp55xx.txt ASEL1 ASEL0 Address · line 91

Quoted 3 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +0 -0

@@ -70,59 +89,70 @@ ASEL1    ASEL0    Address
   VEN      VEN       35h
 
  lp5523@32 {

s/lp5523/led-controller/

Quoted 77 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +20 -9

+	#address-cells = <1>;
+	#size-cells = <0>;
  	compatible = "national,lp5523";
  	reg = <0x32>;
  	clock-mode = /bits/ 8 <1>;
 
-	chan0 {
+	chan@0 {
+		reg = <0>;
  		chan-name = "d1";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan1 {
+	chan@1 {
+		reg = <1>;
  		chan-name = "d2";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan2 {
+	chan@2 {
+		reg = <2>;
  		chan-name = "d3";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan3 {
+	chan@3 {
+		reg = <3>;
  		chan-name = "d4";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan4 {
+	chan@4 {
+		reg = <4>;
  		chan-name = "d5";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan5 {
+	chan@5 {
+		reg = <5>;
  		chan-name = "d6";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan6 {
+	chan@6 {
+		reg = <6>;
  		chan-name = "d7";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan7 {
+	chan@7 {
+		reg = <7>;
  		chan-name = "d8";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan8 {
+	chan@8 {
+		reg = <8>;
  		chan-name = "d9";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;

Documentation/devicetree/bindings/leds/leds-lp55xx.txt example 3) LP5562 · line 165

Quoted 3 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +0 -0

@@ -133,29 +163,35 @@ example 3) LP5562
  4 channels are defined.
 
  lp5562@30 {

s/lp5562/led-controller/

Quoted 37 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +10 -4

+	#address-cells = <1>;
+	#size-cells = <0>;
  	compatible = "ti,lp5562";
  	reg = <0x30>;
  	clock-mode = /bits/8 <2>;
 
-	chan0 {
+	chan@0 {
+		reg = <0>;
  		chan-name = "R";
  		led-cur = /bits/ 8 <0x20>;
  		max-cur = /bits/ 8 <0x60>;
  	};
 
-	chan1 {
+	chan@1 {
+		reg = <1>;
  		chan-name = "G";
  		led-cur = /bits/ 8 <0x20>;
  		max-cur = /bits/ 8 <0x60>;
  	};
 
-	chan2 {
+	chan@2 {
+		reg = <2>;
  		chan-name = "B";
  		led-cur = /bits/ 8 <0x20>;
  		max-cur = /bits/ 8 <0x60>;
  	};
 
-	chan3 {
+	chan@3 {
+		reg = <3>;
  		chan-name = "W";
  		led-cur = /bits/ 8 <0x20>;
  		max-cur = /bits/ 8 <0x60>;

Documentation/devicetree/bindings/leds/leds-lp55xx.txt example 4) LP8501 · line 205

Quoted 3 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +0 -0

@@ -167,62 +203,125 @@ example 4) LP8501
  Others are same as LP5523.
 
  lp8501@32 {

s/lp8501/led-controller/

Quoted 85 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +26 -9

+	#address-cells = <1>;
+	#size-cells = <0>;
  	compatible = "ti,lp8501";
  	reg = <0x32>;
  	clock-mode = /bits/ 8 <2>;
  	pwr-sel = /bits/ 8 <3>;	/* D1~9 connected to VOUT */
 
-	chan0 {
+	chan@0 {
+		reg = <0>;
  		chan-name = "d1";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan1 {
+	chan@1 {
+		reg = <1>;
  		chan-name = "d2";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan2 {
+	chan@2 {
+		reg = <2>;
  		chan-name = "d3";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan3 {
+	chan@3 {
+		reg = <3>;
  		chan-name = "d4";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan4 {
+	chan@4 {
+		reg = <4>;
  		chan-name = "d5";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan5 {
+	chan@5 {
+		reg = <5>;
  		chan-name = "d6";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan6 {
+	chan@6 {
+		reg = <6>;
  		chan-name = "d7";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan7 {
+	chan@7 {
+		reg = <7>;
  		chan-name = "d8";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
 
-	chan8 {
+	chan@8 {
+		reg = <8>;
  		chan-name = "d9";
  		led-cur = /bits/ 8 <0x14>;
  		max-cur = /bits/ 8 <0x20>;
  	};
  };
+
+Multicolor Framework Support
+In addition to the nodes and properties defined above for device support the
+properties below are needed for multicolor framework support as defined in
+Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
+

Documentation/devicetree/bindings/leds/leds-lp55xx.txt example 4) LP8501 · line 291

Quoted 10 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +10 -0

+Required child properties for multicolor framework
+	- color : Must be LED_COLOR_ID_MULTI
+	- function : see Documentation/devicetree/bindings/leds/common.txt
+
+Required grandchildren properties
+	- reg : This is the LED output of the device
+	- color : see Documentation/devicetree/bindings/leds/common.txt
+
+Multicolor LED example:
+lp5523: lp5523@32 {

Do you really need "lp5523:" node label ?

Besides:

s/lp5523@32/led-controller@32/

Jacek

Quoted 51 lines · Jacek Anaszewski

Dan,

Quoted 48 lines · Dan Murphy

Update the DT binding to include the properties to use the multicolor framework for the devices that use the LP55xx framework.

Signed-off-by Dan Murphy <[email protected]>

CC Tony Lindgren <[email protected]>

CC "Benoît Cousson" <[email protected]>

CC Linus Walleij <[email protected]>

CC Shawn Guo <[email protected]>

CC Sascha Hauer <[email protected]>

CC Pengutronix Kernel Team <[email protected]>

CC Fabio Estevam <[email protected]>

CC NXP Linux Team <[email protected]>

---
  .../devicetree/bindings/leds/leds-lp55xx.txt  | 149 +++++++++++++++---

  1 file changed, 124 insertions(+), 25 deletions(-)

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +0 -0

index bfe2805c5534..736a2e1538be 100644
@@ -1,6 +1,8 @@

  Binding for TI/National Semiconductor LP55xx LED Drivers Required properties:

+- #address-cells: 1
+- #size-cells: 0

  - compatible: one of
      national,lp5521
      national,lp5523

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +0 -0

@@ -14,6 +16,18 @@ Required properties:

  Each child has own specific current settings
  - led-cur: Current setting at each LED channel (mA x10, 0 if LED is not connected)
  - max-cur: Maximun current at each LED channel.
+- reg: Output channel for the LED.  This is zero based channel identifier and
+    the data sheet is a one based channel identifier.
+    reg value to output to LED output number
+    D1 = reg value is 0
+    D2 = reg value is 1
+    D3 = reg value is 2
+    D4 = reg value is 3
+    D5 = reg value is 4
+    D6 = reg value is 5
+    D7 = reg value is 6
+    D8 = reg value is 7
+    D9 = reg value is 8

Quoted 8 lines

Quoted 5 lines

    Optional properties:
  - enable-gpio: GPIO attached to the chip's enable pin

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +0 -0

@@ -35,23 +49,28 @@ example 1) LP5521

  on channel 0.
    lp5521@32 {

While at it let's fix node prefix to led-controller. It will not break anyone and yet it will adapt the node naming to what is now recommended.

I would love to do that but honestly I prefer to fix that in a different patch series and not add more changes to this patch review.

Quoted 15 lines

Quoted 8 lines

+    #address-cells = <1>;
+    #size-cells = <0>;
      compatible = "national,lp5521";
      reg = <0x32>;
      label = "lp5521_pri";
      clock-mode = /bits/ 8 <2>;
  -    chan0 {
+    chan@0 {

Similarly in case of every child node:

Documentation/devicetree/bindings/leds/leds-lp55xx.txt example 4) LP8501 · line 291

Quoted 2 lines

Documentation/devicetree/bindings/leds/leds-lp55xx.txt +2 -0

+Multicolor LED example:
+lp5523: lp5523@32 {

Do you really need "lp5523:" node label ?

Quoted 3 lines

Besides:

s/lp5523@32/led-controller@32/

This change I will make since I added this.

Add multicolor framework support for the lp55xx family.

Signed-off-by Dan Murphy <[email protected]>

---
 drivers/leds/Kconfig                      |   1 +
 drivers/leds/leds-lp55xx-common.c         | 176 +++++++++++++++++++---
 drivers/leds/leds-lp55xx-common.h         |   9 ++
 include/linux/platform_data/leds-lp55xx.h |   7 +
 4 files changed, 169 insertions(+), 24 deletions(-)

drivers/leds/Kconfig +1 -0

index fb614a6b9afa..5706bf8d8bd1 100644
@@ -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 OF
 	select FW_LOADER
 	select FW_LOADER_USER_HELPER
 	help

drivers/leds/leds-lp55xx-common.c +68 -8

index 44ced02b49f9..1417df6df8a7 100644
@@ -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) {
+			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->color_intensity_fn)
+			return -EINVAL;
+
+		led_mc_calc_color_components(&led->mc_cdev, brightness,
+					     color_component);
+
+		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->color_intensity_fn(led);
+		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 +187,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 +199,34 @@ 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->color_component,
+		       pdata->led_config[chan].color_component,
+		       sizeof(led->color_component));
+	} 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 +234,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].name) {

drivers/leds/leds-lp55xx-common.c +80 -8

 	for (i = 0; i < num_channels; i++) {
 
 		/* do not initialize channels that are not connected */

@@ -538,6 +594,82 @@ 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,
+				    int color_num)
+{
+	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].color_component[color_num].output_num =
+								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;
+	int num_colors = 0;
+	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,
+						       num_colors);
+			if (ret)
+				return ret;
+
+			ret = of_property_read_u32(child, "color", &color_id);
+			if (ret)
+				return ret;
+
+			cfg[child_number].color_component[num_colors].color_id =
+						color_id;
+			set_bit(color_id, &cfg[child_number].available_colors);
+			num_colors++;
+		}
+
+		cfg[child_number].num_colors = num_colors;
+	} else {
+		return lp5xx_parse_common_child(np, cfg, child_number, false,
+						num_colors);
+	}
+
+	return 0;
+}
+
 struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
 						      struct device_node *np)
 {

@@ -546,6 +678,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 +698,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++;
 	}
 

drivers/leds/leds-lp55xx-common.h +2 -0

index 783ed5103ce5..75d57fb835c3 100644
@@ -12,6 +12,8 @@
 #ifndef _LEDS_LP55XX_COMMON_H
 #define _LEDS_LP55XX_COMMON_H
 
+#include <linux/led-class-multicolor.h>
+
 enum lp55xx_engine_index {
 	LP55XX_ENGINE_INVALID,
 	LP55XX_ENGINE_1,

drivers/leds/leds-lp55xx-common.h +5 -0

+
 	/* current setting function */
 	void (*set_led_current) (struct lp55xx_led *led, u8 led_current);
 

@@ -159,6 +164,8 @@ struct lp55xx_chip {
  * struct lp55xx_led
  * @chan_nr         : Channel number
  * @cdev            : LED class device
+ * @mc_cdev	    : Multi color class device
+ * @color_component : Multi color LED map information
  * @led_current     : Current setting at each led channel
  * @max_current     : Maximun current at each led channel
  * @brightness      : Brightness value

@@ -167,6 +174,8 @@ struct lp55xx_chip {
 struct lp55xx_led {
 	int chan_nr;
 	struct led_classdev cdev;
+	struct led_classdev_mc mc_cdev;
+	struct led_mc_color_conversion color_component[LP55XX_MAX_GROUPED_CHAN];
 	u8 led_current;
 	u8 max_current;
 	u8 brightness;

include/linux/platform_data/leds-lp55xx.h +7 -0

index 96a787100fda..0f9b88019c18 100644
@@ -12,17 +12,24 @@
 #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
 #define LP55XX_CLOCK_EXT	2
 
+#define LP55XX_MAX_GROUPED_CHAN	4
+
 struct lp55xx_led_config {
 	const char *name;
 	const char *default_trigger;
 	u8 chan_nr;
 	u8 led_current; /* mA x10, 0 if led is not connected */
 	u8 max_current;
+	int num_colors;
+	unsigned long available_colors;
+	struct led_mc_color_conversion color_component[LP55XX_MAX_GROUPED_CHAN];
 };
 
 struct lp55xx_predef_pattern {

Dan,

Quoted 134 lines · Dan Murphy

Add multicolor framework support for the lp55xx family.

Signed-off-by Dan Murphy <[email protected]>

---
  drivers/leds/Kconfig                      |   1 +
  drivers/leds/leds-lp55xx-common.c         | 176 +++++++++++++++++++---
  drivers/leds/leds-lp55xx-common.h         |   9 ++
  include/linux/platform_data/leds-lp55xx.h |   7 +
  4 files changed, 169 insertions(+), 24 deletions(-)

drivers/leds/Kconfig +1 -0

index fb614a6b9afa..5706bf8d8bd1 100644
@@ -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 OF
  	select FW_LOADER
  	select FW_LOADER_USER_HELPER
  	help

drivers/leds/leds-lp55xx-common.c +68 -4

index 44ced02b49f9..1417df6df8a7 100644
@@ -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) {
+			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->color_intensity_fn)
+			return -EINVAL;
+
+		led_mc_calc_color_components(&led->mc_cdev, brightness,
+					     color_component);
+
+		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->color_intensity_fn(led);
+		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 +187,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 +199,34 @@ 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->color_component,
+		       pdata->led_config[chan].color_component,
+		       sizeof(led->color_component));
+	} 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 +234,11 @@ static int lp55xx_init_led(struct lp55xx_led *led,
  		return -EINVAL;
  	}
 

Quoted 4 lines

drivers/leds/leds-lp55xx-common.c +0 -4

-	led->cdev.brightness_set_blocking = lp55xx_set_brightness;
-	led->cdev.groups = lp55xx_led_groups;
-
-	if (pdata->led_config[chan].name) {

drivers/leds/leds-lp55xx-common.c static int lp55xx_init_led(struct lp55xx_led *led, · line 240

Quoted 10 lines

drivers/leds/leds-lp55xx-common.c +4 -6

-		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)
+		ret = led_classdev_multicolor_register(dev, &led->mc_cdev);
+	else
+		ret = led_classdev_register(dev, &led->cdev);

Why not devm versions?

drivers/leds/leds-lp55xx-common.c int lp55xx_register_leds(struct lp55xx_led *led, struct lp55xx_chip *chip) · line 469

Quoted 10 lines

drivers/leds/leds-lp55xx-common.c +0 -2

-	ret = led_classdev_register(dev, &led->cdev);
  	if (ret) {
  		dev_err(dev, "led register err: %d\n", ret);
  		return ret;

@@ -466,7 +523,6 @@ int lp55xx_register_leds(struct lp55xx_led *led, struct lp55xx_chip *chip)
  		dev_err(&chip->cl->dev, "empty brightness configuration\n");
  		return -EINVAL;
  	}
-

This empty line removal was not intended I suppose.

Quoted 130 lines

drivers/leds/leds-lp55xx-common.c +80 -8

  	for (i = 0; i < num_channels; i++) {
 
  		/* do not initialize channels that are not connected */

@@ -538,6 +594,82 @@ 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,
+				    int color_num)
+{
+	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].color_component[color_num].output_num =
+								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;
+	int num_colors = 0;
+	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,
+						       num_colors);
+			if (ret)
+				return ret;
+
+			ret = of_property_read_u32(child, "color", &color_id);
+			if (ret)
+				return ret;
+
+			cfg[child_number].color_component[num_colors].color_id =
+						color_id;
+			set_bit(color_id, &cfg[child_number].available_colors);
+			num_colors++;
+		}
+
+		cfg[child_number].num_colors = num_colors;
+	} else {
+		return lp5xx_parse_common_child(np, cfg, child_number, false,
+						num_colors);
+	}
+
+	return 0;
+}
+
  struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
  						      struct device_node *np)
  {

@@ -546,6 +678,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 +698,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++;
  	}
 

drivers/leds/leds-lp55xx-common.h +2 -0

index 783ed5103ce5..75d57fb835c3 100644
@@ -12,6 +12,8 @@
  #ifndef _LEDS_LP55XX_COMMON_H
  #define _LEDS_LP55XX_COMMON_H
 
+#include <linux/led-class-multicolor.h>
+
  enum lp55xx_engine_index {
  	LP55XX_ENGINE_INVALID,
  	LP55XX_ENGINE_1,

@@ -109,6 +111,9 @@ struct lp55xx_device_config {
  	/* access brightness register */
  	int (*brightness_fn)(struct lp55xx_led *led);
 

drivers/leds/leds-lp55xx-common.h struct lp55xx_device_config { · line 115

Quoted 2 lines

drivers/leds/leds-lp55xx-common.h +2 -0

+	/* access specific brightness register */
+	int (*color_intensity_fn)(struct lp55xx_led *led);

Wouldn't multicolor_brightness_fn be more accurate now ?

Jacek

Quoted 142 lines · Jacek Anaszewski

Dan,

Quoted 139 lines · Dan Murphy

Add multicolor framework support for the lp55xx family.

Signed-off-by Dan Murphy <[email protected]>

---
  drivers/leds/Kconfig                      |   1 + drivers/leds/leds-lp55xx-common.c         | 176 +++++++++++++++++++--- drivers/leds/leds-lp55xx-common.h         |   9 ++ include/linux/platform_data/leds-lp55xx.h |   7 +

  4 files changed, 169 insertions(+), 24 deletions(-)

drivers/leds/Kconfig +0 -0

index fb614a6b9afa..5706bf8d8bd1 100644
@@ -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 OF
      select FW_LOADER
      select FW_LOADER_USER_HELPER
      help

drivers/leds/leds-lp55xx-common.c +66 -9

index 44ced02b49f9..1417df6df8a7 100644
@@ -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) {
+            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->color_intensity_fn)
+            return -EINVAL;
+
+        led_mc_calc_color_components(&led->mc_cdev, brightness,
+                         color_component);
+
+        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->color_intensity_fn(led);
+        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 +187,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 +199,34 @@ 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->color_component,
+               pdata->led_config[chan].color_component,
+               sizeof(led->color_component));
+    } 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 +234,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].name) {
-        led->cdev.name = pdata->led_config[chan].name;
-    } else {
-        snprintf(name, sizeof(name), "%s:channel%d",

Quoted 8 lines

drivers/leds/leds-lp55xx-common.c static int lp55xx_init_led(struct lp55xx_led *led, · line 240

Quoted 7 lines

drivers/leds/leds-lp55xx-common.c +4 -3

-            pdata->label ? : chip->cl->name, chan);
-        led->cdev.name = name;
-    }
+    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);

Why not devm versions?

I will change the multicolor but not the led_classdev

Quoted 12 lines

drivers/leds/leds-lp55xx-common.c int lp55xx_register_leds(struct lp55xx_led *led, · line 470

Quoted 11 lines

drivers/leds/leds-lp55xx-common.c +0 -2

-    ret = led_classdev_register(dev, &led->cdev);
       if (ret) {
           dev_err(dev, "led register err: %d\n", ret);
           return ret;

@@ -466,7 +523,6 @@ int lp55xx_register_leds(struct lp55xx_led *led,
 struct lp55xx_chip *chip)
           dev_err(&chip->cl->dev, "empty brightness configuration\n");
           return -EINVAL;
       }
-

This empty line removal was not intended I suppose.

I can put it back

Quoted 123 lines

Quoted 123 lines

drivers/leds/leds-lp55xx-common.c +75 -0

       for (i = 0; i < num_channels; i++) {
             /* do not initialize channels that are not connected */

@@ -538,6 +594,82 @@ 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,
+                    int color_num)
+{
+    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].color_component[color_num].output_num =
+                                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;
+    int num_colors = 0;
+    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,
+                               num_colors);
+            if (ret)
+                return ret;
+
+            ret = of_property_read_u32(child, "color", &color_id);
+            if (ret)
+                return ret;
+
+            cfg[child_number].color_component[num_colors].color_id =
+                        color_id;
+            set_bit(color_id, &cfg[child_number].available_colors);
+            num_colors++;
+        }
+
+        cfg[child_number].num_colors = num_colors;
+    } else {
+        return lp5xx_parse_common_child(np, cfg, child_number, false,
+                        num_colors);
+    }
+
+    return 0;
+}
+

  struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
                                struct device_node *np)
  {

drivers/leds/leds-lp55xx-common.c +4 -8

@@ -546,6 +678,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 +698,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..75d57fb835c3 100644

drivers/leds/leds-lp55xx-common.h +0 -0

@@ -12,6 +12,8 @@
   #ifndef _LEDS_LP55XX_COMMON_H
   #define _LEDS_LP55XX_COMMON_H

  +#include <linux/led-class-multicolor.h>
+
  enum lp55xx_engine_index {

Quoted 8 lines

Quoted 7 lines

      LP55XX_ENGINE_INVALID,
      LP55XX_ENGINE_1,

drivers/leds/leds-lp55xx-common.h +0 -0

@@ -109,6 +111,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);

Wouldn't multicolor_brightness_fn be more accurate now ?

So just change the name to multicolor_brightness_fn?

Hello

Minor changes per review comments.
https://lore.kernel.org/patchwork/project/lkml/list/?series=413385

Rebased the series on top of Pavel's next branch.

Multicolor changes:
Rename led_mc_calc_brightness to led_mc_calc_color_components Updated the binding example for the function from STATUS to CHARGING

LP50xx changes:
None

LP55xx changes:
Changed color_component array allocation from COLOR_MAX_ID to MAX_GROUPED_CHANNELS Updated MC API to call led_mc_calc_color_components

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    |  36 +
 .../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           | 271 ++++++
 drivers/leds/led-core.c                       |   1 +
 drivers/leds/leds-lp50xx.c                    | 799 ++++++++++++++++++
 drivers/leds/leds-lp5521.c                    |  20 +
 drivers/leds/leds-lp5523.c                    |  39 +-
 drivers/leds/leds-lp55xx-common.c             | 198 ++++-
 drivers/leds/leds-lp55xx-common.h             |   9 +
 include/dt-bindings/leds/common.h             |   3 +-
 include/linux/led-class-multicolor.h          | 143 ++++
 include/linux/platform_data/leds-lp55xx.h     |   7 +
 21 files changed, 2020 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,

Quoted 5 lines · Dan Murphy

Hello

Minor changes per review comments.
https://lore.kernel.org/patchwork/project/lkml/list/?series=413385

Quoted 5 lines

Rebased the series on top of Pavel's next branch.

Multicolor changes:
Rename led_mc_calc_brightness to led_mc_calc_color_components Updated the binding example for the function from STATUS to CHARGING

I see STATUS is still there, as well ass "Child".