[PATCH v3 0/9] Multicolor Framework update

[PATCH v3 0/9] Multicolor Framework update

[Dan Murphy]

Hello

In this series I have done quite a bit of rework and since there was not to
many review comments we can pretty much treat this as unreviewed.

I have removed the brightness-model definition from this series as it is an
enhancement to the multi color framework but is not required in the initial
submission.  And this seems to be the biggest source of contention in the patch
set.  So the idea is to get the base MC FW in place and let developers play with
it.

I have converted the LP5523 to use the MC framework and will update a couple
more drivers once I have the test harnesses to verify the implementation.

The biggest change to this series is that the parent brightness file now controls
all the color LEDs brightness within the directory.

I have written a script that writes brightness values to the colors and then
proceeds to write the parent brightness from 0->255 and then back to 0.
Basically pulsing the LED group.  I found that the color is uniform across the
brightness ranges.

Dan

Dan Murphy (9):
  leds: multicolor: Add sysfs interface definition
  dt: bindings: Add multicolor class dt bindings documention
  documention: leds: Add multicolor class documentation
  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
  leds: Update the lp55xx to use the multi color framework

 .../ABI/testing/sysfs-class-led-multicolor    |  57 ++
 .../bindings/leds/leds-class-multicolor.txt   |  97 +++
 .../devicetree/bindings/leds/leds-lp50xx.txt  | 142 ++++
 Documentation/leds/leds-class-multicolor.txt  |  99 +++
 drivers/leds/Kconfig                          |  17 +
 drivers/leds/Makefile                         |   2 +
 drivers/leds/led-class-multicolor.c           | 421 +++++++++++
 drivers/leds/led-core.c                       |   1 +
 drivers/leds/leds-lp50xx.c                    | 714 ++++++++++++++++++
 drivers/leds/leds-lp5523.c                    |  13 +
 drivers/leds/leds-lp55xx-common.c             | 133 +++-
 drivers/leds/leds-lp55xx-common.h             |  10 +
 include/dt-bindings/leds/common.h             |   3 +-
 include/linux/led-class-multicolor.h          |  95 +++
 include/linux/platform_data/leds-lp55xx.h     |   5 +
 15 files changed, 1789 insertions(+), 20 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.txt
 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

-- 
2.21.0.5.gaeb582a983

[PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Dan Murphy]

Add a documentation of LED Multicolor LED class specific
sysfs attributes.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../ABI/testing/sysfs-class-led-multicolor    | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor

diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
new file mode 100644
index 000000000000..2f102ede258b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
@@ -0,0 +1,57 @@
+What:		/sys/class/leds/<led>/colors/sync_enable
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	read/write
+		Writing a 1 to this file will enable the synchronization of all
+		the defined color LEDs within the LED node.  Brightness values
+		for each LED will be stored and written when sync is set to 1.
+		Writing a 0 to this file will disable syncing and allow
+		individual control of the LEDs brightness settings.
+
+What:		/sys/class/leds/<led>/colors/sync
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	write only
+		Writing a 1 to this file while sync_enable is set to 1 will
+		write the current brightness values to all defined LEDs within
+		the LED node.  All LEDs defined will be configured based
+		on the brightness that has been requested.
+
+		If sync_enable is set to 0 then writing a 1 to sync has no
+		affect on the LEDs.
+
+What:		/sys/class/leds/<led>/colors/<led_color>/brightness
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	read/write
+		The led_color directory is dynamically created based on the
+		colors defined by the registrar of the class.
+		The led_color can be but not limited to red, green, blue,
+		white, amber, yellow and violet.  Drivers can also declare a
+		LED color for presentation.  There is one directory per color
+		presented.  The brightness file is created under each
+		led_color directory and controls the individual LED color
+		setting.
+
+		If sync is enabled then	writing the brightness value of the LED
+		is deferred until a 1 is written to
+		/sys/class/leds/<led>/color/sync.  If syncing is
+		disabled then the LED brightness value will be written
+		immediately to the LED driver.
+
+		The value of the color is from 0 to
+		/sys/class/leds/<led>/colors/<led_color>/max_brightness.
+
+What:		/sys/class/leds/<led>/colors/<led_color>/max_brightness
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Dan Murphy <dmurphy@ti.com>
+Description:	read only
+		Maximum brightness level for the LED color, default is
+		255 (LED_FULL).
+
+		If the LED does not support different brightness levels, this
+		should be 1.
-- 
2.21.0.5.gaeb582a983

[PATCH v3 2/9] dt: bindings: Add multicolor class dt bindings documention

[Dan Murphy]

Add DT bindings for the LEDs multicolor class framework.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../bindings/leds/leds-class-multicolor.txt   | 97 +++++++++++++++++++
 1 file changed, 97 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
new file mode 100644
index 000000000000..e2a2ce3279cb
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
@@ -0,0 +1,97 @@
+* Multicolor LED properties
+
+Multicolor LEDs can consist of a RGB, RGBW or a RGBA LED clusters.  These devices
+can be grouped together and also provide a modeling mechanism so that the
+cluster LEDs can vary in hue and intensity to produce a wide range of colors.
+
+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:
+	- 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.
+
+led-controller@30 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "ti,lp5024";
+	reg = <0x29>;
+
+	multi-led@4 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <4>;
+		color = <LED_COLOR_ID_MULTI>;
+		function = LED_FUNCTION_ACTIVITY;
+
+		led@12 {
+			reg = <12>;
+			color = <LED_COLOR_ID_RED>;
+		};
+
+		led@13 {
+			reg = <13>;
+			color = <LED_COLOR_ID_GREEN>;
+		};
+
+		led@14 {
+			reg = <14>;
+			color = <LED_COLOR_ID_BLUE>;
+		};
+	};
+
+	/* Only support RGB no model defined */
+	multi-led@1 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <1>;
+		color = <LED_COLOR_ID_MULTI>;
+		function = LED_FUNCTION_;
+
+
+		led@3 {
+			reg = <3>;
+			color = <LED_COLOR_ID_RED>;
+		};
+
+		led@4 {
+			reg = <4>;
+			color = <LED_COLOR_ID_GREEN>;
+		};
+
+		led@5 {
+			reg = <5>;
+			color = <LED_COLOR_ID_BLUE>;
+		};
+	};
+
+	multi-led@2 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		color = <LED_COLOR_ID_MULTI>;
+		function = LED_FUNCTION_ACTIVITY;
+		reg = <2>;
+		ti,led-bank = <2 3 5>;
+
+		led@6 {
+			reg = <0x6>;
+			color = <LED_COLOR_ID_RED>;
+			led-sources = <6 9 15>;
+		};
+
+		led@7 {
+			reg = <0x7>;
+			color = <LED_COLOR_ID_GREEN>;
+			led-sources = <7 10 16>;
+		};
+
+		led@8 {
+			reg = <0x8>;
+			color = <LED_COLOR_ID_BLUE>;
+			led-sources = <8 11 17>;
+		};
+	};
+
+};
-- 
2.21.0.5.gaeb582a983

[PATCH v3 3/9] documention: leds: Add multicolor class documentation

[Dan Murphy]

Add the support documentation on the multicolor LED framework.
This document defines the directores and file generated by the
multicolor framework.  It also documents usage.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 Documentation/leds/leds-class-multicolor.txt | 99 ++++++++++++++++++++
 1 file changed, 99 insertions(+)
 create mode 100644 Documentation/leds/leds-class-multicolor.txt

diff --git a/Documentation/leds/leds-class-multicolor.txt b/Documentation/leds/leds-class-multicolor.txt
new file mode 100644
index 000000000000..edbb8139e514
--- /dev/null
+++ b/Documentation/leds/leds-class-multicolor.txt
@@ -0,0 +1,99 @@
+
+Multi Color LED handling under Linux
+=================================================
+
+Author: Dan Murphy <dmurphy@ti.com>
+
+Description
+-----------
+There are varying monochrome LED colors available for application.  These
+LEDs can be used as a single use case LED or can be mixed with other color
+LEDs to produce the full spectrum of color.  Color LEDs that are grouped
+can be presented under a single LED node with individual color control.
+The multicolor class groups these LEDs and allows dynamically setting the value
+of a single LED or setting the brightness values of the LEDs in the group and
+updating the LEDs virtually simultaneously.
+
+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.txt within this
+documentation directory.
+
+Each colored LED is given its own directory.  These directories can be, but not
+limited to red, green, blue, white, amber, yellow and violet.  Under these
+directories the brightness and max_brightness files are presented for each LED.
+
+Under the "colors" directory there are two files created: "sync" and
+"sync_enable". The sync_enable file controls whether the LED brightness
+value is set real time or if the LED brightness value setting is deferred until
+the "sync" file is written.  If sync_enable is set then writing to each LED
+"brightness" file will store the brightness value.  Once the "sync" file is
+written then each LED color defined in the node will write the brightness of
+the LED in the device driver.
+
+If "sync_enable" is not set then writing the brightness value of the LED to the
+device driver is done immediately.  Writing the "sync" file has no affect.
+
+Directory Layout Example
+------------------------
+root:/sys/class/leds/rgb:grouped_leds# ls -lR colors/
+colors/:
+drwxr-xr-x    2 root     root             0 Jun 28 20:21 blue
+drwxr-xr-x    2 root     root             0 Jun 28 20:21 green
+drwxr-xr-x    2 root     root             0 Jun 28 20:21 red
+--w-------    1 root     root          4096 Jun 28 20:21 sync
+-rw-------    1 root     root          4096 Jun 28 20:22 sync_enable
+
+colors/blue:
+-rw-------    1 root     root          4096 Jun 28 20:21 brightness
+-r--------    1 root     root          4096 Jun 28 20:27 max_brightness
+
+colors/green:
+-rw-------    1 root     root          4096 Jun 28 20:22 brightness
+-r--------    1 root     root          4096 Jun 28 20:27 max_brightness
+
+colors/red:
+-rw-------    1 root     root          4096 Jun 28 20:21 brightness
+-r--------    1 root     root          4096 Jun 28 20:27 max_brightness
+
+Example of Writing LEDs with Sync Enabled
+-----------------------------------------
+Below the red, green and blue LEDs are set to corresponding values.  These
+values are stored and not written until the sync file is written.
+
+cd /sys/class/leds/rgb:grouped_leds/colors
+
+echo 1 > sync_enable
+
+echo 100 > red/brightness
+echo 80 > green/brightness
+echo 180 > blue/brightness
+
+* LED device driver has not been updated and the LED states have not changed.
+* Writing the LED brightness files again will only change the stored value and
+* not the device driver value.
+
+echo 1 > sync
+
+* LED device driver has been updated; the LEDs should present the brightness
+* levels that have been set.  Since sync_enable is still enabled writing to the
+* LED brightness files will not change the current brightnesses.
+
+Example of Writing LEDs with Sync Disabled
+------------------------------------------
+Below the values of each LED are written to the device driver immediately upon
+request.
+
+cd /sys/class/leds/rgb:grouped_leds/colors
+
+echo 0 > sync_enable
+
+echo 100 > red/brightness // Red LED should be on with the current brightness
+echo 80 > green/brightness // Green LED should be on with the current brightness
+echo 180 > blue/brightness // Blue LED should be on with the current brightness
+.
+.
+.
+echo 0 > green/brightness // Green LED should be off
-- 
2.21.0.5.gaeb582a983

[PATCH v3 4/9] dt-bindings: leds: Add multicolor ID to the color ID list

[Dan Murphy]

Add a new color ID that is declared as MULTICOLOR as with the
multicolor framework declaring a definitive color is not accurate
as the node can contain multiple colors.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 include/dt-bindings/leds/common.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index d8c8c38700bd..259cea653844 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -65,6 +65,7 @@
 #define LED_COLOR_ID_VIOLET	5
 #define LED_COLOR_ID_YELLOW	6
 #define LED_COLOR_ID_IR		7
-#define LED_COLOR_ID_COUNT	8
+#define LED_COLOR_ID_MULTI	8
+#define LED_COLOR_ID_COUNT	9
 
 #endif /* __DT_BINDINGS_LEDS_H */
-- 
2.21.0.5.gaeb582a983

[PATCH v3 5/9] leds: Add multicolor ID to the color ID list

[Dan Murphy]

Add a new color ID that is declared as MULTICOLOR as with the
multicolor framework declaring a definitive color is not accurate
as the node can contain multiple colors.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/led-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 0ce1f4aeca92..e1e1749b119d 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -38,6 +38,7 @@ const char *led_colors[LED_COLOR_ID_COUNT] = {
 	[LED_COLOR_ID_VIOLET] = "violet",
 	[LED_COLOR_ID_YELLOW] = "yellow",
 	[LED_COLOR_ID_IR] = "ir",
+	[LED_COLOR_ID_MULTI] = "multicolor",
 };
 EXPORT_SYMBOL_GPL(led_colors);
 
-- 
2.21.0.5.gaeb582a983

[PATCH v3 6/9] leds: multicolor: Introduce a multicolor class definition

[Dan Murphy]

Introduce a multicolor class that groups colored LEDs
within a LED node.

The framework allows for dynamically setting individual LEDs
or setting brightness levels of LEDs and updating them virtually
simultaneously.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/Kconfig                 |  10 +
 drivers/leds/Makefile                |   1 +
 drivers/leds/led-class-multicolor.c  | 421 +++++++++++++++++++++++++++
 include/linux/led-class-multicolor.h |  95 ++++++
 4 files changed, 527 insertions(+)
 create mode 100644 drivers/leds/led-class-multicolor.c
 create mode 100644 include/linux/led-class-multicolor.h

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 0414adebb177..0696a13c9527 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -29,6 +29,16 @@ config LEDS_CLASS_FLASH
 	  for the flash related features of a LED device. It can be built
 	  as a module.
 
+config LEDS_CLASS_MULTI_COLOR
+	tristate "LED Mulit Color LED Class Support"
+	depends on LEDS_CLASS
+	help
+	  This option enables the multicolor LED sysfs class in /sys/class/leds.
+	  It wraps LED Class and adds multicolor LED specific sysfs attributes
+	  and kernel internal API to it. You'll need this to provide support
+	  for multicolor LEDs that are grouped together. This class is not
+	  intended for single color LEDs.  It can be built as a module.
+
 config LEDS_BRIGHTNESS_HW_CHANGED
 	bool "LED Class brightness_hw_changed attribute support"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b0054f379..c57c2aec9510 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -4,6 +4,7 @@
 obj-$(CONFIG_NEW_LEDS)			+= led-core.o
 obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
 obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
+obj-$(CONFIG_LEDS_CLASS_MULTI_COLOR)	+= led-class-multicolor.o
 obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
 
 # LED Platform Drivers
