[RFC PATCH 0/5] MultiColor LED framework Documentation

[RFC PATCH 0/5] MultiColor LED framework Documentation

[Dan Murphy]

I am RFCing these patches to start this Multicolor framework as it appears we
have a few devices that need to leverage a framework like this.

I have attempted to create as much documentation as possible and have no issue
in combining documents into a single document but I think this patchset is logical.

I have also added the dt-bindings patch that include the COLOR_ID and COLOR_NAME
and documented the existing patch on the mail list.  I have added this only for
demonstration purposes.

Finally the code itself is a work in progress that at the very least will create
a colors directory as well as the associated colors.  The code works in principle
but I need to scrub it for issues.  The framework code also does not contain the
available brightness support as this seems to be a sticky point.

Dan

Dan Murphy (5):
  leds: multicolor: Add sysfs interface definition
  dt: bindings: Add multicolor class dt bindings documention
  documention: leds: Add multicolor class documentation
  dt-bindings: leds: Add LED_COLOR_ID and COLOR_NAME definitions
  leds: multicolor: Introduce a multicolor class definition

 .../ABI/testing/sysfs-class-led-multicolor    |  91 ++++
 .../bindings/leds/leds-class-multicolor.txt   | 140 ++++++
 Documentation/leds/leds-class-multicolor.txt  |  99 +++++
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/led-class-multicolor.c           | 411 ++++++++++++++++++
 include/dt-bindings/leds/common.h             |  19 +
 include/linux/led-class-multicolor.h          |  69 +++
 8 files changed, 840 insertions(+)
 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/leds/leds-class-multicolor.txt
 create mode 100644 drivers/leds/led-class-multicolor.c
 create mode 100644 include/linux/led-class-multicolor.h

-- 
2.19.0

[RFC PATCH 1/5] 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    | 91 +++++++++++++++++++
 1 file changed, 91 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..5583ecaa3170
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
@@ -0,0 +1,91 @@
+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.
+
+What:		/sys/class/leds/<led>/brightness_model/<model_color>
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Jacek Anaszewski <j.anaszewski@samsung.com>
+Description:	read/write
+		This directory contains the defined color models that are read
+		from the firmware node.  The model color will be the read from
+		the firmware node.  The model names should be a color that the
+		LED cluster can produce.
+
+What:		/sys/class/leds/<led>/brightness_model/<model_color>/brightness
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Jacek Anaszewski <j.anaszewski@samsung.com>
+Description:	read/write
+		Writing this file will initiate a synchronized write to the LED
+		cluster to produce the color output defined in the firmware node.
+
+		This value cannot exceed <model_color>/max_brightness.
+		Writing a 0 to this file will cause the LED cluster to turn off.
+
+		The sync and sync_enable files do not have an affect on this as
+		this is a real time update of the LED cluster.
+
+What:		/sys/class/leds/<led>/brightness_model/<model_color>/max_brightness
+Date:		April 2019
+KernelVersion:	5.2
+Contact:	Jacek Anaszewski <j.anaszewski@samsung.com>
+Description:	read
+		Reading this file will return the maximum number of brightness
+		levels that are defined in the firmware node for the model color.
+		It should be assumed that the levels are from 1 -> max_brightness
+		and the LED cluster brightness is increasing.
-- 
2.19.0