diff --git a/drivers/leds/led-class-multicolor.c b/drivers/leds/led-class-multicolor.c
new file mode 100644
index 000000000000..3f989e552d09
--- /dev/null
+++ b/drivers/leds/led-class-multicolor.c
@@ -0,0 +1,421 @@
+// SPDX-License-Identifier: GPL-2.0
+// LED Multi Color class interface
+// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include "leds.h"
+
+struct led_classdev_mc_priv {
+	struct led_classdev_mc *mcled_cdev;
+
+	struct device_attribute max_brightness_attr;
+	struct device_attribute brightness_attr;
+
+	enum led_brightness max_brightness;
+	enum led_brightness brightness;
+	enum led_brightness new_brightness;
+	struct list_head list;
+
+	int color_id;
+};
+
+static ssize_t sync_store(struct device *dev,
+			  struct device_attribute *sync_attr,
+			  const char *buf, size_t size)
+{
+	struct led_classdev_mc_data *data = container_of(sync_attr,
+						    struct led_classdev_mc_data,
+						      sync_attr);
+	struct led_classdev_mc *mcled_cdev = data->mcled_cdev;
+	struct led_classdev *led_cdev = &mcled_cdev->led_cdev;
+	struct led_classdev_mc_priv *priv;
+	unsigned long sync_value;
+	ssize_t ret = -EINVAL;
+
+	mutex_lock(&led_cdev->led_access);
+
+	if (!mcled_cdev->sync_enabled)
+		goto unlock;
+
+	ret = kstrtoul(buf, 0, &sync_value);
+	if (ret)
+		goto unlock;
+
+	if (!sync_value) {
+		ret = size;
+		goto unlock;
+	}
+
+	list_for_each_entry(priv, &data->color_list, list)
+		priv->brightness = priv->new_brightness;
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+
+static ssize_t sync_enable_store(struct device *dev,
+				 struct device_attribute *sync_enable_attr,
+				 const char *buf, size_t size)
+{
+	struct led_classdev_mc_data *data = container_of(sync_enable_attr,
+						    struct led_classdev_mc_data,
+						      sync_enable_attr);
+	struct led_classdev_mc *mcled_cdev = data->mcled_cdev;
+	struct led_classdev *led_cdev = &mcled_cdev->led_cdev;
+	unsigned long sync_value;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_access);
+
+	ret = kstrtoul(buf, 0, &sync_value);
+	if (ret)
+		goto unlock;
+
+	mcled_cdev->sync_enabled = sync_value;
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+
+static ssize_t sync_enable_show(struct device *dev,
+				struct device_attribute *sync_enable_attr,
+				char *buf)
+{
+	struct led_classdev_mc_data *data = container_of(sync_enable_attr,
+						    struct led_classdev_mc_data,
+						      sync_enable_attr);
+	struct led_classdev_mc *mcled_cdev = data->mcled_cdev;
+
+	return sprintf(buf, "%d\n", mcled_cdev->sync_enabled);
+}
+
+static ssize_t brightness_store(struct device *dev,
+				struct device_attribute *brightness_attr,
+				const char *buf, size_t size)
+{
+	struct led_classdev_mc_priv *priv = container_of(brightness_attr,
+						    struct led_classdev_mc_priv,
+						      brightness_attr);
+	struct led_classdev *led_cdev = &priv->mcled_cdev->led_cdev;
+	struct led_multicolor_ops *ops = priv->mcled_cdev->ops;
+
+	unsigned long value;
+	ssize_t ret;
+
+	mutex_lock(&led_cdev->led_access);
+
+	ret = kstrtoul(buf, 10, &value);
+	if (ret)
+		goto unlock;
+
+	if (value > priv->max_brightness) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	priv->new_brightness = value;
+
+	if (priv->mcled_cdev->sync_enabled) {
+		ret = size;
+		goto unlock;
+	}
+
+	ret = ops->set_color_brightness(priv->mcled_cdev,
+					priv->color_id, value);
+	if (ret < 0)
+		goto unlock;
+
+	priv->brightness = priv->new_brightness;
+
+	ret = size;
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+
+static ssize_t brightness_show(struct device *dev,
+			      struct device_attribute *brightness_attr,
+			      char *buf)
+{
+	struct led_classdev_mc_priv *priv = container_of(brightness_attr,
+						    struct led_classdev_mc_priv,
+						      brightness_attr);
+	const struct led_multicolor_ops *ops = priv->mcled_cdev->ops;
+	int value = 0;
+
+	if (priv->mcled_cdev->sync_enabled) {
+		value = priv->brightness;
+		goto sync_enabled;
+	}
+
+	if (ops->get_color_brightness) {
+		value = ops->get_color_brightness(priv->mcled_cdev,
+						  priv->color_id);
+		priv->brightness = value;
+	} else {
+		value = priv->brightness;
+	}
+
+sync_enabled:
+	return sprintf(buf, "%d\n", value);
+}
+
+static ssize_t max_brightness_show(struct device *dev,
+				   struct device_attribute *max_brightness_attr,
+				   char *buf)
+{
+	struct led_classdev_mc_priv *priv = container_of(max_brightness_attr,
+						    struct led_classdev_mc_priv,
+						      max_brightness_attr);
+
+	return sprintf(buf, "%d\n", priv->max_brightness);
+}
+
+static int multicolor_set_brightness(struct led_classdev *led_cdev,
+			     enum led_brightness brightness)
+{
+	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
+	struct led_classdev_mc_data *data = mcled_cdev->data;
+	struct led_multicolor_ops *ops = mcled_cdev->ops;
+	struct led_classdev_mc_priv *priv;
+	unsigned long state = brightness;
+	int adj_value;
+	ssize_t ret = -EINVAL;
+
+	mutex_lock(&led_cdev->led_access);
+
+	if (ops->set_module_brightness) {
+		ret = ops->set_module_brightness(mcled_cdev, state);
+		goto unlock;
+	}
+
+	list_for_each_entry(priv, &data->color_list, list) {
+		if (state && priv->brightness && priv->max_brightness) {
+			adj_value = state * ((priv->brightness * 100) / priv->max_brightness);
+			adj_value = adj_value / 100;
+		} else
+			adj_value = LED_OFF;
+
+		ret = ops->set_color_brightness(priv->mcled_cdev,
+						priv->color_id,	adj_value);
+		if (ret < 0)
+			goto unlock;
+	}
+
+unlock:
+	mutex_unlock(&led_cdev->led_access);
+	return ret;
+}
+
+static int led_multicolor_init_color(struct led_classdev_mc_data *data,
+				     struct led_classdev_mc *mcled_cdev,
+				     int color_id)
+{
+	struct led_classdev *led_cdev = &mcled_cdev->led_cdev;
+	struct led_classdev_mc_priv *mc_priv;
+	int ret;
+
+	mc_priv = devm_kzalloc(led_cdev->dev, sizeof(*mc_priv), GFP_KERNEL);
+	if (!mc_priv)
+		return -ENOMEM;
+
+	mc_priv->color_id = color_id;
+	mc_priv->mcled_cdev = mcled_cdev;
+
+	data->led_kobj = kobject_create_and_add(led_colors[color_id],
+						data->color_kobj);
+	if (!data->led_kobj)
+		return -EINVAL;
+
+	sysfs_attr_init(&mc_priv->brightness_attr.attr);
+	mc_priv->brightness_attr.attr.name = "brightness";
+	mc_priv->brightness_attr.attr.mode = S_IRUGO | S_IWUGO;
+	mc_priv->brightness_attr.store = brightness_store;
+	mc_priv->brightness_attr.show = brightness_show;
+	ret = sysfs_create_file(data->led_kobj,
+				&mc_priv->brightness_attr.attr);
+	if (ret)
+		goto err_out;
+
+	sysfs_attr_init(&mc_priv->max_brightness_attr.attr);
+	mc_priv->max_brightness_attr.attr.name = "max_brightness";
+	mc_priv->max_brightness_attr.attr.mode = S_IRUGO;
+	mc_priv->max_brightness_attr.show = max_brightness_show;
+	ret = sysfs_create_file(data->led_kobj,
+				&mc_priv->max_brightness_attr.attr);
+	if (ret)
+		goto err_out;
+
+	mc_priv->max_brightness = LED_FULL;
+	list_add_tail(&mc_priv->list, &data->color_list);
+
+err_out:
+	return ret;
+}
+
+static int led_multicolor_init_color_dir(struct led_classdev_mc_data *data,
+					 struct led_classdev_mc *mcled_cdev)
+{
+	struct led_classdev *led_cdev = &mcled_cdev->led_cdev;
+	int ret;
+
+	data->color_kobj = kobject_create_and_add("colors",
+						  &led_cdev->dev->kobj);
+	if (!data->color_kobj)
+		return -EINVAL;
+
+	sysfs_attr_init(&data->sync_enable_attr.attr);
+	data->sync_enable_attr.attr.name = "sync_enable";
+	data->sync_enable_attr.attr.mode = S_IRUGO | S_IWUGO;
+	data->sync_enable_attr.show = sync_enable_show;
+	data->sync_enable_attr.store = sync_enable_store;
+	ret = sysfs_create_file(data->color_kobj,
+				&data->sync_enable_attr.attr);
+	if (ret)
+		goto err_out;
+
+	sysfs_attr_init(&data->sync_attr.attr);
+	data->sync_attr.attr.name = "sync";
+	data->sync_attr.attr.mode = S_IWUGO;
+	data->sync_attr.store = sync_store;
+	ret = sysfs_create_file(data->color_kobj,
+				&data->sync_attr.attr);
+	if (ret)
+		goto err_out;
+
+	data->mcled_cdev = mcled_cdev;
+err_out:
+	return ret;
+}
+
+int led_classdev_multicolor_register_ext(struct device *parent,
+				     struct led_classdev_mc *mcled_cdev,
+				     struct led_init_data *init_data)
+{
+	struct led_classdev *led_cdev;
+	struct led_multicolor_ops *ops;
+	struct led_classdev_mc_data *data;
+	int ret;
+	int i;
+
+	if (!mcled_cdev)
+		return -EINVAL;
+
+	ops = mcled_cdev->ops;
+	if (!ops || !ops->set_color_brightness)
+		return -EINVAL;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mcled_cdev->data = data;
+	led_cdev = &mcled_cdev->led_cdev;
+
+	if (led_cdev->brightness_set_blocking)
+		led_cdev->brightness_set_blocking = multicolor_set_brightness;
+
+	INIT_LIST_HEAD(&data->color_list);
+
+	/* Register led class device */
+	ret = led_classdev_register_ext(parent, led_cdev, init_data);
+	if (ret)
+		return ret;
+
+	ret = led_multicolor_init_color_dir(data, mcled_cdev);
+	if (ret)
+		return ret;
+
+	/* Select the sysfs attributes to be created for the device */
+	for (i = 0; i < mcled_cdev->num_leds; i++) {
+		ret = led_multicolor_init_color(data, mcled_cdev,
+						mcled_cdev->available_colors[i]);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
+
+void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev)
+{
+	if (!mcled_cdev)
+		return;
+
+	led_classdev_unregister(&mcled_cdev->led_cdev);
+}
+EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister);
+
+static void devm_led_classdev_multicolor_release(struct device *dev, void *res)
+{
+	led_classdev_multicolor_unregister(*(struct led_classdev_mc **)res);
+}
+
+/**
+ * devm_of_led_classdev_register - resource managed led_classdev_register()
+ *
+ * @parent: parent of LED device
+ * @led_cdev: the led_classdev structure for this device.
+ */
+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;
+
+	if (WARN_ON(!p || !*p))
+		return 0;
+
+	return *p == data;
+}
+
+/**
+ * devm_led_classdev_multicolor_unregister() - resource managed
+ *					led_classdev_multicolor_unregister()
+ * @parent: The device to unregister.
+ * @mcled_cdev: the led_classdev_mc structure for this device.
+ */
+void devm_led_classdev_multicolor_unregister(struct device *dev,
+				  struct led_classdev_mc *mcled_cdev)
+{
+	WARN_ON(devres_release(dev,
+			       devm_led_classdev_multicolor_release,
+			       devm_led_classdev_multicolor_match, mcled_cdev));
+}
+EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_unregister);
+
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
+MODULE_DESCRIPTION("Multi Color LED class interface");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h
new file mode 100644
index 000000000000..f9e71d984b03
--- /dev/null
+++ b/include/linux/led-class-multicolor.h
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* LED Multicolor class interface
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED
+#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED
+
+#include <linux/leds.h>
+#include <dt-bindings/leds/common.h>
+
+struct led_classdev_mc;
+
+struct led_classdev_mc_data {
+	struct led_classdev_mc *mcled_cdev;
+
+	struct kobject *color_kobj;
+	struct kobject *led_kobj;
+
+	struct device_attribute sync_attr;
+	struct device_attribute sync_enable_attr;
+
+	struct list_head color_list;
+};
+
+struct led_multicolor_ops {
+	/* Set brightness for a specific color id */
+	int (*set_color_brightness)(struct led_classdev_mc *mcled_cdev,
+				    int color_id, int value);
+	/* Read current color setting */
+	int (*get_color_brightness)(struct led_classdev_mc *mcled_cdev,
+				    int color_id);
+	/* Set brightness for the color module */
+	int (*set_module_brightness)(struct led_classdev_mc *mcled_cdev,
+				     int value);
+	/* Read current module brightness */
+	int (*get_module_brightness)(struct led_classdev_mc *mcled_cdev,
+				     int color_id);
+};
+
+struct led_classdev_mc {
+	/* led class device */
+	struct led_classdev led_cdev;
+
+	/* multicolor led specific ops */
+	struct led_multicolor_ops *ops;
+
+	struct led_classdev_mc_data *data;
+
+	u32 available_colors[LED_COLOR_ID_COUNT];
+	int num_leds;
+
+	bool sync_enabled;
+};
+
+static inline struct led_classdev_mc *lcdev_to_mccdev(
+						struct led_classdev *lcdev)
+{
+	return container_of(lcdev, struct led_classdev_mc, led_cdev);
+}
+
+/**
+ * led_classdev_multicolor_register_ext - register a new object of led_classdev
+ *				      class with support for multicolor LEDs
+ * @parent: the multicolor LED to register
+ * @mcled_cdev: the led_classdev_mc structure for this device
+ * @init_data: the LED class Multi color device initialization data
+ *
+ * Returns: 0 on success or negative error value on failure
+ */
+extern int led_classdev_multicolor_register_ext(struct device *parent,
+					    struct led_classdev_mc *mcled_cdev,
+					    struct led_init_data *init_data);
+
+#define led_classdev_multicolor_register(parent, mcled_cdev)		\
+	led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL)
+
+/**
+ * led_classdev_multicolor_unregister - unregisters an object of led_classdev
+ *					class with support for multicolor LEDs
+ * @mcled_cdev: the multicolor LED to unregister
+ *
+ * Unregister a previously registered via led_classdev_multicolor_register
+ * object
+ */
+extern void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev);
+
+extern int devm_led_classdev_multicolor_register(struct device *parent,
+					struct led_classdev_mc *mcled_cdev);
+
+extern void devm_led_classdev_multicolor_unregister(struct device *parent,
+					   struct led_classdev_mc *mcled_cdev);
+
+#endif	/* __LINUX_MULTICOLOR_LEDS_H_INCLUDED */
-- 
2.21.0.5.gaeb582a983

[PATCH v3 7/9] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers

[Dan Murphy]

Introduce the bindings for the Texas Instruments LP5036, LP5030, LP5024 and
LP5018 RGB LED device driver.  The LP5036/30/24/18 can control RGB LEDs
individually or as part of a control bank group.  These devices have the ability
to adjust the mixing control for the RGB LEDs to obtain different colors
independent of the overall brightness of the LED grouping.

Datasheet:
http://www.ti.com/lit/ds/symlink/lp5024.pdf
http://www.ti.com/lit/ds/symlink/lp5036.pdf

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/leds/leds-lp50xx.txt  | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.txt b/Documentation/devicetree/bindings/leds/leds-lp50xx.txt
new file mode 100644
index 000000000000..531e6a55fac3
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.txt
@@ -0,0 +1,142 @@
+* Texas Instruments - LP5018/24/30/36 RGB LED driver
+
+The LP50XX is multi-channel, I2C RGB LED Drivers that can group RGB LEDs into
+a LED group or control them individually.
+
+The difference in these RGB LED drivers is the number of supported RGB modules.
+
+Required parent properties:
+	- compatible:
+		"ti,lp5018"
+		"ti,lp5024"
+		"ti,lp5030"
+		"ti,lp5036"
+	- reg :  I2C slave address
+		lp5018/24 - 0x28
+		lp5030/36 - 0x30
+	- #address-cells : 1
+	- #size-cells : 0
+
+Optional parent properties:
+	- enable-gpios : gpio pin to enable/disable the device.
+	- vled-supply : LED supply
+
+Required child properties:
+	- #address-cells : 1
+	- #size-cells : 0
+	- reg : This is the LED module number.
+	- color : see Documentation/devicetree/bindings/leds/common.txt
+	- function : see Documentation/devicetree/bindings/leds/common.txt
+
+Required child properties only is LED modules will be banked:
+	- ti,led-bank : This property denotes the LED module numbers that will
+			be controlled as a single RGB cluster.  Each LED module
+			number will be controlled by a single LED class instance.
+			There can only be one instance of the ti,led-bank
+			property for each device node.
+
+Required grandchildren properties:
+	- reg : A single entry denoting the LED module that controls
+		the RGB cluster.
+	- color : see Documentation/devicetree/bindings/leds/leds-multicolor.txt
+	- led-sources : see Documentation/devicetree/bindings/leds/common.txt
+
+The LED outputs associated with the LED modules are defined in Table 1 of the
+corresponding data sheets.
+
+LP5018 - 6 Total RGB cluster LED outputs 0-5
+LP5024 - 8 Total RGB cluster LED outputs 0-7
+LP5030 - 10 Total RGB cluster LED outputs 0-9
+LP5036 - 12 Total RGB cluster LED outputs 0-11
+
+Optional child properties:
+	- label : see Documentation/devicetree/bindings/leds/common.txt
+	- linux,default-trigger :
+	   see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+led-controller@29 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "ti,lp5024";
+	reg = <0x29>;
+	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+	vled-supply = <&vmmcsd_fixed>;
+
+	multi-led@2 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <2>;
+		color = <LED_COLOR_ID_MULTI>;
+		function = LED_FUNCTION_STANDBY;
+		ti,led-bank = <2 3 5>;
+
+		led@6 {
+			reg = <0x6>;
+			color = <LED_COLOR_ID_RED>;
+			led-sources = <6 9 15>;
+		};
+
+		led@7 {
+			reg = <0x7>;
+			color = <LED_COLOR_ID_GREEN>;
+			led-sources = <7 10 16>;
+		};
+
+		led@8 {
+			reg = <0x8>;
+			color = <LED_COLOR_ID_BLUE>;
+			led-sources = <8 11 17>;
+		};
+	};
+
+	multi-led@1 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <1>;
+		color = <LED_COLOR_ID_MULTI>;
+		function = LED_FUNCTION_STATUS;
+
+		led@3 {
+			reg = <3>;
+			color = <LED_COLOR_ID_RED>;
+		};
+
+		led@4 {
+			reg = <4>;
+			color = <LED_COLOR_ID_GREEN>;
+		};
+
+		led@5 {
+			reg = <5>;
+			color = <LED_COLOR_ID_BLUE>;
+		};
+	};
+
+	multi-led@4 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <4>;
+		color = <LED_COLOR_ID_MULTI>;
+		function = LED_FUNCTION_ACTIVITY;
+
+		led@12 {
+			reg = <12>;
+			color = <LED_COLOR_ID_RED>;
+		};
+
+		led@13 {
+			reg = <13>;
+			color = <LED_COLOR_ID_GREEN>;
+		};
+
+		led@14 {
+			reg = <14>;
+			color = <LED_COLOR_ID_BLUE>;
+		};
+	};
+};
+
+For more product information please see the link below:
+http://www.ti.com/lit/ds/symlink/lp5024.pdf
+http://www.ti.com/lit/ds/symlink/lp5036.pdf
-- 
2.21.0.5.gaeb582a983

[PATCH v3 8/9] leds: lp50xx: Add the LP50XX family of the RGB LED driver

[Dan Murphy]

Introduce the LP5036/30/24/18 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

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       |   7 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-lp50xx.c | 714 +++++++++++++++++++++++++++++++++++++
 3 files changed, 722 insertions(+)
 create mode 100644 drivers/leds/leds-lp50xx.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 0696a13c9527..518429999986 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -336,6 +336,13 @@ 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 LED driver chip"
+	depends on LEDS_CLASS && REGMAP_I2C
+	help
+	  If you say yes here you get support for the Texas Instruments
+	  LP5036, LP5030, LP5024 and LP5018 LED driver.
+
 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 c57c2aec9510..5e8dfe28c5a5 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -33,6 +33,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