[RFC PATCH 2/5] 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   | 140 ++++++++++++++++++
 1 file changed, 140 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..4b1a26104c79
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
@@ -0,0 +1,140 @@
+* 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 : This is the color ID of the LED.  Definitions can be found
+		  in include/linux/leds/common.txt
+
+Optional LED Child properties:
+	- available-brightness-models : This is the phandle to the brightness-model
+					node(s) that this LED cluster can support.
+
+Required Brightness model properties
+	- led-brightness-model : This flag alerts the device driver and class
+				 code that this node is a brightness model node
+				 and to process the properties differently.
+
+Required Brightness model child properties
+	- model_name : This is the name of the model presented to the user.  This
+		       should be a color that the LED cluster can produce for
+		       the device it is attached to.
+	- layout : This is the LED layout for the levels.  This layout will
+		   determine the color order of the levels.  The layout and
+		   level-x properties array should be the same size.
+	- level-x : These are the values for the LEDs to produce the color that
+		    is defined.  These values are placed in the array according
+		    to the layout property.
+
+led-controller@30 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "ti,lp5024";
+	reg = <0x29>;
+
+	lp5024_model_yellow: brightness-models {
+		led-brightness-model;
+		model@0 {
+			model_name = "yellow";
+			layout = <LED_COLOR_ID_RED
+				  LED_COLOR_ID_GREEN
+				  LED_COLOR_ID_BLUE>;
+			level-1 = <255 227 40>;
+			level-2 = <255 240 136>;
+			level-3 = <255 247 196>;
+		};
+	};
+
+	lp5024_model_orange: brightness-models {
+		led-brightness-model;
+		model@1 {
+			model_name = "orange";
+			layout = <LED_COLOR_ID_RED
+				  LED_COLOR_ID_GREEN
+				  LED_COLOR_ID_BLUE>;
+			level-1 = <236 140 16>;
+			level-2 = <236 157 55>;
+			level-3 = <236 183 115>;
+		};
+	}; 
+
+	multi-led@4 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <4>;
+		label = "rgb:led_mod4";
+		available-brightness-models = <&lp5024_model_yellow>;
+		
+		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>;
+		label = "rgb:led_mod1";
+		
+		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>;
+		label = "rgb:banks";
+		reg = <2>;
+		ti,led-bank = <2 3 5>;
+		available-brightness-models = <&lp5024_model_orange>;
+
+		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.19.0

[RFC PATCH 3/5] 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..8112b99a7668
--- /dev/null
+++ b/Documentation/leds/leds-class-multicolor.txt
@@ -0,0 +1,99 @@
+
+Multi Color LED handling under Linux
+=================================================
+
+Authors: 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 but the led_class
+framework.  The led_class framework is documented in led-class.txt within this
+documentation directory. 
+
+Each colored LED is given it's 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.19.0

[RFC PATCH 4/5] dt-bindings: leds: Add LED_COLOR_ID and COLOR_NAME definitions

[Dan Murphy]

This patch is being added to demonstrate reuse of Jacek's patch
https://lore.kernel.org/patchwork/patch/1056726/

I have added color name definitions to the above patch.
I do not claim authorship and the intent of this patch to strictly
for demonstration.

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

diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index e171d0a6beb2..7e1279d761e4 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -19,4 +19,23 @@
 #define LEDS_BOOST_ADAPTIVE	1
 #define LEDS_BOOST_FIXED	2
 
+#define LED_COLOR_ID_WHITE	 0
+#define LED_COLOR_ID_RED	 1
+#define LED_COLOR_ID_GREEN	 2
+#define LED_COLOR_ID_BLUE	 3
+#define LED_COLOR_ID_AMBER	 4
+#define LED_COLOR_ID_VIOLET	 5
+#define LED_COLOR_ID_YELLOW	 6
+#define LED_COLOR_ID_MAX	 LED_COLOR_ID_YELLOW
+
+#define LED_COLOR_NAME_WHITE	"white"
+#define LED_COLOR_NAME_RED	"red"
+#define LED_COLOR_NAME_GREEN	"green"
+#define LED_COLOR_NAME_BLUE	"blue"
+#define LED_COLOR_NAME_AMBER	"amber"
+#define LED_COLOR_NAME_VIOLET	"violet"
+#define LED_COLOR_NAME_YELLOW	"yellow"
+
+#define LED_COLOR_NAME_MAX_SZ	6
+
 #endif /* __DT_BINDINGS_LEDS_H */
-- 
2.19.0

[RFC PATCH 5/5] 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  | 411 +++++++++++++++++++++++++++
 include/linux/led-class-multicolor.h |  69 +++++
 4 files changed, 491 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 a72f97fca57b..477e108fd2a7 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 flash led sysfs class in /sys/class/leds.