new file mode 100644
index 000000000000..5245e87e725f
--- /dev/null
+++ b/drivers/leds/leds-lp50xx.c
@@ -0,0 +1,714 @@
+// SPDX-License-Identifier: GPL-2.0
+// TI LP50XX LED chip family driver
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#include <linux/led-class-multicolor.h>
+
+#define LP50XX_DEV_CFG0		0x00
+#define LP50XX_DEV_CFG1		0x01
+#define LP50XX_LED_CFG0		0x02
+
+/* LP5018 and LP5024 registers */
+#define LP5024_BNK_BRT		0x03
+#define LP5024_BNKA_CLR		0x04
+#define LP5024_BNKB_CLR		0x05
+#define LP5024_BNKC_CLR		0x06
+#define LP5024_LED0_BRT		0x07
+#define LP5024_LED1_BRT		0x08
+#define LP5024_LED2_BRT		0x09
+#define LP5024_LED3_BRT		0x0a
+#define LP5024_LED4_BRT		0x0b
+#define LP5024_LED5_BRT		0x0c
+#define LP5024_LED6_BRT		0x0d
+#define LP5024_LED7_BRT		0x0e
+
+#define LP5024_OUT0_CLR		0x0f
+#define LP5024_OUT1_CLR		0x10
+#define LP5024_OUT2_CLR		0x11
+#define LP5024_OUT3_CLR		0x12
+#define LP5024_OUT4_CLR		0x13
+#define LP5024_OUT5_CLR		0x14
+#define LP5024_OUT6_CLR		0x15
+#define LP5024_OUT7_CLR		0x16
+#define LP5024_OUT8_CLR		0x17
+#define LP5024_OUT9_CLR		0x18
+#define LP5024_OUT10_CLR	0x19
+#define LP5024_OUT11_CLR	0x1a
+#define LP5024_OUT12_CLR	0x1b
+#define LP5024_OUT13_CLR	0x1c
+#define LP5024_OUT14_CLR	0x1d
+#define LP5024_OUT15_CLR	0x1e
+#define LP5024_OUT16_CLR	0x1f
+#define LP5024_OUT17_CLR	0x20
+#define LP5024_OUT18_CLR	0x21
+#define LP5024_OUT19_CLR	0x22
+#define LP5024_OUT20_CLR	0x23
+#define LP5024_OUT21_CLR	0x24
+#define LP5024_OUT22_CLR	0x25
+#define LP5024_OUT23_CLR	0x26
+#define LP5024_RESET		0x27
+
+/* LP5030 and LP5036 registers */
+#define LP5036_LED_CFG1		0x03
+#define LP5036_BNK_BRT		0x04
+#define LP5036_BNKA_CLR		0x05
+#define LP5036_BNKB_CLR		0x06
+#define LP5036_BNKC_CLR		0x07
+#define LP5036_LED0_BRT		0x08
+#define LP5036_LED1_BRT		0x09
+#define LP5036_LED2_BRT		0x0a
+#define LP5036_LED3_BRT		0x0b
+#define LP5036_LED4_BRT		0x0c
+#define LP5036_LED5_BRT		0x0d
+#define LP5036_LED6_BRT		0x0e
+#define LP5036_LED7_BRT		0x0f
+#define LP5036_LED8_BRT		0x10
+#define LP5036_LED9_BRT		0x11
+#define LP5036_LED10_BRT	0x12
+#define LP5036_LED11_BRT	0x13
+
+#define LP5036_OUT0_CLR		0x14
+#define LP5036_OUT1_CLR		0x15
+#define LP5036_OUT2_CLR		0x16
+#define LP5036_OUT3_CLR		0x17
+#define LP5036_OUT4_CLR		0x18
+#define LP5036_OUT5_CLR		0x19
+#define LP5036_OUT6_CLR		0x1a
+#define LP5036_OUT7_CLR		0x1b
+#define LP5036_OUT8_CLR		0x1c
+#define LP5036_OUT9_CLR		0x1d
+#define LP5036_OUT10_CLR	0x1e
+#define LP5036_OUT11_CLR	0x1f
+#define LP5036_OUT12_CLR	0x20
+#define LP5036_OUT13_CLR	0x21
+#define LP5036_OUT14_CLR	0x22
+#define LP5036_OUT15_CLR	0x23
+#define LP5036_OUT16_CLR	0x24
+#define LP5036_OUT17_CLR	0x25
+#define LP5036_OUT18_CLR	0x26
+#define LP5036_OUT19_CLR	0x27
+#define LP5036_OUT20_CLR	0x28
+#define LP5036_OUT21_CLR	0x29
+#define LP5036_OUT22_CLR	0x2a
+#define LP5036_OUT23_CLR	0x2b
+#define LP5036_OUT24_CLR	0x2c
+#define LP5036_OUT25_CLR	0x2d
+#define LP5036_OUT26_CLR	0x2e
+#define LP5036_OUT27_CLR	0x2f
+#define LP5036_OUT28_CLR	0x30
+#define LP5036_OUT29_CLR	0x31
+#define LP5036_OUT30_CLR	0x32
+#define LP5036_OUT31_CLR	0x33
+#define LP5036_OUT32_CLR	0x34
+#define LP5036_OUT33_CLR	0x35
+#define LP5036_OUT34_CLR	0x36
+#define LP5036_OUT35_CLR	0x37
+#define LP5036_RESET		0x38
+
+#define LP50XX_SW_RESET		0xff
+
+#define LP50XX_CHIP_EN		BIT(6)
+
+#define LP5018_MAX_LED_MODULES	6
+#define LP5024_MAX_LED_MODULES	8
+#define LP5030_MAX_LED_MODULES	10
+#define LP5036_MAX_LED_MODULES	12
+
+enum lp50xx_model {
+	LP5018,
+	LP5024,
+	LP5030,
+	LP5036,
+};
+
+struct lp50xx_led {
+	unsigned long bank_modules;
+	char label[LED_MAX_NAME_SIZE];
+	struct led_classdev led_dev;
+	struct led_classdev_mc mc_cdev;
+	struct lp50xx *priv;
+	u8 ctrl_bank_enabled;
+	u32 available_leds;
+	int sync_enabled;
+	int led_number;
+};
+
+/**
+ * struct lp50xx -
+ * @enable_gpio: Hardware enable gpio
+ * @regulator: LED supply regulator pointer
+ * @client: Pointer to the I2C client
+ * @regmap: Devices register map
+ * @dev: Pointer to the devices device struct
+ * @lock: Lock for reading/writing the device
+ * @model_id: id of the device
+ * @max_leds: maximum number of LEDs the device supports
+ * @num_of_banked_leds: holds the number of banked LEDs
+ * @led_brightness0_reg: first brightness register of the device
+ * @mix_out0_reg: first color mix register of the device
+ * @bank_brt_reg: bank brightness register
+ * @bank_mix_reg: color mix register
+ * @reset_reg: devices reset register
+ * @leds: Array of LED strings
+ */
+struct lp50xx {
+	struct gpio_desc *enable_gpio;
+	struct regulator *regulator;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+	enum lp50xx_model model_id;
+	int max_leds;
+	int num_of_banked_leds;
+
+	u8 led_brightness0_reg;
+	u8 mix_out0_reg;
+	u8 bank_brt_reg;
+	u8 bank_mix_reg;
+	u8 reset_reg;
+
+	/* This needs to be at the end of the struct */
+	struct lp50xx_led leds[];
+};
+
+static const struct reg_default lp5024_reg_defs[] = {
+	{LP50XX_DEV_CFG0, 0x0},
+	{LP50XX_DEV_CFG1, 0x3c},
+	{LP50XX_LED_CFG0, 0x0},
+	{LP5024_BNK_BRT, 0xff},
+	{LP5024_BNKA_CLR, 0x0f},
+	{LP5024_BNKB_CLR, 0x0f},
+	{LP5024_BNKC_CLR, 0x0f},
+	{LP5024_LED0_BRT, 0x0f},
+	{LP5024_LED1_BRT, 0xff},
+	{LP5024_LED2_BRT, 0xff},
+	{LP5024_LED3_BRT, 0xff},
+	{LP5024_LED4_BRT, 0xff},
+	{LP5024_LED5_BRT, 0xff},
+	{LP5024_LED6_BRT, 0xff},
+	{LP5024_LED7_BRT, 0xff},
+	{LP5024_OUT0_CLR, 0x0f},
+	{LP5024_OUT1_CLR, 0x00},
+	{LP5024_OUT2_CLR, 0x00},
+	{LP5024_OUT3_CLR, 0x00},
+	{LP5024_OUT4_CLR, 0x00},
+	{LP5024_OUT5_CLR, 0x00},
+	{LP5024_OUT6_CLR, 0x00},
+	{LP5024_OUT7_CLR, 0x00},
+	{LP5024_OUT8_CLR, 0x00},
+	{LP5024_OUT9_CLR, 0x00},
+	{LP5024_OUT10_CLR, 0x00},
+	{LP5024_OUT11_CLR, 0x00},
+	{LP5024_OUT12_CLR, 0x00},
+	{LP5024_OUT13_CLR, 0x00},
+	{LP5024_OUT14_CLR, 0x00},
+	{LP5024_OUT15_CLR, 0x00},
+	{LP5024_OUT16_CLR, 0x00},
+	{LP5024_OUT17_CLR, 0x00},
+	{LP5024_OUT18_CLR, 0x00},
+	{LP5024_OUT19_CLR, 0x00},
+	{LP5024_OUT20_CLR, 0x00},
+	{LP5024_OUT21_CLR, 0x00},
+	{LP5024_OUT22_CLR, 0x00},
+	{LP5024_OUT23_CLR, 0x00},
+	{LP5024_RESET, 0x00}
+};
+
+static const struct reg_default lp5036_reg_defs[] = {
+	{LP50XX_DEV_CFG0, 0x0},
+	{LP50XX_DEV_CFG1, 0x3c},
+	{LP50XX_LED_CFG0, 0x0},
+	{LP5036_LED_CFG1, 0x0},
+	{LP5036_BNK_BRT, 0xff},
+	{LP5036_BNKA_CLR, 0x0f},
+	{LP5036_BNKB_CLR, 0x0f},
+	{LP5036_BNKC_CLR, 0x0f},
+	{LP5036_LED0_BRT, 0x0f},
+	{LP5036_LED1_BRT, 0xff},
+	{LP5036_LED2_BRT, 0xff},
+	{LP5036_LED3_BRT, 0xff},
+	{LP5036_LED4_BRT, 0xff},
+	{LP5036_LED5_BRT, 0xff},
+	{LP5036_LED6_BRT, 0xff},
+	{LP5036_LED7_BRT, 0xff},
+	{LP5036_OUT0_CLR, 0x0f},
+	{LP5036_OUT1_CLR, 0x00},
+	{LP5036_OUT2_CLR, 0x00},
+	{LP5036_OUT3_CLR, 0x00},
+	{LP5036_OUT4_CLR, 0x00},
+	{LP5036_OUT5_CLR, 0x00},
+	{LP5036_OUT6_CLR, 0x00},
+	{LP5036_OUT7_CLR, 0x00},
+	{LP5036_OUT8_CLR, 0x00},
+	{LP5036_OUT9_CLR, 0x00},
+	{LP5036_OUT10_CLR, 0x00},
+	{LP5036_OUT11_CLR, 0x00},
+	{LP5036_OUT12_CLR, 0x00},
+	{LP5036_OUT13_CLR, 0x00},
+	{LP5036_OUT14_CLR, 0x00},
+	{LP5036_OUT15_CLR, 0x00},
+	{LP5036_OUT16_CLR, 0x00},
+	{LP5036_OUT17_CLR, 0x00},
+	{LP5036_OUT18_CLR, 0x00},
+	{LP5036_OUT19_CLR, 0x00},
+	{LP5036_OUT20_CLR, 0x00},
+	{LP5036_OUT21_CLR, 0x00},
+	{LP5036_OUT22_CLR, 0x00},
+	{LP5036_OUT23_CLR, 0x00},
+	{LP5036_OUT24_CLR, 0x00},
+	{LP5036_OUT25_CLR, 0x00},
+	{LP5036_OUT26_CLR, 0x00},
+	{LP5036_OUT27_CLR, 0x00},
+	{LP5036_OUT28_CLR, 0x00},
+	{LP5036_OUT29_CLR, 0x00},
+	{LP5036_OUT30_CLR, 0x00},
+	{LP5036_OUT31_CLR, 0x00},
+	{LP5036_OUT32_CLR, 0x00},
+	{LP5036_OUT33_CLR, 0x00},
+	{LP5036_OUT34_CLR, 0x00},
+	{LP5036_OUT35_CLR, 0x00},
+	{LP5036_RESET, 0x00}
+};
+
+static const struct regmap_config lp5024_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = LP5024_RESET,
+	.reg_defaults = lp5024_reg_defs,
+	.num_reg_defaults = ARRAY_SIZE(lp5024_reg_defs),
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static const struct regmap_config lp5036_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = LP5036_RESET,
+	.reg_defaults = lp5036_reg_defs,
+	.num_reg_defaults = ARRAY_SIZE(lp5036_reg_defs),
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static struct lp50xx_led *mcled_cdev_to_led(struct led_classdev_mc *mcled_cdev)
+{
+	return container_of(mcled_cdev, struct lp50xx_led, mc_cdev);
+}
+
+static int lp50xx_brightness_set(struct led_classdev_mc *mcled_cdev,
+				 int brt_val)
+{
+	struct lp50xx_led *led = mcled_cdev_to_led(mcled_cdev);
+	int ret = 0;
+	u8 reg_val;
+
+	mutex_lock(&led->priv->lock);
+
+	if (led->ctrl_bank_enabled)
+		reg_val = led->priv->bank_brt_reg;
+	else
+		reg_val = led->priv->led_brightness0_reg + led->led_number;
+
+	ret = regmap_write(led->priv->regmap, reg_val, brt_val);
+
+	mutex_unlock(&led->priv->lock);
+
+	return ret;
+}
+
+static int lp50xx_brightness_get(struct led_classdev_mc *mcled_cdev,
+				     int color_id)
+{
+	struct lp50xx_led *led = mcled_cdev_to_led(mcled_cdev);
+	unsigned int brt_val;
+	u8 reg_val;
+	int ret;
+
+	mutex_lock(&led->priv->lock);
+
+	if (led->ctrl_bank_enabled)
+		reg_val = led->priv->bank_brt_reg;
+	else
+		reg_val = led->priv->led_brightness0_reg + led->led_number;
+
+	ret = regmap_read(led->priv->regmap, reg_val, &brt_val);
+
+	mutex_unlock(&led->priv->lock);
+
+	return brt_val;
+}
+
+static int lp50xx_get_color(struct led_classdev_mc *mcled_cdev, int color)
+{
+	struct lp50xx_led *led = mcled_cdev_to_led(mcled_cdev);
+	struct lp50xx *priv = led->priv;
+	u8 led_offset, reg_number, reg_color_off;
+	unsigned int value;
+	int ret;
+
+	if (color == LED_COLOR_ID_RED)
+		reg_color_off = 0;
+	else if (color == LED_COLOR_ID_GREEN)
+		reg_color_off = 1;
+	else if (color == LED_COLOR_ID_BLUE)
+		reg_color_off = 2;
+	else
+		goto out;
+
+	if (led->ctrl_bank_enabled) {
+		reg_number = priv->bank_mix_reg + reg_color_off;
+	} else {
+		led_offset = (led->led_number * 3)  + reg_color_off;
+		reg_number = priv->mix_out0_reg + led_offset;
+	}
+
+	ret = regmap_read(led->priv->regmap, reg_number, &value);
+	if (ret) {
+		dev_err(&priv->client->dev, "Cannot write LED value\n");
+		goto out;
+	}
+out:
+	return value;
+}
+
+static int lp50xx_set_color(struct led_classdev_mc *mcled_cdev,
+			    int color, int value)
+{
+	struct lp50xx_led *led = mcled_cdev_to_led(mcled_cdev);
+	struct lp50xx *priv = led->priv;
+	u8 led_offset, reg_number, reg_color_off;
+	int ret = -EINVAL;
+
+	if (color == LED_COLOR_ID_RED)
+		reg_color_off = 0;
+	else if (color == LED_COLOR_ID_GREEN)
+		reg_color_off = 1;
+	else if (color == LED_COLOR_ID_BLUE)
+		reg_color_off = 2;
+	else
+		goto out;
+
+	if (led->ctrl_bank_enabled) {
+		reg_number = priv->bank_mix_reg + reg_color_off;
+	} else {
+		led_offset = (led->led_number * 3)  + reg_color_off;
+		reg_number = priv->mix_out0_reg + led_offset;
+	}
+
+	ret = regmap_write(priv->regmap, reg_number, value);
+	if (ret) {
+		dev_err(&priv->client->dev, "Cannot write LED value\n");
+		goto out;
+	}
+out:
+	return ret;
+}
+
+static struct led_multicolor_ops lp50xx_mc_ops = {
+	.set_color_brightness = lp50xx_set_color,
+	.get_color_brightness = lp50xx_get_color,
+	.set_module_brightness = lp50xx_brightness_set,
+	.get_module_brightness = lp50xx_brightness_get,
+};
+
+static void lp50xx_set_led_values(struct lp50xx *priv)
+{
+	if (priv->model_id == LP5018 || priv->model_id == LP5024) {
+		priv->led_brightness0_reg = LP5024_LED0_BRT;
+		priv->mix_out0_reg = LP5024_OUT0_CLR;
+		priv->bank_brt_reg = LP5024_BNK_BRT;
+		priv->bank_mix_reg = LP5024_BNKA_CLR;
+		priv->reset_reg = LP5024_RESET;
+	} else {
+		priv->led_brightness0_reg = LP5036_LED0_BRT;
+		priv->mix_out0_reg = LP5036_OUT0_CLR;
+		priv->bank_brt_reg = LP5036_BNK_BRT;
+		priv->bank_mix_reg = LP5036_BNKA_CLR;
+		priv->reset_reg = LP5036_RESET;
+	}
+}
+
+static int lp50xx_set_banks(struct lp50xx *priv, u32 led_strings[])
+{
+	u8 led_ctrl_enable = 0;
+	u8 led1_ctrl_enable = 0;
+	u8 ctrl_ext = 0;
+	int ret;
+	int j;
+
+	for (j = 0; j <= priv->max_leds - 1; j++) {
+		if (led_strings[j] > (LP5024_MAX_LED_MODULES - 1)) {
+			ctrl_ext = led_strings[j] - LP5024_MAX_LED_MODULES;
+			led1_ctrl_enable |= (1 << ctrl_ext);
+		} else {
+			led_ctrl_enable |= (1 << led_strings[j]);
+		}
+	}
+
+	ret = regmap_write(priv->regmap, LP50XX_LED_CFG0, led_ctrl_enable);
+
+	if (led1_ctrl_enable)
+		ret = regmap_write(priv->regmap, LP5036_LED_CFG1,
+				   led1_ctrl_enable);
+
+	return ret;
+}
+
+static int lp50xx_reset(struct lp50xx *priv)
+{
+	if (priv->enable_gpio)
+		return gpiod_direction_output(priv->enable_gpio, 1);
+	else
+		return regmap_write(priv->regmap, priv->reset_reg,
+				    LP50XX_SW_RESET);
+}
+
+static int lp50xx_enable_disable(struct lp50xx *priv, u8 enable_disable)
+{
+	return regmap_write(priv->regmap, LP50XX_DEV_CFG0, enable_disable);
+}
+
+static int lp50xx_probe_dt(struct lp50xx *priv)
+{
+	u32 led_strings[LP5036_MAX_LED_MODULES];
+	struct fwnode_handle *child = NULL;
+	struct fwnode_handle *led_node = NULL;
+	struct led_init_data init_data;
+	struct led_classdev *led_cdev;
+	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;
+
+	if (priv->model_id == LP5018)
+		priv->max_leds = LP5018_MAX_LED_MODULES;
+	else if (priv->model_id == LP5024)
+		priv->max_leds = LP5024_MAX_LED_MODULES;
+	else if (priv->model_id == LP5030)
+		priv->max_leds = LP5030_MAX_LED_MODULES;
+	else
+		priv->max_leds = LP5036_MAX_LED_MODULES;
+
+	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_strings,
+							     ret);
+
+			priv->num_of_banked_leds = ARRAY_SIZE(led_strings);
+
+			ret = lp50xx_set_banks(priv, led_strings);
+			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);
+
+			led->led_number = led_number;
+		}
+		if (ret) {
+			dev_err(&priv->client->dev,
+				"led sourcing property missing\n");
+			fwnode_handle_put(child);
+			goto child_out;
+		}
+
+		if (led_number > priv->max_leds) {
+			dev_err(&priv->client->dev,
+				"led-sources property is invalid\n");
+			ret = -EINVAL;
+			fwnode_handle_put(child);
+			goto child_out;
+		}
+
+		init_data.fwnode = child;
+		init_data.devicename = priv->client->name;
+		init_data.default_label = ":";
+
+		fwnode_property_read_string(child, "linux,default-trigger",
+				    &led->led_dev.default_trigger);
+		num_colors = 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");
+
+			led->mc_cdev.available_colors[num_colors] = color_id;
+			num_colors++;
+
+		}
+
+		led->mc_cdev.ops = &lp50xx_mc_ops;
+		led->mc_cdev.sync_enabled = 1;
+		led->mc_cdev.num_leds = num_colors;
+
+		led->priv = priv;
+		led_cdev = &led->mc_cdev.led_cdev;
+		ret = led_classdev_multicolor_register_ext(&priv->client->dev,
+						       &led->mc_cdev,
+						       &init_data);
+		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;
+}
+
+static int lp50xx_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct lp50xx *led;
+	int count;
+	int ret;
+
+	count = device_get_child_node_count(&client->dev);
+	if (!count) {
+		dev_err(&client->dev, "LEDs are not defined in device tree!");
+		return -ENODEV;
+	}
+
+	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
+			   GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	mutex_init(&led->lock);
+	led->client = client;
+	led->dev = &client->dev;
+	led->model_id = id->driver_data;
+	i2c_set_clientdata(client, led);
+
+	if (led->model_id == LP5018 || led->model_id == LP5024)
+		led->regmap = devm_regmap_init_i2c(client,
+						   &lp5024_regmap_config);
+	else
+		led->regmap = devm_regmap_init_i2c(client,
+						   &lp5036_regmap_config);
+
+	if (IS_ERR(led->regmap)) {
+		ret = PTR_ERR(led->regmap);
+		dev_err(&client->dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = lp50xx_reset(led);
+	if (ret)
+		return ret;
+
+	ret = lp50xx_probe_dt(led);
+	if (ret)
+		return ret;
+
+	lp50xx_set_led_values(led);
+
+	return lp50xx_enable_disable(led, LP50XX_CHIP_EN);
+}
+
+static int lp50xx_remove(struct i2c_client *client)
+{
+	struct lp50xx *led = i2c_get_clientdata(client);
+	int ret;
+
+	ret = lp50xx_enable_disable(led, LP50XX_CHIP_EN);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed to disable regulator\n");
+		return ret;
+	}
+
+	if (led->enable_gpio)
+		gpiod_direction_output(led->enable_gpio, 0);
+
+	if (led->regulator) {
+		ret = regulator_disable(led->regulator);
+		if (ret)
+			dev_err(&led->client->dev,
+				"Failed to disable regulator\n");
+	}
+
+	mutex_destroy(&led->lock);
+
+	return 0;
+}
+
+static const struct i2c_device_id lp50xx_id[] = {
+	{ "lp5018", LP5018 },
+	{ "lp5024", LP5024 },
+	{ "lp5030", LP5030 },
+	{ "lp5036", LP5036 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lp50xx_id);
+
+static const struct of_device_id of_lp50xx_leds_match[] = {
+	{ .compatible = "ti,lp5018", },
+	{ .compatible = "ti,lp5024", },
+	{ .compatible = "ti,lp5030", },
+	{ .compatible = "ti,lp5036", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_lp50xx_leds_match);
+
+static struct i2c_driver lp50xx_driver = {
+	.driver = {
+		.name	= "lp50xx",
+		.of_match_table = of_lp50xx_leds_match,
+	},
+	.probe		= lp50xx_probe,
+	.remove		= lp50xx_remove,
+	.id_table	= lp50xx_id,
+};
+module_i2c_driver(lp50xx_driver);
+
+MODULE_DESCRIPTION("Texas Instruments LP5024 LED driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.21.0.5.gaeb582a983

[PATCH v3 9/9] leds: Update the lp55xx to use the multi color framework

[Dan Murphy]

Update the lp5523 to use the multi color framework.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/leds-lp5523.c                |  13 +++
 drivers/leds/leds-lp55xx-common.c         | 133 ++++++++++++++++++----
 drivers/leds/leds-lp55xx-common.h         |  10 ++
 include/linux/platform_data/leds-lp55xx.h |   5 +
 4 files changed, 142 insertions(+), 19 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index fd64df5a57a5..dbcc4d100054 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -804,6 +804,18 @@ static ssize_t store_master_fader_leds(struct device *dev,
 	return ret;
 }
 
+static int lp5523_led_intensity(struct lp55xx_led *led, int chan_num)
+{
+	struct lp55xx_chip *chip = led->chip;
+	int ret;
+
+	mutex_lock(&chip->lock);
+	ret = lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + chan_num,
+		     led->brightness);
+	mutex_unlock(&chip->lock);
+	return ret;
+}
+
 static int lp5523_led_brightness(struct lp55xx_led *led)
 {
 	struct lp55xx_chip *chip = led->chip;
@@ -870,6 +882,7 @@ static struct lp55xx_device_config lp5523_cfg = {
 	.max_channel  = LP5523_MAX_LEDS,
 	.post_init_device   = lp5523_post_init_device,
 	.brightness_fn      = lp5523_led_brightness,
+	.intensity_fn       = lp5523_led_intensity,
 	.set_led_current    = lp5523_set_led_current,
 	.firmware_cb        = lp5523_firmware_loaded,
 	.run_engine         = lp5523_run_engine,
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 723f2f17497a..b54fbb8f78c6 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -38,6 +38,11 @@ static struct lp55xx_led *dev_to_lp55xx_led(struct device *dev)
 	return cdev_to_lp55xx_led(dev_get_drvdata(dev));
 }
 
+static struct lp55xx_led *mcdev_to_lp55xx_led(struct led_classdev_mc *mc_dev)
+{
+	return container_of(mc_dev, struct lp55xx_led, mc_cdev);
+}
+
 static void lp55xx_reset_device(struct lp55xx_chip *chip)
 {
 	struct lp55xx_device_config *cfg = chip->cfg;
@@ -141,15 +146,36 @@ static int lp55xx_set_brightness(struct led_classdev *cdev,
 	struct lp55xx_device_config *cfg = led->chip->cfg;
 
 	led->brightness = (u8)brightness;
+
 	return cfg->brightness_fn(led);
 }
 
+static int lp55xx_set_color(struct led_classdev_mc *mcled_cdev,
+			    int color, int value)
+{
+	struct lp55xx_led *led = mcdev_to_lp55xx_led(mcled_cdev);
+	struct lp55xx_device_config *cfg = led->chip->cfg;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(led->channel_color); i++)
+		if (led->channel_color[i] == color)
+			break;
+
+	led->brightness = (u8)value;
+	return cfg->intensity_fn(led, led->grouped_channels[i]);
+}
+
+static struct led_multicolor_ops lp55xx_mc_ops = {
+	.set_color_brightness = lp55xx_set_color,
+};
+
 static int lp55xx_init_led(struct lp55xx_led *led,
 			struct lp55xx_chip *chip, int chan)
 {
 	struct lp55xx_platform_data *pdata = chip->pdata;
 	struct lp55xx_device_config *cfg = chip->cfg;
 	struct device *dev = &chip->cl->dev;
+	struct led_classdev *led_cdev;
 	char name[32];
 	int ret;
 	int max_channel = cfg->max_channel;
@@ -162,10 +188,39 @@ 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_cdev = &led->mc_cdev.led_cdev;
+		led_cdev->brightness_set_blocking = lp55xx_set_brightness;
+		led_cdev->name = led->cdev.name;
+		led_cdev->groups = lp55xx_led_groups;
+		led->mc_cdev.ops = &lp55xx_mc_ops;
+		led->mc_cdev.num_leds = pdata->led_config[chan].num_colors;
+		memcpy(led->mc_cdev.available_colors,
+		       pdata->led_config[chan].channel_color,
+		       sizeof(led->mc_cdev.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",
@@ -173,18 +228,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",
-			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);
 
-	ret = led_classdev_register(dev, &led->cdev);
 	if (ret) {
 		dev_err(dev, "led register err: %d\n", ret);
 		return ret;
@@ -541,6 +589,38 @@ void lp55xx_unregister_sysfs(struct lp55xx_chip *chip)
 }
 EXPORT_SYMBOL_GPL(lp55xx_unregister_sysfs);
 
+static int lp5xx_parse_channel_child(struct device_node *np,
+				     struct lp55xx_led_config *cfg,
+				     int chan_num)
+{
+	struct device_node *child;
+	int num_colors = 0;
+	u32 color_id;
+	u32 led_number;
+	int ret;
+
+	cfg[chan_num].default_trigger =
+			of_get_property(np, "linux,default-trigger", NULL);
+
+	for_each_child_of_node(np, child) {
+		of_property_read_string(child, "chan-name",
+					&cfg[chan_num].name);
+		of_property_read_u8(child, "led-cur",
+				    &cfg[chan_num].led_current);
+		of_property_read_u8(child, "max-cur",
+				    &cfg[chan_num].max_current);
+		of_property_read_u32(child, "color", &color_id);
+		cfg[chan_num].channel_color[num_colors] = color_id;
+		ret = of_property_read_u32(child, "reg", &led_number);
+		cfg[chan_num].grouped_channels[num_colors] = led_number;
+		num_colors++;
+	}
+
+	cfg->num_colors = num_colors;
+
+	return 0;
+}
+
 struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
 						      struct device_node *np)
 {
@@ -548,6 +628,8 @@ struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
 	struct lp55xx_platform_data *pdata;
 	struct lp55xx_led_config *cfg;
 	int num_channels;
+	int num_chan_children;
+	u32 led_number;
 	int i = 0;
 
 	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
@@ -568,13 +650,26 @@ 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);
+		num_chan_children = of_get_child_count(child);
+		if (num_chan_children != 0)
+			lp5xx_parse_channel_child(child, cfg, i);
+		else {
+			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);
+			of_property_read_u32(child, "reg", &led_number);
+
+			if (led_number < 0 || led_number > 6)
+				return ERR_PTR(EINVAL);
+
+			cfg[i].chan_nr = led_number;
+		}
 
 		i++;
 	}
diff --git a/drivers/leds/leds-lp55xx-common.h b/drivers/leds/leds-lp55xx-common.h
index abf1fb5da37d..cd28ec2c78a5 100644
--- a/drivers/leds/leds-lp55xx-common.h
+++ b/drivers/leds/leds-lp55xx-common.h
@@ -15,6 +15,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,
@@ -112,6 +114,9 @@ struct lp55xx_device_config {
 	/* access brightness register */
 	int (*brightness_fn)(struct lp55xx_led *led);
 
+	/* access brightness register */
+	int (*intensity_fn)(struct lp55xx_led *led, int chan_num);
+
 	/* current setting function */
 	void (*set_led_current) (struct lp55xx_led *led, u8 led_current);
 
@@ -162,6 +167,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
@@ -170,9 +176,13 @@ 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 num_colors;
+	int channel_color[10];
+	int grouped_channels[10];
 	struct lp55xx_chip *chip;
 };
 
diff --git a/include/linux/platform_data/leds-lp55xx.h b/include/linux/platform_data/leds-lp55xx.h
index 624ff9edad6f..bde6827f1aea 100644
--- a/include/linux/platform_data/leds-lp55xx.h
+++ b/include/linux/platform_data/leds-lp55xx.h
@@ -15,6 +15,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
@@ -26,6 +28,9 @@ struct lp55xx_led_config {
 	u8 chan_nr;
 	u8 led_current; /* mA x10, 0 if led is not connected */
 	u8 max_current;
+	int num_colors;
+	int channel_color[10];
+	int grouped_channels[10];
 };
 
 struct lp55xx_predef_pattern {
-- 
2.21.0.5.gaeb582a983

Re: [PATCH v3 7/9] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers

[Rob Herring]

On Thu, 23 May 2019 14:08:18 -0500, Dan Murphy wrote:
> Introduce the bindings for the Texas Instruments LP5036, LP5030, LP5024 and
> LP5018 RGB LED device driver.  The LP5036/30/24/18 can control RGB LEDs
> individually or as part of a control bank group.  These devices have the ability
> to adjust the mixing control for the RGB LEDs to obtain different colors
> independent of the overall brightness of the LED grouping.
> 
> Datasheet:
> http://www.ti.com/lit/ds/symlink/lp5024.pdf
> http://www.ti.com/lit/ds/symlink/lp5036.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../devicetree/bindings/leds/leds-lp50xx.txt  | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.txt
> 

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

On Thu 2019-05-23 14:08:12, Dan Murphy wrote:
> Add a documentation of LED Multicolor LED class specific
> sysfs attributes.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../ABI/testing/sysfs-class-led-multicolor    | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
> new file mode 100644
> index 000000000000..2f102ede258b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
> @@ -0,0 +1,57 @@
> +What:		/sys/class/leds/<led>/colors/sync_enable
> +Date:		April 2019

I believe I suggested more reasonable interface. Why not use that?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Jacek Anaszewski]

Hi Dan,

Thank you for the update.

One thing is missing here - we need to document how legacy brightness
levels map to the sub-LED color levels, i.e. what you do in
multicolor_set_brightness().

Best regards,
Jacek Anaszewski

On 5/23/19 9:08 PM, Dan Murphy wrote:
> Add a documentation of LED Multicolor LED class specific
> sysfs attributes.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>   .../ABI/testing/sysfs-class-led-multicolor    | 57 +++++++++++++++++++
>   1 file changed, 57 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
> new file mode 100644
> index 000000000000..2f102ede258b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
> @@ -0,0 +1,57 @@
> +What:		/sys/class/leds/<led>/colors/sync_enable
> +Date:		April 2019
> +KernelVersion:	5.2
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	read/write
> +		Writing a 1 to this file will enable the synchronization of all
> +		the defined color LEDs within the LED node.  Brightness values
> +		for each LED will be stored and written when sync is set to 1.
> +		Writing a 0 to this file will disable syncing and allow
> +		individual control of the LEDs brightness settings.
> +
> +What:		/sys/class/leds/<led>/colors/sync
> +Date:		April 2019
> +KernelVersion:	5.2
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	write only
> +		Writing a 1 to this file while sync_enable is set to 1 will
> +		write the current brightness values to all defined LEDs within
> +		the LED node.  All LEDs defined will be configured based
> +		on the brightness that has been requested.
> +
> +		If sync_enable is set to 0 then writing a 1 to sync has no
> +		affect on the LEDs.
> +
> +What:		/sys/class/leds/<led>/colors/<led_color>/brightness
> +Date:		April 2019
> +KernelVersion:	5.2
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	read/write
> +		The led_color directory is dynamically created based on the
> +		colors defined by the registrar of the class.
> +		The led_color can be but not limited to red, green, blue,
> +		white, amber, yellow and violet.  Drivers can also declare a
> +		LED color for presentation.  There is one directory per color
> +		presented.  The brightness file is created under each
> +		led_color directory and controls the individual LED color
> +		setting.
> +
> +		If sync is enabled then	writing the brightness value of the LED
> +		is deferred until a 1 is written to
> +		/sys/class/leds/<led>/color/sync.  If syncing is
> +		disabled then the LED brightness value will be written
> +		immediately to the LED driver.
> +
> +		The value of the color is from 0 to
> +		/sys/class/leds/<led>/colors/<led_color>/max_brightness.
> +
> +What:		/sys/class/leds/<led>/colors/<led_color>/max_brightness
> +Date:		April 2019
> +KernelVersion:	5.2
> +Contact:	Dan Murphy <dmurphy@ti.com>
> +Description:	read only
> +		Maximum brightness level for the LED color, default is
> +		255 (LED_FULL).
> +
> +		If the LED does not support different brightness levels, this
> +		should be 1.
> 

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Dan Murphy]

Pavel

On 5/27/19 5:33 AM, Pavel Machek wrote:
> On Thu 2019-05-23 14:08:12, Dan Murphy wrote:
>> Add a documentation of LED Multicolor LED class specific
>> sysfs attributes.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   .../ABI/testing/sysfs-class-led-multicolor    | 57 +++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
>> new file mode 100644
>> index 000000000000..2f102ede258b
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
>> @@ -0,0 +1,57 @@
>> +What:		/sys/class/leds/<led>/colors/sync_enable
>> +Date:		April 2019
> I believe I suggested more reasonable interface. Why not use that?
>

Can you please provide the reference to your interface?

These patchsets have been around for a while (Decemeber 2018) and I 
cannot seem to find the reference to your suggestion.

The suggestion may have been mired in the brightness model discussions.

So I don't want to over look what you suggested as it may be more 
reasonable then what I have implemented.

Dan

Re: [PATCH v3 7/9] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers

[Dan Murphy]

Rob

On 5/24/19 3:50 PM, Rob Herring wrote:
> On Thu, 23 May 2019 14:08:18 -0500, Dan Murphy wrote:
>> Introduce the bindings for the Texas Instruments LP5036, LP5030, LP5024 and
>> LP5018 RGB LED device driver.  The LP5036/30/24/18 can control RGB LEDs
>> individually or as part of a control bank group.  These devices have the ability
>> to adjust the mixing control for the RGB LEDs to obtain different colors
>> independent of the overall brightness of the LED grouping.
>>
>> Datasheet:
>> http://www.ti.com/lit/ds/symlink/lp5024.pdf
>> http://www.ti.com/lit/ds/symlink/lp5036.pdf
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   .../devicetree/bindings/leds/leds-lp50xx.txt  | 142 ++++++++++++++++++
>>   1 file changed, 142 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.txt
>>
> Please add Acked-by/Reviewed-by tags when posting new versions. However,
> there's no need to repost patches *only* to add the tags. The upstream
> maintainer will do that for acks received on the version they apply.
>
> If a tag was not added on purpose, please state why and what changed.

I usually provide the tags but in my cover letter I did indicate that 
these patchsets have changed pretty dramatically so I did not feel right 
about pulling in anyones tags.

Adding your reviewed by on a binding that changed did not seem right to me.

Dan

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Dan Murphy]

Pavel

On 5/27/19 7:45 PM, Dan Murphy wrote:
> Pavel
>
> On 5/27/19 5:33 AM, Pavel Machek wrote:
>> On Thu 2019-05-23 14:08:12, Dan Murphy wrote:
>>> Add a documentation of LED Multicolor LED class specific
>>> sysfs attributes.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>   .../ABI/testing/sysfs-class-led-multicolor    | 57 
>>> +++++++++++++++++++
>>>   1 file changed, 57 insertions(+)
>>>   create mode 100644 
>>> Documentation/ABI/testing/sysfs-class-led-multicolor
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor 
>>> b/Documentation/ABI/testing/sysfs-class-led-multicolor
>>> new file mode 100644
>>> index 000000000000..2f102ede258b
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
>>> @@ -0,0 +1,57 @@
>>> +What:        /sys/class/leds/<led>/colors/sync_enable
>>> +Date:        April 2019
>> I believe I suggested more reasonable interface. Why not use that?
>>
>
> Can you please provide the reference to your interface?
>
I think I found the suggestion [0].  Assuming that was the suggestion it 
violates the kernel 1 value/file and there was agreement that this 
interface had value. In testing the interface, it made sense to be able 
to setup a color after writing each color brightness file and then 
syncing the color LEDs with the new brightness levels.  It also provides 
flexibility to be able to set a single LED color without writing sync.  
The decision to do either or should really be a user space decision.   
If I have referenced the wrong thread please let me know which thread 
you are referring to.

[0] https://lore.kernel.org/patchwork/patch/1057044/

<snip>

Dan

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Dan Murphy]

Jacek

On 5/27/19 3:00 PM, Jacek Anaszewski wrote:
> Hi Dan,
>
> Thank you for the update.
>
> One thing is missing here - we need to document how legacy brightness
> levels map to the sub-LED color levels, i.e. what you do in
> multicolor_set_brightness().


Ok so i will need to document the algorithm that is used to determine 
the color LED brightness.


Dan

>
> Best regards,
> Jacek Anaszewski
>

<snip>

>

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Jacek Anaszewski]

Dan,

On 5/28/19 7:32 PM, Dan Murphy wrote:
> Jacek
> 
> On 5/27/19 3:00 PM, Jacek Anaszewski wrote:
>> Hi Dan,
>>
>> Thank you for the update.
>>
>> One thing is missing here - we need to document how legacy brightness
>> levels map to the sub-LED color levels, i.e. what you do in
>> multicolor_set_brightness().
> 
> 
> Ok so i will need to document the algorithm that is used to determine 
> the color LED brightness.

Right, and please send just an update of that single patch.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Dan Murphy]

Jacek

On 5/28/19 12:44 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 5/28/19 7:32 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 5/27/19 3:00 PM, Jacek Anaszewski wrote:
>>> Hi Dan,
>>>
>>> Thank you for the update.
>>>
>>> One thing is missing here - we need to document how legacy brightness
>>> levels map to the sub-LED color levels, i.e. what you do in
>>> multicolor_set_brightness().
>>
>>
>> Ok so i will need to document the algorithm that is used to determine 
>> the color LED brightness.
>
> Right, and please send just an update of that single patch.
>

So you are asking for v4 with only this patch updated.


Dan

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Jacek Anaszewski]

On 5/28/19 8:19 PM, Dan Murphy wrote:
> Jacek
> 
> On 5/28/19 12:44 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 5/28/19 7:32 PM, Dan Murphy wrote:
>>> Jacek
>>>
>>> On 5/27/19 3:00 PM, Jacek Anaszewski wrote:
>>>> Hi Dan,
>>>>
>>>> Thank you for the update.
>>>>
>>>> One thing is missing here - we need to document how legacy brightness
>>>> levels map to the sub-LED color levels, i.e. what you do in
>>>> multicolor_set_brightness().
>>>
>>>
>>> Ok so i will need to document the algorithm that is used to determine 
>>> the color LED brightness.
>>
>> Right, and please send just an update of that single patch.
>>
> 
> So you are asking for v4 with only this patch updated.

Not exactly - I am asking for v4 of this patch. We don't need
to spam the lists with the rest of unaltered patches from the series.

The interface seems to be the most vital part of this patch set,
and it is possible that it will undergo at least slight modifications.