+	  It wrapps LED Class and adds flash LEDs specific sysfs attributes
+	  and kernel internal API to it. You'll need this to provide support
+	  for the flash related features of a LED device. 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..d54a884eaf09
--- /dev/null
+++ b/drivers/leds/led-class-multicolor.c
@@ -0,0 +1,411 @@
+// 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/leds.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include "leds.h"
+
+const char *led_colors[LED_COLOR_ID_MAX + 1] = {
+	[LED_COLOR_ID_WHITE] = LED_COLOR_NAME_WHITE,
+	[LED_COLOR_ID_RED] = LED_COLOR_NAME_RED,
+	[LED_COLOR_ID_GREEN] = LED_COLOR_NAME_GREEN,
+	[LED_COLOR_ID_BLUE] = LED_COLOR_NAME_BLUE,
+	[LED_COLOR_ID_AMBER] = LED_COLOR_NAME_AMBER,
+	[LED_COLOR_ID_VIOLET] = LED_COLOR_NAME_VIOLET,
+	[LED_COLOR_ID_YELLOW] = LED_COLOR_NAME_YELLOW,
+};
+
+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_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;
+	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;
+	const struct led_multicolor_ops *ops = mcled_cdev->ops;
+	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) {
+		ret = ops->set_color_brightness(priv->mcled_cdev,
+						priv->color_id,
+						priv->brightness);
+		if (ret < 0)
+			goto unlock;
+	}
+
+	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 = -EINVAL;
+
+	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_multicolor_ops *ops = priv->mcled_cdev->ops;
+	struct led_classdev *led_cdev = &priv->mcled_cdev->led_cdev;
+
+	int old_brightness;
+	unsigned long value;
+	ssize_t ret = -EINVAL;
+
+	mutex_lock(&led_cdev->led_access);
+
+	ret = kstrtoul(buf, 10, &value);
+	if (ret)
+		goto unlock;
+
+	if (value > priv->max_brightness) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
+	/* Retain the current brightness in case writing the LED fails */
+	old_brightness = priv->brightness;
+	priv->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) {
+		priv->brightness = old_brightness;
+		goto unlock;
+	}
+
+	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 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_IRUSR | S_IWUSR;
+	mc_priv->brightness_attr.show = brightness_show;
+	mc_priv->brightness_attr.store = brightness_store;
+	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_IRUSR;
+	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_IRUSR | S_IWUSR;
+	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_IWUSR;
+	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(struct device *parent,
+				     struct led_classdev_mc *mcled_cdev)
+{
+	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;
+
+	led_cdev = &mcled_cdev->led_cdev;
+
+	INIT_LIST_HEAD(&data->color_list);
+
+	/* Register led class device */
+	ret = led_classdev_register(parent, led_cdev);
+	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_of_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);
+
+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..8ca33ffe4493
--- /dev/null
+++ b/include/linux/led-class-multicolor.h
@@ -0,0 +1,69 @@
+// 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_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);
+};
+
+struct led_classdev_mc {
+	/* led class device */
+	struct led_classdev led_cdev;
+
+	/* multicolor led specific ops */
+	struct led_multicolor_ops *ops;
+
+	u32 available_colors[LED_COLOR_ID_MAX];
+	int num_of_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 - 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
+ *
+ * Returns: 0 on success or negative error value on failure
+ */
+extern int led_classdev_multicolor_register(struct device *parent,
+					    struct led_classdev_mc *mcled_cdev);
+
+/**
+ * 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.19.0

Re: [RFC PATCH 5/5] leds: multicolor: Introduce a multicolor class definition

[Randy Dunlap]

Hi,

On 4/1/19 10:34 AM, 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  | 411 +++++++++++++++++++++++++++
>  include/linux/led-class-multicolor.h |  69 +++++
>  4 files changed, 491 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 a72f97fca57b..477e108fd2a7 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"

	              Multi

> +	depends on LEDS_CLASS
> +	help
> +	  This option enables the flash led sysfs class in /sys/class/leds.

	                                LED (?)

> +	  It wrapps LED Class and adds flash LEDs specific sysfs attributes

	     wraps

> +	  and kernel internal API to it. You'll need this to provide support
> +	  for the flash related features of a LED device. It can be built

	                                 of an LED device.

> +	  as a module.
> +
>  config LEDS_BRIGHTNESS_HW_CHANGED
>  	bool "LED Class brightness_hw_changed attribute support"
>  	depends on LEDS_CLASS


ciao.
-- 
~Randy

Re: [RFC PATCH 3/5] documention: leds: Add multicolor class documentation

[Randy Dunlap]

Hi,

On 4/1/19 10:33 AM, Dan Murphy wrote:
> Add the support documentation on the multicolor LED framework.
> This document defines the directores and file generated by the

                            directories and files

> 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..8112b99a7668
> --- /dev/null
> +++ b/Documentation/leds/leds-class-multicolor.txt
> @@ -0,0 +1,99 @@
> +
> +Multi Color LED handling under Linux
> +=================================================
> +
> +Authors: Dan Murphy <dmurphy@ti.com>

or Author:

> +
> +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 but the led_class

                                                       created by

> +framework.  The led_class framework is documented in led-class.txt within this
> +documentation directory. 
> +
> +Each colored LED is given it's own directory.  These directories can be but not

                             its                                    huh???????????

> +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

                                                    created:

> +"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

                                updated; the LEDs

> +* 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
> 

cheers.
-- 
~Randy

Re: [RFC PATCH 3/5] documention: leds: Add multicolor class documentation

[Pavel Machek]

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

Hi!

> +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.

I believe better solutions exists for this problem.

One was discussed before -- have single file which contains
coefficients for r/g/b channels.

Or at least... you should not really need separate sync and
sync_enable files. One should do.

									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: [RFC PATCH 2/5] dt: bindings: Add multicolor class dt bindings documention

[Pavel Machek]

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

Hi!


>  .../bindings/leds/leds-class-multicolor.txt   | 140 ++++++++++++++++++
>  1 file changed, 140 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..4b1a26104c79
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
> @@ -0,0 +1,140 @@
> +* 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 : This is the color ID of the LED.  Definitions can be found
> +		  in include/linux/leds/common.txt
> +
> +Optional LED Child properties:
> +	- available-brightness-models : This is the phandle to the brightness-model
> +					node(s) that this LED cluster can support.
> +
> +Required Brightness model properties
> +	- led-brightness-model : This flag alerts the device driver and class
> +				 code that this node is a brightness model node
> +				 and to process the properties differently.
> +
> +Required Brightness model child properties
> +	- model_name : This is the name of the model presented to the user.  This
> +		       should be a color that the LED cluster can produce for
> +		       the device it is attached to.
> +	- layout : This is the LED layout for the levels.  This layout will
> +		   determine the color order of the levels.  The layout and
> +		   level-x properties array should be the same size.
> +	- level-x : These are the values for the LEDs to produce the color that
> +		    is defined.  These values are placed in the array according
> +		    to the layout property.

> +led-controller@30 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "ti,lp5024";
> +	reg = <0x29>;
> +
> +	lp5024_model_yellow: brightness-models {
> +		led-brightness-model;
> +		model@0 {
> +			model_name = "yellow";
> +			layout = <LED_COLOR_ID_RED
> +				  LED_COLOR_ID_GREEN
> +				  LED_COLOR_ID_BLUE>;
> +			level-1 = <255 227 40>;
> +			level-2 = <255 240 136>;
> +			level-3 = <255 247 196>;
> +		};
> +	};

I don't think this works. RGB LED can show millions of colors. Do you
propose to have millions of entries in dts?
									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: [RFC PATCH 5/5] leds: multicolor: Introduce a multicolor class definition

[Marek Behun]

Hi Dan,

On Mon, 1 Apr 2019 12:34:00 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> +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;
> +	const struct led_multicolor_ops *ops = mcled_cdev->ops;
> +	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;

This lock is redundant, you could just put
  if (mcled_cdev->sync_enabled)
    return ret;
before the lock.

Re: [RFC PATCH 3/5] documention: leds: Add multicolor class documentation

[Marek Behun]

On Mon, 1 Apr 2019 23:27:10 +0200
Pavel Machek <pavel@ucw.cz> wrote:

> One was discussed before -- have single file which contains
> coefficients for r/g/b channels.

That would somehow break the rule one file/one value. Although you
could consider the whole color one value, that way it would not be
broken...

Re: [RFC PATCH 2/5] dt: bindings: Add multicolor class dt bindings documention

[Dan Murphy]

Pavel

On 4/1/19 4:29 PM, Pavel Machek wrote:
> Hi!
> 
> 
>>  .../bindings/leds/leds-class-multicolor.txt   | 140 ++++++++++++++++++
>>  1 file changed, 140 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..4b1a26104c79
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>> @@ -0,0 +1,140 @@
>> +* 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 : This is the color ID of the LED.  Definitions can be found
>> +		  in include/linux/leds/common.txt
>> +
>> +Optional LED Child properties:
>> +	- available-brightness-models : This is the phandle to the brightness-model
>> +					node(s) that this LED cluster can support.
>> +
>> +Required Brightness model properties
>> +	- led-brightness-model : This flag alerts the device driver and class
>> +				 code that this node is a brightness model node
>> +				 and to process the properties differently.
>> +
>> +Required Brightness model child properties
>> +	- model_name : This is the name of the model presented to the user.  This
>> +		       should be a color that the LED cluster can produce for
>> +		       the device it is attached to.
>> +	- layout : This is the LED layout for the levels.  This layout will
>> +		   determine the color order of the levels.  The layout and
>> +		   level-x properties array should be the same size.
>> +	- level-x : These are the values for the LEDs to produce the color that
>> +		    is defined.  These values are placed in the array according
>> +		    to the layout property.
> 
>> +led-controller@30 {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	compatible = "ti,lp5024";
>> +	reg = <0x29>;
>> +
>> +	lp5024_model_yellow: brightness-models {
>> +		led-brightness-model;
>> +		model@0 {
>> +			model_name = "yellow";
>> +			layout = <LED_COLOR_ID_RED
>> +				  LED_COLOR_ID_GREEN
>> +				  LED_COLOR_ID_BLUE>;
>> +			level-1 = <255 227 40>;
>> +			level-2 = <255 240 136>;
>> +			level-3 = <255 247 196>;
>> +		};
>> +	};
> 
> I don't think this works. RGB LED can show millions of colors. Do you
> propose to have millions of entries in dts?
> 									Pavel
> 

I have had off line conversations with Jacek about this brightness model node.

Your concern was actually one of my concerns as well.  Not only millions of entries but also having a huge
DT binary.

We wanted to RFC this to get feedback.  And this is why I have not added any support for this in the framework code.

Dan

-- 
------------------
Dan Murphy

Re: [RFC PATCH 3/5] documention: leds: Add multicolor class documentation

[Dan Murphy]

Pavel

Thanks for the comments

On 4/1/19 4:27 PM, Pavel Machek wrote:
> Hi!
> 
>> +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.
> 
> I believe better solutions exists for this problem.
> 

I agree there are other solutions.  Better not so sure.
We keep kicking a solution down the road we have been talking about adding something to the kernel
for almost 5 months now, but no one can decide what to do.

How do we move a solution forward?
Should we just forget the whole idea and keep developing against the current framework?

> One was discussed before -- have single file which contains
> coefficients for r/g/b channels.
> 

That has been presented as well and the concept was not well received either.

> Or at least... you should not really need separate sync and
> sync_enable files. One should do.
> 

The idea here was to be able to set the LED brightness immediately on a single LED
with a single brightness write or setup the color brightness on all the LEDs and then
sync the LED brightnesses.

If you wanted to control a single LED for notifications the user space may have to do

echo 0 > blue/brightness
echo 0 > green/brightness
echo 255 > red/brightness
echo 1 > sync

As opposed to just
echo 255 > red/brightness

But if that is not a concern then removing the sync_enable is fine with me from a code standpoint
but not from a user space side.

Dan

> 									Pavel
> 


-- 
------------------
Dan Murphy

Re: [RFC PATCH 5/5] leds: multicolor: Introduce a multicolor class definition

[Dan Murphy]

Marek

On 4/2/19 12:44 AM, Marek Behun wrote:
> Hi Dan,
> 
> On Mon, 1 Apr 2019 12:34:00 -0500
> Dan Murphy <dmurphy@ti.com> wrote:
> 
>> +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;
>> +	const struct led_multicolor_ops *ops = mcled_cdev->ops;
>> +	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;
> 
> This lock is redundant, you could just put
>   if (mcled_cdev->sync_enabled)
>     return ret;
> before the lock.
> 

Ack

Thanks the code is just proof of concept code.  But I will make this change as well.

Dan

-- 
------------------
Dan Murphy

Re: [RFC PATCH 3/5] documention: leds: Add multicolor class documentation

[Marek Behun]

Hi

On Tue, 2 Apr 2019 06:53:30 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> I agree there are other solutions.  Better not so sure.
> We keep kicking a solution down the road we have been talking about adding something to the kernel
> for almost 5 months now, but no one can decide what to do.
> 
> How do we move a solution forward?
> Should we just forget the whole idea and keep developing against the current framework?
> 
> > One was discussed before -- have single file which contains
> > coefficients for r/g/b channels.
> >   
> 
> That has been presented as well and the concept was not well received either.
> 
> > Or at least... you should not really need separate sync and
> > sync_enable files. One should do.
> >   
> 
> The idea here was to be able to set the LED brightness immediately on a single LED
> with a single brightness write or setup the color brightness on all the LEDs and then
> sync the LED brightnesses.
> 
> If you wanted to control a single LED for notifications the user space may have to do
> 
> echo 0 > blue/brightness
> echo 0 > green/brightness
> echo 255 > red/brightness
> echo 1 > sync
> 
> As opposed to just
> echo 255 > red/brightness
> 

Maybe other kernel developers, or sysfs maintainers like Greg, schould
also have a word about this.

Another solution, although not usable from shell, could be this:
  if a process opens all brightness files of a singled multicolor LED
  and then does a special SYNC_ON_THIS ioctl on one of them, then the
  sync is done when this color is written.
But that would also require ioctl for sysfs files, and I don't think
Greg would like that.

Dan's sync_enable API looks right to me from the one file/one value
sysfs rule as well - you can enable this setting (sync_enable) via one
file.

What is the main reason you guys are concerned about this?
That users will complain that they are writing to red/brightness and
the LED does not change color (because they did not disable
sync_enable)?

Marek

Re: [RFC PATCH 2/5] dt: bindings: Add multicolor class dt bindings documention

[Marek Behun]

On Tue, 2 Apr 2019 06:40:53 -0500
Dan Murphy <dmurphy@ti.com> wrote:

> I have had off line conversations with Jacek about this brightness model node.
> 
> Your concern was actually one of my concerns as well.  Not only millions of entries but also having a huge
> DT binary.
> 
> We wanted to RFC this to get feedback.  And this is why I have not added any support for this in the framework code.
> 
> Dan
> 

What if we just stuck to color spaces and defined channel curves in the
device tree?

The curve could then be defined via an array of integer pairs, which
would be interpreted as points via which the curve passes and the curve
would be approximated by linear segemts...

  led@0 {
    colorspace = <COLOR_SPACE_ID_RGB>;
    red-curve = <0 0>, <255 255>;
    green-curve = <0 0>, <255 128>;
    blue-curve = <0 0>, <255 128>;
  };

Re: [RFC PATCH 2/5] dt: bindings: Add multicolor class dt bindings documention

[Dan Murphy]

Marek

On 4/2/19 11:17 AM, Marek Behun wrote:
> On Tue, 2 Apr 2019 06:40:53 -0500
> Dan Murphy <dmurphy@ti.com> wrote:
> 
>> I have had off line conversations with Jacek about this brightness model node.
>>
>> Your concern was actually one of my concerns as well.  Not only millions of entries but also having a huge
>> DT binary.
>>
>> We wanted to RFC this to get feedback.  And this is why I have not added any support for this in the framework code.
>>
>> Dan
>>
> 
> What if we just stuck to color spaces and defined channel curves in the
> device tree?
> 

Vesa did a good summary of the current MC framework implementations that are being proposed
The thread also documents Pavel's requirements

https://lore.kernel.org/patchwork/patch/1037147/

And here is the series with a single brightness file and passing in multiple integers.
I did not document this code as I was pretty certain it was not the way to go.

https://patches.linaro.org/project/linux-leds/list/?series=18022

Dan

> The curve could then be defined via an array of integer pairs, which
> would be interpreted as points via which the curve passes and the curve
> would be approximated by linear segemts...
> 
>   led@0 {
>     colorspace = <COLOR_SPACE_ID_RGB>;
>     red-curve = <0 0>, <255 255>;
>     green-curve = <0 0>, <255 128>;
>     blue-curve = <0 0>, <255 128>;
>   };
> 


-- 
------------------
Dan Murphy

Re: [RFC PATCH 5/5] leds: multicolor: Introduce a multicolor class definition

[Dan Murphy]

Randy

Thank you.

On 4/1/19 4:10 PM, Randy Dunlap wrote:
> Hi,
> 
> On 4/1/19 10:34 AM, 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  | 411 +++++++++++++++++++++++++++
>>  include/linux/led-class-multicolor.h |  69 +++++
>>  4 files changed, 491 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 a72f97fca57b..477e108fd2a7 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"
> 
> 	              Multi

Ack

> 
>> +	depends on LEDS_CLASS
>> +	help
>> +	  This option enables the flash led sysfs class in /sys/class/leds.
> 
> 	                                LED (?)
> 

HA!  This was a copy paste from the Flash class kconfig.  I need to re-write the description anyway.

So your comments actually will turn into a bug fix for the Kconfig on that flag.

Dan
<snip>
-- 
------------------
Dan Murphy

Re: [RFC PATCH 2/5] dt: bindings: Add multicolor class dt bindings documention

[Jacek Anaszewski]

On 4/1/19 11:29 PM, Pavel Machek wrote:
> Hi!
> 
> 
>>   .../bindings/leds/leds-class-multicolor.txt   | 140 ++++++++++++++++++
>>   1 file changed, 140 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..4b1a26104c79
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>> @@ -0,0 +1,140 @@
>> +* 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 : This is the color ID of the LED.  Definitions can be found
>> +		  in include/linux/leds/common.txt
>> +
>> +Optional LED Child properties:
>> +	- available-brightness-models : This is the phandle to the brightness-model
>> +					node(s) that this LED cluster can support.
>> +
>> +Required Brightness model properties
>> +	- led-brightness-model : This flag alerts the device driver and class
>> +				 code that this node is a brightness model node
>> +				 and to process the properties differently.
>> +
>> +Required Brightness model child properties
>> +	- model_name : This is the name of the model presented to the user.  This
>> +		       should be a color that the LED cluster can produce for
>> +		       the device it is attached to.
>> +	- layout : This is the LED layout for the levels.  This layout will
>> +		   determine the color order of the levels.  The layout and
>> +		   level-x properties array should be the same size.
>> +	- level-x : These are the values for the LEDs to produce the color that
>> +		    is defined.  These values are placed in the array according
>> +		    to the layout property.
> 
>> +led-controller@30 {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	compatible = "ti,lp5024";
>> +	reg = <0x29>;
>> +
>> +	lp5024_model_yellow: brightness-models {
>> +		led-brightness-model;
>> +		model@0 {
>> +			model_name = "yellow";
>> +			layout = <LED_COLOR_ID_RED
>> +				  LED_COLOR_ID_GREEN
>> +				  LED_COLOR_ID_BLUE>;
>> +			level-1 = <255 227 40>;
>> +			level-2 = <255 240 136>;
>> +			level-3 = <255 247 196>;
>> +		};
>> +	};
> 
> I don't think this works. RGB LED can show millions of colors. Do you
> propose to have millions of entries in dts?

Pavel, you know very well what is going on here and what is the
background of this design. I suppose you've just entered your trolling
mode - you lately admitted that it sometimes happens :-) [0]

This aims at solving the problem with multi color LED support in
a backward compatible way, which is especially important in relation
to existing triggers.

This is our attempt of addressing that problem in a color space
agnostic way. It was inspired by out-of-tree approach presented
by Vesa few months ago.

[0] https://www.spinics.net/lists/linux-leds/msg11388.html

-- 
Best regards,
Jacek Anaszewski

Re: [RFC PATCH 2/5] dt: bindings: Add multicolor class dt bindings documention

[Jacek Anaszewski]

On 4/2/19 1:40 PM, Dan Murphy wrote:
> Pavel
> 
> On 4/1/19 4:29 PM, Pavel Machek wrote:
>> Hi!
>>
>>
>>>   .../bindings/leds/leds-class-multicolor.txt   | 140 ++++++++++++++++++
>>>   1 file changed, 140 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..4b1a26104c79
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-class-multicolor.txt
>>> @@ -0,0 +1,140 @@
>>> +* 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 : This is the color ID of the LED.  Definitions can be found
>>> +		  in include/linux/leds/common.txt
>>> +
>>> +Optional LED Child properties:
>>> +	- available-brightness-models : This is the phandle to the brightness-model
>>> +					node(s) that this LED cluster can support.
>>> +
>>> +Required Brightness model properties
>>> +	- led-brightness-model : This flag alerts the device driver and class
>>> +				 code that this node is a brightness model node
>>> +				 and to process the properties differently.
>>> +
>>> +Required Brightness model child properties
>>> +	- model_name : This is the name of the model presented to the user.  This
>>> +		       should be a color that the LED cluster can produce for
>>> +		       the device it is attached to.
>>> +	- layout : This is the LED layout for the levels.  This layout will
>>> +		   determine the color order of the levels.  The layout and
>>> +		   level-x properties array should be the same size.
>>> +	- level-x : These are the values for the LEDs to produce the color that
>>> +		    is defined.  These values are placed in the array according
>>> +		    to the layout property.
>>
>>> +led-controller@30 {
>>> +	#address-cells = <1>;
>>> +	#size-cells = <0>;
>>> +	compatible = "ti,lp5024";
>>> +	reg = <0x29>;
>>> +
>>> +	lp5024_model_yellow: brightness-models {
>>> +		led-brightness-model;
>>> +		model@0 {
>>> +			model_name = "yellow";
>>> +			layout = <LED_COLOR_ID_RED
>>> +				  LED_COLOR_ID_GREEN
>>> +				  LED_COLOR_ID_BLUE>;
>>> +			level-1 = <255 227 40>;
>>> +			level-2 = <255 240 136>;
>>> +			level-3 = <255 247 196>;
>>> +		};
>>> +	};
>>
>> I don't think this works. RGB LED can show millions of colors. Do you
>> propose to have millions of entries in dts?
>> 									Pavel
>>
> 
> I have had off line conversations with Jacek about this brightness model node.
> 
> Your concern was actually one of my concerns as well.  Not only millions of entries but also having a huge
> DT binary.
> 
> We wanted to RFC this to get feedback.  And this is why I have not added any support for this in the framework code.

We all know that nobody sane is going to add millions of color entries.
It is just a holistic solution to the problem, without the need to worry
about different colors being produced by different LED elements, either
due to their inherent properties, due to different parameters of LED
controllers and board designs, or due to aging. Of course the last
case will be possible to fix only by update of DTB blob.

Regarding the possible size of DTB blob, I made some calculations and
even compiled sample dts. Here is my reasoning from one of the replies
to Dan during our private discussion:

-------------------------------------------------------------------------

Brightness models will be reusable between modules. There could be
arbitrary number of brightness models, limited only by maximum dtb blob
size.

In uboot it can be configured with "setenv fdt_high". For the dtbs
appended to zImage for ARM the limit is 2MB AFAIK. Brightness models
will be what consumes the most of memory with this proposed DT design.

Let's calculate.

4 bytes for single brightness value (we allow for values > 255).

I think quite exaggerated exemplary quantities:

number of colors in the mcled_dev: 10
number of levels per model: 256
number of models: 100

bytes required = 4 * 10 * 256 * 100 = 1024000, i.e. less than 1MB

And average mainline dtbs usually don't exceed 100kB, so I think
we are safe. I've even just tried it out by adding such 10 models
of 255 levels * 10 values * 4-byte to versatile-pb.dts and I got dtb
size ~150kB.

But to be sure it would be good to get early ack from DT guys.

-------------------------------------------------------------------------

And now is this moment - we'd like to hear the feedback from DT guys.

-- 
Best regards,
Jacek Anaszewski