We will move to discussing the code once we achieve a total consensus.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Dan Murphy]

Jacek

On 5/28/19 1:29 PM, Jacek Anaszewski wrote:
> On 5/28/19 8:19 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 5/28/19 12:44 PM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 5/28/19 7:32 PM, Dan Murphy wrote:
>>>> Jacek
>>>>
>>>> On 5/27/19 3:00 PM, Jacek Anaszewski wrote:
>>>>> Hi Dan,
>>>>>
>>>>> Thank you for the update.
>>>>>
>>>>> One thing is missing here - we need to document how legacy brightness
>>>>> levels map to the sub-LED color levels, i.e. what you do in
>>>>> multicolor_set_brightness().
>>>>
>>>>
>>>> Ok so i will need to document the algorithm that is used to 
>>>> determine the color LED brightness.
>>>
>>> Right, and please send just an update of that single patch.
>>>
>>
>> So you are asking for v4 with only this patch updated.
>
> Not exactly - I am asking for v4 of this patch. We don't need
> to spam the lists with the rest of unaltered patches from the series.
>
> The interface seems to be the most vital part of this patch set,
> and it is possible that it will undergo at least slight modifications.
>
> We will move to discussing the code once we achieve a total consensus.
>

Sorry for the late reply.  OK I have updated the sysfs documentation but 
in doing so I am also going to send in v4 of the sysfs documentation as 
it explains the interfaces in more detail. Also I will be adding more 
technical information into that sysfs doc on what is done and what to 
expect and it's usage.


Dan

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

On Tue 2019-05-28 06:34:47, Dan Murphy wrote:
> Pavel
> 
> On 5/27/19 7:45 PM, Dan Murphy wrote:
> >Pavel
> >
> >On 5/27/19 5:33 AM, Pavel Machek wrote:
> >>On Thu 2019-05-23 14:08:12, Dan Murphy wrote:
> >>>Add a documentation of LED Multicolor LED class specific
> >>>sysfs attributes.
> >>>
> >>>Signed-off-by: Dan Murphy <dmurphy@ti.com>
> >>>---
> >>>  .../ABI/testing/sysfs-class-led-multicolor    | 57
> >>>+++++++++++++++++++
> >>>  1 file changed, 57 insertions(+)
> >>>  create mode 100644
> >>>Documentation/ABI/testing/sysfs-class-led-multicolor
> >>>
> >>>diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor
> >>>b/Documentation/ABI/testing/sysfs-class-led-multicolor
> >>>new file mode 100644
> >>>index 000000000000..2f102ede258b
> >>>--- /dev/null
> >>>+++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
> >>>@@ -0,0 +1,57 @@
> >>>+What:        /sys/class/leds/<led>/colors/sync_enable
> >>>+Date:        April 2019
> >>I believe I suggested more reasonable interface. Why not use that?
> >>
> >
> >Can you please provide the reference to your interface?
> >
> I think I found the suggestion [0].  Assuming that was the suggestion it
> violates the kernel 1 value/file and there was agreement that this interface
> had value. In testing the interface, it made sense to be able to

1 value/file is actually slightly more complex rule:

Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type.

See sysfs.txt. Proposed "sync_enable" is ugly enough, and the values
really are array of values of same type, so we should be ok with nicer
interface.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 1/9] leds: multicolor: Add sysfs interface definition

[Dan Murphy]

Pavel

On 5/30/19 2:40 PM, Pavel Machek wrote:
> On Tue 2019-05-28 06:34:47, Dan Murphy wrote:
>> Pavel
>>
>> On 5/27/19 7:45 PM, Dan Murphy wrote:
>>> Pavel
>>>
>>> On 5/27/19 5:33 AM, Pavel Machek wrote:
>>>> On Thu 2019-05-23 14:08:12, Dan Murphy wrote:
>>>>> Add a documentation of LED Multicolor LED class specific
>>>>> sysfs attributes.
>>>>>
>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>>> ---
>>>>>    .../ABI/testing/sysfs-class-led-multicolor    | 57
>>>>> +++++++++++++++++++
>>>>>    1 file changed, 57 insertions(+)
>>>>>    create mode 100644
>>>>> Documentation/ABI/testing/sysfs-class-led-multicolor
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor
>>>>> b/Documentation/ABI/testing/sysfs-class-led-multicolor
>>>>> new file mode 100644
>>>>> index 000000000000..2f102ede258b
>>>>> --- /dev/null
>>>>> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
>>>>> @@ -0,0 +1,57 @@
>>>>> +What:        /sys/class/leds/<led>/colors/sync_enable
>>>>> +Date:        April 2019
>>>> I believe I suggested more reasonable interface. Why not use that?
>>>>
>>> Can you please provide the reference to your interface?
>>>
>> I think I found the suggestion [0].  Assuming that was the suggestion it
>> violates the kernel 1 value/file and there was agreement that this interface
>> had value. In testing the interface, it made sense to be able to
> 1 value/file is actually slightly more complex rule:
>
> Attributes should be ASCII text files, preferably with only one value
> per file. It is noted that it may not be efficient to contain only one
> value per file, so it is socially acceptable to express an array of
> values of the same type.
>
> See sysfs.txt. Proposed "sync_enable" is ugly enough, and the values
> really are array of values of same type, so we should be ok with nicer
> interface.

sync_enable is a binary input 0 or 1, but I could change that to ASCII 
"enabled/disabled".  And the sole purpose of this file is to enable or 
disable the synchronization of the grouped monochrome color LED 
brightness settings within that given directory.  So not sure why the 
sync_enable is ugly since it is meant to enable a feature of the class.  
Honestly I could drop the whole sync feature but I have found value in 
this interface through my testing both with a pulsing script I wrote and 
the notint.py you wrote.  This feature is meant for slower processors or 
processes with low priority that could be interrupted or switched out 
during writing the LED and cause LED color flicker.  Priming the colors 
and then updating all the available LEDs in the group in the kernel 
space with a single write to the sync file is a little more intuitive.  
I could change both the sync and sync_enable to module_params instead.

After mulling over the proposed solution of passing in the array of 
values I came to the conclusion this is actually more complicated than 
it needs to be and this implementation is not flexible.

There would need to be additional files that need to present information 
to the user space on what LEDs available, how many LEDs are available 
and what the LED array order is.  The user interface cannot always 
assume that the array is RGB.

There could be RGBW, RGBA, RGBWA, GBW or WIR or any combination of 
varying LED colors.  The interface that is proposed would need to have a 
varying array as the input.

I have posted v4 of the documentation of this patchset with updated 
sysfs documentation and ABI description.

Dan

>
> Best regards,
> 									Pavel

Re: [PATCH v3 7/9] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers

[Rob Herring]

On Thu, May 23, 2019 at 02:08:18PM -0500, Dan Murphy wrote:
> Introduce the bindings for the Texas Instruments LP5036, LP5030, LP5024 and
> LP5018 RGB LED device driver.  The LP5036/30/24/18 can control RGB LEDs
> individually or as part of a control bank group.  These devices have the ability
> to adjust the mixing control for the RGB LEDs to obtain different colors
> independent of the overall brightness of the LED grouping.
> 
> Datasheet:
> http://www.ti.com/lit/ds/symlink/lp5024.pdf
> http://www.ti.com/lit/ds/symlink/lp5036.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../devicetree/bindings/leds/leds-lp50xx.txt  | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.txt

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 0/9] Multicolor Framework update

[Alexander Dahl]

Hello Dan,

Am Donnerstag, 23. Mai 2019, 14:08:11 CEST schrieb Dan Murphy:
>   leds: multicolor: Add sysfs interface definition
>   dt: bindings: Add multicolor class dt bindings documention
>   documention: leds: Add multicolor class documentation
>   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
>   leds: Update the lp55xx to use the multi color framework

While not having much experience with the implementation of the LED subsystem, 
I've had a short look at those. Curious question: would it be possible to take 
three gpio-leds and group those together to one multicolor-led? I know at 
least one board, where things are wired up like this, see e.g. 
at91-sama5d27_som1_ek.dts

Greets
Alex

Re: [PATCH v3 0/9] Multicolor Framework update

[Dan Murphy]

Hello

On 6/14/19 2:02 AM, Alexander Dahl wrote:
> Hello Dan,
>
> Am Donnerstag, 23. Mai 2019, 14:08:11 CEST schrieb Dan Murphy:
>>    leds: multicolor: Add sysfs interface definition
>>    dt: bindings: Add multicolor class dt bindings documention
>>    documention: leds: Add multicolor class documentation
>>    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
>>    leds: Update the lp55xx to use the multi color framework
> While not having much experience with the implementation of the LED subsystem,
> I've had a short look at those. Curious question: would it be possible to take
> three gpio-leds and group those together to one multicolor-led? I know at
> least one board, where things are wired up like this, see e.g.
> at91-sama5d27_som1_ek.dts

I have been thinking a lot about how gpio LEDs would fit into the mix here.

The leds-gpio.c would need to be extended to register to the multicolor 
framework for this to work.

I would need to get or create a gpio led cluster to test it out.

Dan


> Greets
> Alex
>

Re: [PATCH v3 2/9] dt: bindings: Add multicolor class dt bindings documention

[Rob Herring]

On Thu, May 23, 2019 at 02:08:13PM -0500, Dan Murphy wrote:
> Add DT bindings for the LEDs multicolor class framework.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../bindings/leds/leds-class-multicolor.txt   | 97 +++++++++++++++++++
>  1 file changed, 97 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> new file mode 100644
> index 000000000000..e2a2ce3279cb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> @@ -0,0 +1,97 @@
> +* Multicolor LED properties
> +
> +Multicolor LEDs can consist of a RGB, RGBW or a RGBA LED clusters.  These devices
> +can be grouped together and also provide a modeling mechanism so that the
> +cluster LEDs can vary in hue and intensity to produce a wide range of colors.
> +
> +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:
> +	- 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.
> +
> +led-controller@30 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "ti,lp5024";
> +	reg = <0x29>;
> +
> +	multi-led@4 {

Typically we sort by address order.

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <4>;
> +		color = <LED_COLOR_ID_MULTI>;
> +		function = LED_FUNCTION_ACTIVITY;
> +
> +		led@12 {
> +			reg = <12>;
> +			color = <LED_COLOR_ID_RED>;
> +		};
> +
> +		led@13 {
> +			reg = <13>;
> +			color = <LED_COLOR_ID_GREEN>;
> +		};
> +
> +		led@14 {
> +			reg = <14>;
> +			color = <LED_COLOR_ID_BLUE>;
> +		};
> +	};
> +
> +	/* Only support RGB no model defined */

I don't understand this comment.

> +	multi-led@1 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <1>;
> +		color = <LED_COLOR_ID_MULTI>;
> +		function = LED_FUNCTION_;

??

> +
> +
> +		led@3 {
> +			reg = <3>;
> +			color = <LED_COLOR_ID_RED>;
> +		};
> +
> +		led@4 {
> +			reg = <4>;
> +			color = <LED_COLOR_ID_GREEN>;
> +		};
> +
> +		led@5 {
> +			reg = <5>;
> +			color = <LED_COLOR_ID_BLUE>;
> +		};
> +	};
> +
> +	multi-led@2 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		color = <LED_COLOR_ID_MULTI>;
> +		function = LED_FUNCTION_ACTIVITY;
> +		reg = <2>;
> +		ti,led-bank = <2 3 5>;
> +
> +		led@6 {
> +			reg = <0x6>;
> +			color = <LED_COLOR_ID_RED>;
> +			led-sources = <6 9 15>;
> +		};
> +
> +		led@7 {
> +			reg = <0x7>;
> +			color = <LED_COLOR_ID_GREEN>;
> +			led-sources = <7 10 16>;
> +		};
> +
> +		led@8 {
> +			reg = <0x8>;
> +			color = <LED_COLOR_ID_BLUE>;
> +			led-sources = <8 11 17>;
> +		};
> +	};
> +
> +};
> -- 
> 2.21.0.5.gaeb582a983
> 

Re: [PATCH v3 4/9] dt-bindings: leds: Add multicolor ID to the color ID list

[Rob Herring]

On Thu, 23 May 2019 14:08:15 -0500, Dan Murphy wrote:
> Add a new color ID that is declared as MULTICOLOR as with the
> multicolor framework declaring a definitive color is not accurate
> as the node can contain multiple colors.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  include/dt-bindings/leds/common.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/9] dt: bindings: Add multicolor class dt bindings documention

[Dan Murphy]

Rob

Thanks for the review

On 6/14/19 12:00 PM, Rob Herring wrote:
> On Thu, May 23, 2019 at 02:08:13PM -0500, Dan Murphy wrote:
>> Add DT bindings for the LEDs multicolor class framework.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   .../bindings/leds/leds-class-multicolor.txt   | 97 +++++++++++++++++++
>>   1 file changed, 97 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>> new file mode 100644
>> index 000000000000..e2a2ce3279cb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>> @@ -0,0 +1,97 @@
>> +* Multicolor LED properties
>> +
>> +Multicolor LEDs can consist of a RGB, RGBW or a RGBA LED clusters.  These devices
>> +can be grouped together and also provide a modeling mechanism so that the
>> +cluster LEDs can vary in hue and intensity to produce a wide range of colors.
>> +
>> +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:
>> +	- 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.
>> +
>> +led-controller@30 {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	compatible = "ti,lp5024";
>> +	reg = <0x29>;
>> +
>> +	multi-led@4 {
> Typically we sort by address order.

These are not addresses these end up being the "module" number that the 
LEDs below are associated to.


>
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		reg = <4>;
>> +		color = <LED_COLOR_ID_MULTI>;
>> +		function = LED_FUNCTION_ACTIVITY;
>> +
>> +		led@12 {
>> +			reg = <12>;
>> +			color = <LED_COLOR_ID_RED>;
>> +		};
>> +
>> +		led@13 {
>> +			reg = <13>;
>> +			color = <LED_COLOR_ID_GREEN>;
>> +		};
>> +
>> +		led@14 {
>> +			reg = <14>;
>> +			color = <LED_COLOR_ID_BLUE>;
>> +		};
>> +	};
>> +
>> +	/* Only support RGB no model defined */
> I don't understand this comment.

Artifact can be removed


>
>> +	multi-led@1 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		reg = <1>;
>> +		color = <LED_COLOR_ID_MULTI>;
>> +		function = LED_FUNCTION_;
> ??

I meant to change that but missed it in the example.

Dan


>
>> +
>> +
>> +		led@3 {
>> +			reg = <3>;
>> +			color = <LED_COLOR_ID_RED>;
>> +		};
>> +
>> +		led@4 {
>> +			reg = <4>;
>> +			color = <LED_COLOR_ID_GREEN>;
>> +		};
>> +
>> +		led@5 {
>> +			reg = <5>;
>> +			color = <LED_COLOR_ID_BLUE>;
>> +		};
>> +	};
>> +
>> +	multi-led@2 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		color = <LED_COLOR_ID_MULTI>;
>> +		function = LED_FUNCTION_ACTIVITY;
>> +		reg = <2>;
>> +		ti,led-bank = <2 3 5>;
>> +
>> +		led@6 {
>> +			reg = <0x6>;
>> +			color = <LED_COLOR_ID_RED>;
>> +			led-sources = <6 9 15>;
>> +		};
>> +
>> +		led@7 {
>> +			reg = <0x7>;
>> +			color = <LED_COLOR_ID_GREEN>;
>> +			led-sources = <7 10 16>;
>> +		};
>> +
>> +		led@8 {
>> +			reg = <0x8>;
>> +			color = <LED_COLOR_ID_BLUE>;
>> +			led-sources = <8 11 17>;
>> +		};
>> +	};
>> +
>> +};
>> -- 
>> 2.21.0.5.gaeb582a983
>>

Re: [PATCH v3 0/9] Multicolor Framework update

[Pavel Machek]

Hi!

> Am Donnerstag, 23. Mai 2019, 14:08:11 CEST schrieb Dan Murphy:
> >   leds: multicolor: Add sysfs interface definition
> >   dt: bindings: Add multicolor class dt bindings documention
> >   documention: leds: Add multicolor class documentation
> >   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
> >   leds: Update the lp55xx to use the multi color framework
> 
> While not having much experience with the implementation of the LED subsystem, 
> I've had a short look at those. Curious question: would it be possible to take 
> three gpio-leds and group those together to one multicolor-led? I know at 
> least one board, where things are wired up like this, see e.g. 
> at91-sama5d27_som1_ek.dts

Probably could be done, but is not really a good match. Multicolor was meant for
LEDs with many brightness levels.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v3 0/9] Multicolor Framework update

[Dan Murphy]

Hello

On 6/16/19 10:49 AM, Pavel Machek wrote:
> Hi!
>
>> Am Donnerstag, 23. Mai 2019, 14:08:11 CEST schrieb Dan Murphy:
>>>    leds: multicolor: Add sysfs interface definition
>>>    dt: bindings: Add multicolor class dt bindings documention
>>>    documention: leds: Add multicolor class documentation
>>>    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
>>>    leds: Update the lp55xx to use the multi color framework
>> While not having much experience with the implementation of the LED subsystem,
>> I've had a short look at those. Curious question: would it be possible to take
>> three gpio-leds and group those together to one multicolor-led? I know at
>> least one board, where things are wired up like this, see e.g.
>> at91-sama5d27_som1_ek.dts
> Probably could be done, but is not really a good match. Multicolor was meant for
> LEDs with many brightness levels.

Well that is one aspect of the MC FW.  The other aspect was to collate 
cluster RGB modules into a single

LED node as that RGB cluster would inherently display a functional color 
(ie RGB battery LED, multi-color

notification LEDs, Multicolor LED keyboards...)

Dan


> 									Pavel

Re: [PATCH v3 2/9] dt: bindings: Add multicolor class dt bindings documention

[Rob Herring]

On Fri, Jun 14, 2019 at 11:18 AM Dan Murphy <dmurphy@ti.com> wrote:
>
> Rob
>
> Thanks for the review
>
> On 6/14/19 12:00 PM, Rob Herring wrote:
> > On Thu, May 23, 2019 at 02:08:13PM -0500, Dan Murphy wrote:
> >> Add DT bindings for the LEDs multicolor class framework.
> >>
> >> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> >> ---
> >>   .../bindings/leds/leds-class-multicolor.txt   | 97 +++++++++++++++++++
> >>   1 file changed, 97 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> >> new file mode 100644
> >> index 000000000000..e2a2ce3279cb
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> >> @@ -0,0 +1,97 @@
> >> +* Multicolor LED properties
> >> +
> >> +Multicolor LEDs can consist of a RGB, RGBW or a RGBA LED clusters.  These devices
> >> +can be grouped together and also provide a modeling mechanism so that the
> >> +cluster LEDs can vary in hue and intensity to produce a wide range of colors.
> >> +
> >> +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:
> >> +    - 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.
> >> +
> >> +led-controller@30 {
> >> +    #address-cells = <1>;
> >> +    #size-cells = <0>;
> >> +    compatible = "ti,lp5024";
> >> +    reg = <0x29>;
> >> +
> >> +    multi-led@4 {
> > Typically we sort by address order.
>
> These are not addresses these end up being the "module" number that the
> LEDs below are associated to.

'reg' (and the unit-address) is an address in the sense that is how
you identify a device or sub-device. It doesn't matter what type of
'address' it is, DT practice is to sort node in unit-address numerical
order.

'module' is a h/w thing, right? A bank or instance within the device?
If not, using 'reg' here is not appropriate.

Rob

Re: [PATCH v3 2/9] dt: bindings: Add multicolor class dt bindings documention

[Jacek Anaszewski]

On 6/18/19 5:36 PM, Rob Herring wrote:
> On Fri, Jun 14, 2019 at 11:18 AM Dan Murphy <dmurphy@ti.com> wrote:
>>
>> Rob
>>
>> Thanks for the review
>>
>> On 6/14/19 12:00 PM, Rob Herring wrote:
>>> On Thu, May 23, 2019 at 02:08:13PM -0500, Dan Murphy wrote:
>>>> Add DT bindings for the LEDs multicolor class framework.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>>    .../bindings/leds/leds-class-multicolor.txt   | 97 +++++++++++++++++++
>>>>    1 file changed, 97 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>>>> new file mode 100644
>>>> index 000000000000..e2a2ce3279cb
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>>>> @@ -0,0 +1,97 @@
>>>> +* Multicolor LED properties
>>>> +
>>>> +Multicolor LEDs can consist of a RGB, RGBW or a RGBA LED clusters.  These devices
>>>> +can be grouped together and also provide a modeling mechanism so that the
>>>> +cluster LEDs can vary in hue and intensity to produce a wide range of colors.
>>>> +
>>>> +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:
>>>> +    - 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.
>>>> +
>>>> +led-controller@30 {
>>>> +    #address-cells = <1>;
>>>> +    #size-cells = <0>;
>>>> +    compatible = "ti,lp5024";
>>>> +    reg = <0x29>;
>>>> +
>>>> +    multi-led@4 {
>>> Typically we sort by address order.
>>
>> These are not addresses these end up being the "module" number that the
>> LEDs below are associated to.
> 
> 'reg' (and the unit-address) is an address in the sense that is how
> you identify a device or sub-device. It doesn't matter what type of
> 'address' it is, DT practice is to sort node in unit-address numerical
> order.
> 
> 'module' is a h/w thing, right? A bank or instance within the device?
> If not, using 'reg' here is not appropriate.

In this case reg represents LEDn_BRIGHTNESS register which controls
a group of three LEDs. The thing is that those registers' addresses
start from 0x07, i.e. the formula for calculating the RGB LED module
address is: LEDn_BRIGHTNESS = 0x07 + n.

 From the above it seems that we should have multi-led@7 and reg = 0x07
for LED0_BRIGHTNESS register governing the brightness of RGB LED
module 0, right?

And regarding sorting by address order I think that Rob was asking for
placing whole multi-led@4 sub-node after multi-led@2 (here sticking to
the numeration from the patch).

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 2/9] dt: bindings: Add multicolor class dt bindings documention

[Rob Herring]

On Tue, Jun 18, 2019 at 12:20 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>
> On 6/18/19 5:36 PM, Rob Herring wrote:
> > On Fri, Jun 14, 2019 at 11:18 AM Dan Murphy <dmurphy@ti.com> wrote:
> >>
> >> Rob
> >>
> >> Thanks for the review
> >>
> >> On 6/14/19 12:00 PM, Rob Herring wrote:
> >>> On Thu, May 23, 2019 at 02:08:13PM -0500, Dan Murphy wrote:
> >>>> Add DT bindings for the LEDs multicolor class framework.
> >>>>
> >>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> >>>> ---
> >>>>    .../bindings/leds/leds-class-multicolor.txt   | 97 +++++++++++++++++++
> >>>>    1 file changed, 97 insertions(+)
> >>>>    create mode 100644 Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> >>>> new file mode 100644
> >>>> index 000000000000..e2a2ce3279cb
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> >>>> @@ -0,0 +1,97 @@
> >>>> +* Multicolor LED properties
> >>>> +
> >>>> +Multicolor LEDs can consist of a RGB, RGBW or a RGBA LED clusters.  These devices
> >>>> +can be grouped together and also provide a modeling mechanism so that the
> >>>> +cluster LEDs can vary in hue and intensity to produce a wide range of colors.
> >>>> +
> >>>> +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:
> >>>> +    - 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.
> >>>> +
> >>>> +led-controller@30 {
> >>>> +    #address-cells = <1>;
> >>>> +    #size-cells = <0>;
> >>>> +    compatible = "ti,lp5024";
> >>>> +    reg = <0x29>;
> >>>> +
> >>>> +    multi-led@4 {
> >>> Typically we sort by address order.
> >>
> >> These are not addresses these end up being the "module" number that the
> >> LEDs below are associated to.
> >
> > 'reg' (and the unit-address) is an address in the sense that is how
> > you identify a device or sub-device. It doesn't matter what type of
> > 'address' it is, DT practice is to sort node in unit-address numerical
> > order.
> >
> > 'module' is a h/w thing, right? A bank or instance within the device?
> > If not, using 'reg' here is not appropriate.
>
> In this case reg represents LEDn_BRIGHTNESS register which controls
> a group of three LEDs. The thing is that those registers' addresses
> start from 0x07, i.e. the formula for calculating the RGB LED module
> address is: LEDn_BRIGHTNESS = 0x07 + n.
>
>  From the above it seems that we should have multi-led@7 and reg = 0x07
> for LED0_BRIGHTNESS register governing the brightness of RGB LED
> module 0, right?

Use whatever makes the most sense from a h/w perspective. If 'module
N' is something that I'd read about in the datasheet, then I'd stick
with 'N'.

>
> And regarding sorting by address order I think that Rob was asking for
> placing whole multi-led@4 sub-node after multi-led@2 (here sticking to
> the numeration from the patch).

Right.

Rob

Re: [PATCH v3 6/9] leds: multicolor: Introduce a multicolor class definition

[Jacek Anaszewski]

Hi Dan,

Thank you for the v5.

I will confine myself to commenting only some parts since
the rest will undergo rework due to removal of sync API.

On 5/23/19 9:08 PM, Dan Murphy wrote:
> Introduce a multicolor class that groups colored LEDs
> within a LED node.
> 
> The framework allows for dynamically setting individual LEDs
> or setting brightness levels of LEDs and updating them virtually
> simultaneously.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>   drivers/leds/Kconfig                 |  10 +
>   drivers/leds/Makefile                |   1 +
>   drivers/leds/led-class-multicolor.c  | 421 +++++++++++++++++++++++++++
>   include/linux/led-class-multicolor.h |  95 ++++++
>   4 files changed, 527 insertions(+)
>   create mode 100644 drivers/leds/led-class-multicolor.c
>   create mode 100644 include/linux/led-class-multicolor.h
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 0414adebb177..0696a13c9527 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -29,6 +29,16 @@ config LEDS_CLASS_FLASH
>   	  for the flash related features of a LED device. It can be built
>   	  as a module.
>   
> +config LEDS_CLASS_MULTI_COLOR
> +	tristate "LED Mulit Color LED Class Support"
> +	depends on LEDS_CLASS
> +	help
> +	  This option enables the multicolor LED sysfs class in /sys/class/leds.
> +	  It wraps LED Class and adds multicolor LED specific sysfs attributes
> +	  and kernel internal API to it. You'll need this to provide support
> +	  for multicolor LEDs that are grouped together. This class is not
> +	  intended for single color LEDs.  It can be built as a module.

extra whitespace:

s/ It can/It can/

[...]
> +
> +static int multicolor_set_brightness(struct led_classdev *led_cdev,
> +			     enum led_brightness brightness)
> +{
> +	struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> +	struct led_classdev_mc_data *data = mcled_cdev->data;
> +	struct led_multicolor_ops *ops = mcled_cdev->ops;
> +	struct led_classdev_mc_priv *priv;
> +	unsigned long state = brightness;
> +	int adj_value;
> +	ssize_t ret = -EINVAL;
> +
> +	mutex_lock(&led_cdev->led_access);
> +
> +	if (ops->set_module_brightness) {
> +		ret = ops->set_module_brightness(mcled_cdev, state);
> +		goto unlock;
> +	}
> +
> +	list_for_each_entry(priv, &data->color_list, list) {
> +		if (state && priv->brightness && priv->max_brightness) {
> +			adj_value = state * ((priv->brightness * 100) / priv->max_brightness);
> +			adj_value = adj_value / 100;

Why the multiplication an then division by 100? And priv->max_brightness
stays unaltered? This changes the proportions. My python script works
just fine without those.

> +		} else
> +			adj_value = LED_OFF;
> +
> +		ret = ops->set_color_brightness(priv->mcled_cdev,
> +						priv->color_id,	adj_value);
> +		if (ret < 0)
> +			goto unlock;
> +	}
> +
> +unlock:
> +	mutex_unlock(&led_cdev->led_access);
> +	return ret;
> +}
[...]
> +int led_classdev_multicolor_register_ext(struct device *parent,
> +				     struct led_classdev_mc *mcled_cdev,
> +				     struct led_init_data *init_data)
> +{
> +	struct led_classdev *led_cdev;
> +	struct led_multicolor_ops *ops;
> +	struct led_classdev_mc_data *data;
> +	int ret;
> +	int i;
> +
> +	if (!mcled_cdev)
> +		return -EINVAL;
> +
> +	ops = mcled_cdev->ops;
> +	if (!ops || !ops->set_color_brightness)
> +		return -EINVAL;
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	mcled_cdev->data = data;
> +	led_cdev = &mcled_cdev->led_cdev;
> +
> +	if (led_cdev->brightness_set_blocking)
> +		led_cdev->brightness_set_blocking = multicolor_set_brightness;

This is weird. In leds-lp50xx.c you don't initialize
brightness_set_blocking and this still works?

I believe this is kind of omission.

And it is not reasonable to just override driver supplied op with
generic one just like that.

I propose to initialize brightness_set or brightness_set_blocking
op as we used to do it for monochrome LEDs. Those function(s) on
driver side will either use device's hardware support for setting
color lightness, or will call a generic function provided by
LED multi color class for calculating intensities of all colors
it comprises in the cluster.

I know this is different to what we've discussed on IRC, but now
it looks for me the most reasonable way to go.

> +	INIT_LIST_HEAD(&data->color_list);
> +
> +	/* Register led class device */
> +	ret = led_classdev_register_ext(parent, led_cdev, init_data);
> +	if (ret)
> +		return ret;
> +
> +	ret = led_multicolor_init_color_dir(data, mcled_cdev);
> +	if (ret)
> +		return ret;
> +
> +	/* Select the sysfs attributes to be created for the device */
> +	for (i = 0; i < mcled_cdev->num_leds; i++) {
> +		ret = led_multicolor_init_color(data, mcled_cdev,
> +						mcled_cdev->available_colors[i]);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
> +
> +void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev)
> +{
> +	if (!mcled_cdev)
> +		return;
> +
> +	led_classdev_unregister(&mcled_cdev->led_cdev);
> +}
> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister);
> +
> +static void devm_led_classdev_multicolor_release(struct device *dev, void *res)
> +{
> +	led_classdev_multicolor_unregister(*(struct led_classdev_mc **)res);
> +}
> +
> +/**
> + * devm_of_led_classdev_register - resource managed led_classdev_register()
> + *
> + * @parent: parent of LED device
> + * @led_cdev: the led_classdev structure for this device.
> + */
> +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;
> +
> +	if (WARN_ON(!p || !*p))
> +		return 0;
> +
> +	return *p == data;
> +}
> +
> +/**
> + * devm_led_classdev_multicolor_unregister() - resource managed
> + *					led_classdev_multicolor_unregister()
> + * @parent: The device to unregister.
> + * @mcled_cdev: the led_classdev_mc structure for this device.
> + */
> +void devm_led_classdev_multicolor_unregister(struct device *dev,
> +				  struct led_classdev_mc *mcled_cdev)
> +{
> +	WARN_ON(devres_release(dev,
> +			       devm_led_classdev_multicolor_release,
> +			       devm_led_classdev_multicolor_match, mcled_cdev));
> +}
> +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_unregister);
> +
> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
> +MODULE_DESCRIPTION("Multi Color LED class interface");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h
> new file mode 100644
> index 000000000000..f9e71d984b03
> --- /dev/null
> +++ b/include/linux/led-class-multicolor.h
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/* LED Multicolor class interface
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + */

Let's have C++ comment style also here.

> +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED
> +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED
> +
> +#include <linux/leds.h>
> +#include <dt-bindings/leds/common.h>
[...]


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 6/9] leds: multicolor: Introduce a multicolor class definition

[Dan Murphy]

Jacek

Thanks for the review

On 6/20/19 11:10 AM, Jacek Anaszewski wrote:
> Hi Dan,
>
> Thank you for the v5.
>
> I will confine myself to commenting only some parts since
> the rest will undergo rework due to removal of sync API.
>
> On 5/23/19 9:08 PM, Dan Murphy wrote:
>> Introduce a multicolor class that groups colored LEDs
>> within a LED node.
>>
>> The framework allows for dynamically setting individual LEDs
>> or setting brightness levels of LEDs and updating them virtually
>> simultaneously.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   drivers/leds/Kconfig                 |  10 +
>>   drivers/leds/Makefile                |   1 +
>>   drivers/leds/led-class-multicolor.c  | 421 +++++++++++++++++++++++++++
>>   include/linux/led-class-multicolor.h |  95 ++++++
>>   4 files changed, 527 insertions(+)
>>   create mode 100644 drivers/leds/led-class-multicolor.c
>>   create mode 100644 include/linux/led-class-multicolor.h
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 0414adebb177..0696a13c9527 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -29,6 +29,16 @@ config LEDS_CLASS_FLASH
>>         for the flash related features of a LED device. It can be built
>>         as a module.
>>   +config LEDS_CLASS_MULTI_COLOR
>> +    tristate "LED Mulit Color LED Class Support"
>> +    depends on LEDS_CLASS
>> +    help
>> +      This option enables the multicolor LED sysfs class in 
>> /sys/class/leds.
>> +      It wraps LED Class and adds multicolor LED specific sysfs 
>> attributes
>> +      and kernel internal API to it. You'll need this to provide 
>> support
>> +      for multicolor LEDs that are grouped together. This class is not
>> +      intended for single color LEDs.  It can be built as a module.
>
> extra whitespace:
>
> s/ It can/It can/

Ack

>
> [...]
>> +
>> +static int multicolor_set_brightness(struct led_classdev *led_cdev,
>> +                 enum led_brightness brightness)
>> +{
>> +    struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
>> +    struct led_classdev_mc_data *data = mcled_cdev->data;
>> +    struct led_multicolor_ops *ops = mcled_cdev->ops;
>> +    struct led_classdev_mc_priv *priv;
>> +    unsigned long state = brightness;
>> +    int adj_value;
>> +    ssize_t ret = -EINVAL;
>> +
>> +    mutex_lock(&led_cdev->led_access);
>> +
>> +    if (ops->set_module_brightness) {
>> +        ret = ops->set_module_brightness(mcled_cdev, state);
>> +        goto unlock;
>> +    }
>> +
>> +    list_for_each_entry(priv, &data->color_list, list) {
>> +        if (state && priv->brightness && priv->max_brightness) {
>> +            adj_value = state * ((priv->brightness * 100) / 
>> priv->max_brightness);
>> +            adj_value = adj_value / 100;
>
> Why the multiplication an then division by 100? And priv->max_brightness
> stays unaltered? This changes the proportions. My python script works
> just fine without those.

Because the kernel does not do floating point math and the calculation 
is using the ratio

between the intensity and max_intensity and multiplying against the 
requested brightness.

priv->intensity = 100 (This is the current intensity of the color LED)

priv->max_intensity = 255

state = 80 (This is the requested cluster brightness)

100/255 = 0.392 which is 0.

0 * 80 = 0 this is not what the value should be

But with the multiplier.

10000/255 = 39.2 which is 39 which means that the intensity is only 39% 
of the

max_intensity.

39 * 80 = 3120  So to preserve the 39% from the 80 we multiply the 
percentage * requested cluster brightness

3120 / 100 = 31 then we normalize back

I am not sure how your script is working without the multiplier.


>
>> +        } else
>> +            adj_value = LED_OFF;
>> +
>> +        ret = ops->set_color_brightness(priv->mcled_cdev,
>> +                        priv->color_id,    adj_value);
>> +        if (ret < 0)
>> +            goto unlock;
>> +    }
>> +
>> +unlock:
>> +    mutex_unlock(&led_cdev->led_access);
>> +    return ret;
>> +}
> [...]
>> +int led_classdev_multicolor_register_ext(struct device *parent,
>> +                     struct led_classdev_mc *mcled_cdev,
>> +                     struct led_init_data *init_data)
>> +{
>> +    struct led_classdev *led_cdev;
>> +    struct led_multicolor_ops *ops;
>> +    struct led_classdev_mc_data *data;
>> +    int ret;
>> +    int i;
>> +
>> +    if (!mcled_cdev)
>> +        return -EINVAL;
>> +
>> +    ops = mcled_cdev->ops;
>> +    if (!ops || !ops->set_color_brightness)
>> +        return -EINVAL;
>> +
>> +    data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    mcled_cdev->data = data;
>> +    led_cdev = &mcled_cdev->led_cdev;
>> +
>> +    if (led_cdev->brightness_set_blocking)
>> +        led_cdev->brightness_set_blocking = multicolor_set_brightness;
>
> This is weird. In leds-lp50xx.c you don't initialize
> brightness_set_blocking and this still works?

I will have to look.  I don't believe I retested this on lp50xx only the 
lp55xx code.

>
> I believe this is kind of omission.
>
> And it is not reasonable to just override driver supplied op with
> generic one just like that.
>
> I propose to initialize brightness_set or brightness_set_blocking
> op as we used to do it for monochrome LEDs. Those function(s) on
> driver side will either use device's hardware support for setting
> color lightness, or will call a generic function provided by
> LED multi color class for calculating intensities of all colors
> it comprises in the cluster.
>
> I know this is different to what we've discussed on IRC, but now
> it looks for me the most reasonable way to go.

So you want the device driver to handle the brightness request and call 
into the framework for

calculating the color intensities?

That would work as well and solves a problem of HW supported brightness 
control like the LP50xx.

The LP50xx would not need to call into the function for calculated 
intensities.


>
>> + INIT_LIST_HEAD(&data->color_list);
>> +
>> +    /* Register led class device */
>> +    ret = led_classdev_register_ext(parent, led_cdev, init_data);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = led_multicolor_init_color_dir(data, mcled_cdev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Select the sysfs attributes to be created for the device */
>> +    for (i = 0; i < mcled_cdev->num_leds; i++) {
>> +        ret = led_multicolor_init_color(data, mcled_cdev,
>> +                        mcled_cdev->available_colors[i]);
>> +        if (ret)
>> +            break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext);
>> +
>> +void led_classdev_multicolor_unregister(struct led_classdev_mc 
>> *mcled_cdev)
>> +{
>> +    if (!mcled_cdev)
>> +        return;
>> +
>> +    led_classdev_unregister(&mcled_cdev->led_cdev);
>> +}
>> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister);
>> +
>> +static void devm_led_classdev_multicolor_release(struct device *dev, 
>> void *res)
>> +{
>> +    led_classdev_multicolor_unregister(*(struct led_classdev_mc 
>> **)res);
>> +}
>> +
>> +/**
>> + * devm_of_led_classdev_register - resource managed 
>> led_classdev_register()
>> + *
>> + * @parent: parent of LED device
>> + * @led_cdev: the led_classdev structure for this device.
>> + */
>> +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;
>> +
>> +    if (WARN_ON(!p || !*p))
>> +        return 0;
>> +
>> +    return *p == data;
>> +}
>> +
>> +/**
>> + * devm_led_classdev_multicolor_unregister() - resource managed
>> + *                    led_classdev_multicolor_unregister()
>> + * @parent: The device to unregister.
>> + * @mcled_cdev: the led_classdev_mc structure for this device.
>> + */
>> +void devm_led_classdev_multicolor_unregister(struct device *dev,
>> +                  struct led_classdev_mc *mcled_cdev)
>> +{
>> +    WARN_ON(devres_release(dev,
>> +                   devm_led_classdev_multicolor_release,
>> +                   devm_led_classdev_multicolor_match, mcled_cdev));
>> +}
>> +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_unregister);
>> +
>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
>> +MODULE_DESCRIPTION("Multi Color LED class interface");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/led-class-multicolor.h 
>> b/include/linux/led-class-multicolor.h
>> new file mode 100644
>> index 000000000000..f9e71d984b03
>> --- /dev/null
>> +++ b/include/linux/led-class-multicolor.h
>> @@ -0,0 +1,95 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/* LED Multicolor class interface
>> + * Copyright (C) 2019 Texas Instruments Incorporated - 
>> http://www.ti.com/
>> + */
>
> Let's have C++ comment style also here.
>
ACK


>> +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED
>> +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED
>> +
>> +#include <linux/leds.h>
>> +#include <dt-bindings/leds/common.h>
> [...]
>
>

Re: [PATCH v3 6/9] leds: multicolor: Introduce a multicolor class definition

[Jacek Anaszewski]

Dan,

On 6/20/19 10:06 PM, Dan Murphy wrote:
> Jacek
> 
> Thanks for the review

You're welcome.

> On 6/20/19 11:10 AM, Jacek Anaszewski wrote:
>> Hi Dan,
>>
>> Thank you for the v5.
>>
>> I will confine myself to commenting only some parts since
>> the rest will undergo rework due to removal of sync API.
>>
>> On 5/23/19 9:08 PM, Dan Murphy wrote:
>>> Introduce a multicolor class that groups colored LEDs
>>> within a LED node.
>>>
>>> The framework allows for dynamically setting individual LEDs
>>> or setting brightness levels of LEDs and updating them virtually
>>> simultaneously.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>   drivers/leds/Kconfig                 |  10 +
>>>   drivers/leds/Makefile                |   1 +
>>>   drivers/leds/led-class-multicolor.c  | 421 +++++++++++++++++++++++++++
>>>   include/linux/led-class-multicolor.h |  95 ++++++
>>>   4 files changed, 527 insertions(+)
>>>   create mode 100644 drivers/leds/led-class-multicolor.c
>>>   create mode 100644 include/linux/led-class-multicolor.h
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 0414adebb177..0696a13c9527 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -29,6 +29,16 @@ config LEDS_CLASS_FLASH
>>>         for the flash related features of a LED device. It can be built
>>>         as a module.
>>>   +config LEDS_CLASS_MULTI_COLOR
>>> +    tristate "LED Mulit Color LED Class Support"
>>> +    depends on LEDS_CLASS
>>> +    help
>>> +      This option enables the multicolor LED sysfs class in 
>>> /sys/class/leds.
>>> +      It wraps LED Class and adds multicolor LED specific sysfs 
>>> attributes
>>> +      and kernel internal API to it. You'll need this to provide 
>>> support
>>> +      for multicolor LEDs that are grouped together. This class is not
>>> +      intended for single color LEDs.  It can be built as a module.
>>
>> extra whitespace:
>>
>> s/ It can/It can/
> 
> Ack
> 
>>
>> [...]
>>> +
>>> +static int multicolor_set_brightness(struct led_classdev *led_cdev,
>>> +                 enum led_brightness brightness)
>>> +{
>>> +    struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
>>> +    struct led_classdev_mc_data *data = mcled_cdev->data;
>>> +    struct led_multicolor_ops *ops = mcled_cdev->ops;
>>> +    struct led_classdev_mc_priv *priv;
>>> +    unsigned long state = brightness;
>>> +    int adj_value;
>>> +    ssize_t ret = -EINVAL;
>>> +
>>> +    mutex_lock(&led_cdev->led_access);
>>> +
>>> +    if (ops->set_module_brightness) {
>>> +        ret = ops->set_module_brightness(mcled_cdev, state);
>>> +        goto unlock;
>>> +    }
>>> +
>>> +    list_for_each_entry(priv, &data->color_list, list) {
>>> +        if (state && priv->brightness && priv->max_brightness) {
>>> +            adj_value = state * ((priv->brightness * 100) / 
>>> priv->max_brightness);
>>> +            adj_value = adj_value / 100;
>>
>> Why the multiplication an then division by 100? And priv->max_brightness
>> stays unaltered? This changes the proportions. My python script works
>> just fine without those.
> 
> Because the kernel does not do floating point math and the calculation 
> is using the ratio
> 
> between the intensity and max_intensity and multiplying against the 
> requested brightness.
> 
> priv->intensity = 100 (This is the current intensity of the color LED)
> 
> priv->max_intensity = 255
> 
> state = 80 (This is the requested cluster brightness)
> 
> 100/255 = 0.392 which is 0.
> 
> 0 * 80 = 0 this is not what the value should be
> 
> But with the multiplier.
> 
> 10000/255 = 39.2 which is 39 which means that the intensity is only 39% 
> of the
> 
> max_intensity.
> 
> 39 * 80 = 3120  So to preserve the 39% from the 80 we multiply the 
> percentage * requested cluster brightness
> 
> 3120 / 100 = 31 then we normalize back
> 
> I am not sure how your script is working without the multiplier.

Try to remove brackets around division operation.
Then first we are multiplying and only after that dividing.

$ echo "80 * 100 / 255" | bc
31

>>> +        } else
>>> +            adj_value = LED_OFF;
>>> +
>>> +        ret = ops->set_color_brightness(priv->mcled_cdev,
>>> +                        priv->color_id,    adj_value);
>>> +        if (ret < 0)
>>> +            goto unlock;
>>> +    }
>>> +
>>> +unlock:
>>> +    mutex_unlock(&led_cdev->led_access);
>>> +    return ret;
>>> +}
>> [...]
>>> +int led_classdev_multicolor_register_ext(struct device *parent,
>>> +                     struct led_classdev_mc *mcled_cdev,
>>> +                     struct led_init_data *init_data)
>>> +{
>>> +    struct led_classdev *led_cdev;
>>> +    struct led_multicolor_ops *ops;
>>> +    struct led_classdev_mc_data *data;
>>> +    int ret;
>>> +    int i;
>>> +
>>> +    if (!mcled_cdev)
>>> +        return -EINVAL;
>>> +
>>> +    ops = mcled_cdev->ops;
>>> +    if (!ops || !ops->set_color_brightness)
>>> +        return -EINVAL;
>>> +
>>> +    data = kzalloc(sizeof(*data), GFP_KERNEL);
>>> +    if (!data)
>>> +        return -ENOMEM;
>>> +
>>> +    mcled_cdev->data = data;
>>> +    led_cdev = &mcled_cdev->led_cdev;
>>> +
>>> +    if (led_cdev->brightness_set_blocking)
>>> +        led_cdev->brightness_set_blocking = multicolor_set_brightness;
>>
>> This is weird. In leds-lp50xx.c you don't initialize
>> brightness_set_blocking and this still works?
> 
> I will have to look.  I don't believe I retested this on lp50xx only the 
> lp55xx code.
> 
>>
>> I believe this is kind of omission.
>>
>> And it is not reasonable to just override driver supplied op with
>> generic one just like that.
>>
>> I propose to initialize brightness_set or brightness_set_blocking
>> op as we used to do it for monochrome LEDs. Those function(s) on
>> driver side will either use device's hardware support for setting
>> color lightness, or will call a generic function provided by
>> LED multi color class for calculating intensities of all colors
>> it comprises in the cluster.
>>
>> I know this is different to what we've discussed on IRC, but now
>> it looks for me the most reasonable way to go.
> 
> So you want the device driver to handle the brightness request and call 
> into the framework for
> 
> calculating the color intensities?

Exactly.

> That would work as well and solves a problem of HW supported brightness 
> control like the LP50xx.
> 
> The LP50xx would not need to call into the function for calculated 
> intensities.

True.

-- 
Best regards,
Jacek Anaszewski