Hello
This is the multi color LED framework. This framework presents clustered
colored LEDs into an array and allows the user space to adjust the brightness
of the cluster using a single file write. The individual colored LEDs
intensities are controlled via a single file that is an array of LEDs
A design alternative to having files that have multiple values written to a
single file is here:
https://lore.kernel.org/patchwork/patch/1186194/
Dan
Dan Murphy (4):
dt: bindings: Add multicolor class dt bindings documention
dt-bindings: leds: Add multicolor ID to the color ID list
leds: Add multicolor ID to the color ID list
leds: multicolor: Introduce a multicolor class definition
.../ABI/testing/sysfs-class-led-multicolor | 51 ++++
.../bindings/leds/leds-class-multicolor.txt | 98 ++++++++
Documentation/leds/index.rst | 1 +
Documentation/leds/leds-class-multicolor.rst | 110 +++++++++
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/led-class-multicolor.c | 224 ++++++++++++++++++
drivers/leds/led-core.c | 1 +
include/dt-bindings/leds/common.h | 3 +-
include/linux/led-class-multicolor.h | 124 ++++++++++
10 files changed, 622 insertions(+), 1 deletion(-)
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.rst
create mode 100644 drivers/leds/led-class-multicolor.c
create mode 100644 include/linux/led-class-multicolor.h
--
2.25.1
Introduce a multicolor class that groups colored LEDs
within a LED node.
The multi color class groups monochrome LEDs and allows controlling two
aspects of the final combined color: hue and lightness. The former is
controlled via color_intensity files and the latter is controlled
via brightness file.
CC: Greg KH <[email protected]>
Signed-off-by: Dan Murphy <[email protected]>
---
A design alternative to having files that having multiple values written to a
single file is here:
https://lore.kernel.org/patchwork/patch/1186194/
.../ABI/testing/sysfs-class-led-multicolor | 51 ++++
Documentation/leds/index.rst | 1 +
Documentation/leds/leds-class-multicolor.rst | 110 +++++++++
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/led-class-multicolor.c | 224 ++++++++++++++++++
include/linux/led-class-multicolor.h | 124 ++++++++++
7 files changed, 521 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor
create mode 100644 Documentation/leds/leds-class-multicolor.rst
create mode 100644 drivers/leds/led-class-multicolor.c
create mode 100644 include/linux/led-class-multicolor.h
diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
new file mode 100644
index 000000000000..a93772212c21
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
@@ -0,0 +1,51 @@
+What: /sys/class/leds/<led>/brightness
+Date: March 2020
+KernelVersion: 5.8
+Contact: Dan Murphy <[email protected]>
+Description: read/write
+ Writing to this file will update all LEDs within the group to a
+ calculated percentage of what each color LED intensity is set
+ to. The percentage is calculated for each grouped LED via the
+ equation below:
+
+ led_brightness = brightness * <color>_intensity/<color>_max_intensity
+
+ For additional details please refer to
+ Documentation/leds/leds-class-multicolor.rst.
+
+ The value of the color is from 0 to
+ /sys/class/leds/<led>/max_brightness.
+
+What: /sys/class/leds/<led>/color_index
+Date: March 2020
+KernelVersion: 5.8
+Contact: Dan Murphy <[email protected]>
+Description: read
+ The color_index array, when read, will output the LED colors
+ by name as they are indexed in the color_intensity array.
+
+What: /sys/class/leds/<led>/num_color_leds
+Date: March 2020
+KernelVersion: 5.8
+Contact: Dan Murphy <[email protected]>
+Description: read
+ The num_color_leds indicates the number of LEDs defined in the
+ color_intensity, color_max_intensity and color_index arrays.
+
+What: /sys/class/leds/<led>/color_max_intensity
+Date: March 2020
+KernelVersion: 5.8
+Contact: Dan Murphy <[email protected]>
+Description: read
+ Maximum intensity level for the LED color within the array.
+ The max intensities for each color must be entered based on the
+ color_index array.
+
+What: /sys/class/leds/<led>/color_intensity
+Date: March 2020
+KernelVersion: 5.8
+Contact: Dan Murphy <[email protected]>
+Description: read/write
+ Intensity level for the LED color within the array.
+ The intensities for each color must be entered based on the
+ color_index array.
diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
index 060f4e485897..bc70c6aa7138 100644
--- a/Documentation/leds/index.rst
+++ b/Documentation/leds/index.rst
@@ -9,6 +9,7 @@ LEDs
leds-class
leds-class-flash
+ leds-class-multicolor
ledtrig-oneshot
ledtrig-transient
ledtrig-usbport
diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst
new file mode 100644
index 000000000000..902a281a44d4
--- /dev/null
+++ b/Documentation/leds/leds-class-multicolor.rst
@@ -0,0 +1,110 @@
+====================================
+Multi Color LED handling under Linux
+====================================
+
+Description
+===========
+The multi color class groups monochrome LEDs and allows controlling two
+aspects of the final combined color: hue and lightness. The former is
+controlled via the color_intensity array file and the latter is controlled
+via brightness file.
+
+For more details on hue and lightness notions please refer to
+https://en.wikipedia.org/wiki/CIECAM02.
+
+Note that intensity file only caches the written values and the actual
+change of hardware state occurs upon writing brightness file. This
+allows for changing many factors of the perceived color in a virtually
+unnoticeable way for the human observer.
+
+Multicolor Class Control
+========================
+The multicolor class presents files that groups the colors as indexes in an
+array. These files are children under the LED parent node created by the
+led_class framework. The led_class framework is documented in led-class.rst
+within this documentation directory.
+
+Each colored LED will be indexed under the color_* files. The order of the
+colors are arbitrary the color_index file can be read to determine the color
+to index value.
+
+The color_index file is an array that contains the string list of the colors as
+they are defined in each color_* array file.
+
+The color_intensity is an array that can be read or written to for the
+individual color intensities. All elements within this array must be written in
+order for the color LED intensities to be updated.
+
+The color_max_intensity is an array that can be read to indicate each color LED
+maximum intensity value.
+
+The num_color_leds file returns the total number of color LEDs that are
+presented in each color_* array.
+
+Directory Layout Example
+========================
+root:/sys/class/leds/multicolor:grouped_leds# ls -lR
+-rw-r--r-- 1 root root 4096 Oct 19 16:16 brightness
+-r--r--r-- 1 root root 4096 Oct 19 16:16 color_index
+-rw-r--r-- 1 root root 4096 Oct 19 16:16 color_intensity
+-r--r--r-- 1 root root 4096 Oct 19 16:16 color_max_intensity
+-r--r--r-- 1 root root 4096 Oct 19 16:16 num_color_leds
+
+Multicolor Class Brightness Control
+===================================
+The multiclor class framework will calculate each monochrome LEDs intensity.
+
+The brightness level for each LED is calculated based on the color LED
+intensity setting divided by the color LED max intensity setting multiplied by
+the requested brightness.
+
+led_brightness = brightness * color_intensity/color_max_intensity
+
+It is important to note that for each LED within the cluster the
+color_max_intensity value should be the same. This will help maintain a
+consistent hue across brightness levels.
+
+Example:
+A user first writes the color LEDs intensity file with the brightness levels
+that for each LED that is necessary to achieve a blueish violet output from a
+RGB LED group.
+
+cat /sys/class/leds/multicolor:grouped_leds/color_index
+green blue red
+
+echo 43 226 138 > /sys/class/leds/multicolor:grouped_leds/color_intensity
+
+red -
+ intensity = 138
+ max_intensity = 255
+green -
+ intensity = 43
+ max_intensity = 255
+blue -
+ intensity = 226
+ max_intensity = 255
+
+The user can control the brightness of that RGB group by writing the parent
+'brightness' control. Assuming a parent max_brightness of 255 the user may want
+to dim the LED color group to half. The user would write a value of 128 to the
+parent brightness file then the values written to each LED will be adjusted
+base on this value
+
+cat /sys/class/leds/multicolor:grouped_leds/max_brightness
+255
+echo 128 > /sys/class/leds/multicolor:grouped_leds/brightness
+
+adjusted_red_value = 128 * 138/255 = 69
+adjusted_green_value = 128 * 43/255 = 21
+adjusted_blue_value = 128 * 226/255 = 113
+
+Reading the parent brightness file will return the current brightness value of
+the color LED group.
+
+cat /sys/class/leds/multicolor:grouped_leds/max_brightness
+255
+
+echo 128 > /sys/class/leds/multicolor:grouped_leds/brightness
+
+cat /sys/class/leds/multicolor:grouped_leds/brightness
+128
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index d82f1dea3711..37dcdb06a29b 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -30,6 +30,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 d7e1107753fb..310b5518783a 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..0dbafede6e58
--- /dev/null
+++ b/drivers/leds/led-class-multicolor.c
@@ -0,0 +1,224 @@
+// 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 <linux/uaccess.h>
+
+#include "leds.h"
+
+int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
+ enum led_brightness brightness)
+{
+ int i;
+
+ for (i = 0; i < mcled_cdev->num_colors; i++)
+ mcled_cdev->color_brightness[i] = brightness *
+ mcled_cdev->color_led_intensity[i] /
+ mcled_cdev->color_led_max_intensity[i];
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(led_mc_calc_color_components);
+
+static ssize_t color_intensity_store(struct device *dev,
+ struct device_attribute *intensity_attr,
+ const char *buf, size_t size)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
+ int nrchars, offset = 0;
+ int intensity_value[LED_COLOR_ID_MAX];
+ int i;
+ ssize_t ret;
+
+ mutex_lock(&led_cdev->led_access);
+
+ for (i = 0; i < priv->num_colors; i++) {
+ ret = sscanf(buf + offset, "%i%n",
+ &intensity_value[i], &nrchars);
+ if (ret != 1) {
+ dev_err(led_cdev->dev,
+ "Incorrect number of LEDs expected %i values intensity was not applied\n",
+ priv->num_colors);
+ goto err_out;
+ }
+ offset += nrchars;
+ }
+
+ memcpy(&priv->color_led_intensity, intensity_value,
+ sizeof(intensity_value));
+err_out:
+ ret = size;
+ mutex_unlock(&led_cdev->led_access);
+ return ret;
+}
+
+static ssize_t color_intensity_show(struct device *dev,
+ struct device_attribute *intensity_attr,
+ char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
+ int len = 0;
+ int i;
+
+ for (i = 0; i < priv->num_colors; i++)
+ len += sprintf(buf + len, "%d ", priv->color_led_intensity[i]);
+
+ len += sprintf(buf + len, "%s", "\n");
+
+ return len;
+}
+static DEVICE_ATTR_RW(color_intensity);
+
+static ssize_t color_index_show(struct device *dev,
+ struct device_attribute *color_index_attr,
+ char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
+ int len = 0;
+ int i;
+
+ for (i = 0; i < priv->num_colors; i++)
+ len += sprintf(buf + len, "%s ", priv->color_index[i]);
+
+ len += sprintf(buf + len, "%s", "\n");
+
+ return len;
+}
+static DEVICE_ATTR_RO(color_index);
+
+static ssize_t color_max_intensity_show(struct device *dev,
+ struct device_attribute *max_intensity_attr,
+ char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
+ int len = 0;
+ int i;
+
+ for (i = 0; i < priv->num_colors; i++)
+ len += sprintf(buf + len, "%d ",
+ priv->color_led_max_intensity[i]);
+
+ len += sprintf(buf + len, "%s", "\n");
+
+ return len;
+}
+static DEVICE_ATTR_RO(color_max_intensity);
+
+static ssize_t num_color_leds_show(struct device *dev,
+ struct device_attribute *max_intensity_attr,
+ char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
+
+ return sprintf(buf, "%d\n", priv->num_colors);
+}
+static DEVICE_ATTR_RO(num_color_leds);
+
+static struct attribute *led_multicolor_attrs[] = {
+ &dev_attr_color_intensity.attr,
+ &dev_attr_color_index.attr,
+ &dev_attr_color_max_intensity.attr,
+ &dev_attr_num_color_leds.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(led_multicolor);
+
+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;
+ int i;
+
+ if (!mcled_cdev)
+ return -EINVAL;
+
+ if (!mcled_cdev->num_colors)
+ return -EINVAL;
+
+ led_cdev = &mcled_cdev->led_cdev;
+
+ mcled_cdev->led_cdev.groups = led_multicolor_groups;
+
+ for (i = 0; i <= mcled_cdev->num_colors; i++)
+ if (!mcled_cdev->color_led_max_intensity[i])
+ mcled_cdev->color_led_max_intensity[i] = LED_FULL;
+
+ return led_classdev_register_ext(parent, &mcled_cdev->led_cdev,
+ init_data);
+}
+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);
+}
+
+int devm_led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data)
+{
+ 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_ext(parent, mcled_cdev,
+ init_data);
+ if (ret) {
+ devres_free(dr);
+ return ret;
+ }
+
+ *dr = mcled_cdev;
+ devres_add(parent, dr);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_register_ext);
+
+static int devm_led_classdev_multicolor_match(struct device *dev,
+ void *res, void *data)
+{
+ struct led_classdev_mc **p = res;
+
+ if (WARN_ON(!p || !*p))
+ return 0;
+
+ return *p == data;
+}
+
+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 <[email protected]>");
+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..bfbde2e98340
--- /dev/null
+++ b/include/linux/led-class-multicolor.h
@@ -0,0 +1,124 @@
+/* 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 {
+ /* led class device */
+ struct led_classdev led_cdev;
+
+ struct device_attribute color_max_intensity_attr;
+ struct device_attribute color_intensity_attr;
+ struct device_attribute color_index_attr;
+
+ int color_brightness[LED_COLOR_ID_MAX];
+
+ int color_led_intensity[LED_COLOR_ID_MAX];
+ int color_led_max_intensity[LED_COLOR_ID_MAX];
+ const char *color_index[LED_COLOR_ID_MAX];
+
+ int num_colors;
+};
+
+static inline struct led_classdev_mc *lcdev_to_mccdev(
+ struct led_classdev *led_cdev)
+{
+ return container_of(led_cdev, struct led_classdev_mc, led_cdev);
+}
+
+#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
+/**
+ * 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
+ */
+int led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data);
+
+static inline int led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return 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
+ */
+void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev);
+
+/* Calculate brightness for the monochrome LED cluster */
+int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
+ enum led_brightness brightness);
+
+int devm_led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data);
+
+static inline int devm_led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev,
+ NULL);
+}
+
+void devm_led_classdev_multicolor_unregister(struct device *parent,
+ struct led_classdev_mc *mcled_cdev);
+#else
+
+static inline int led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data)
+{
+ return -EINVAL;
+}
+
+static inline int led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL);
+}
+
+static inline void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) {};
+static inline int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
+ enum led_brightness brightness,
+ struct led_mc_color_conversion color_components[])
+{
+ return -EINVAL;
+}
+
+static inline int devm_led_classdev_multicolor_register_ext(struct device *parent,
+ struct led_classdev_mc *mcled_cdev,
+ struct led_init_data *init_data)
+{
+ return -EINVAL;
+}
+
+static inline int devm_led_classdev_multicolor_register(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{
+ return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev,
+ NULL);
+}
+
+static inline void devm_led_classdev_multicolor_unregister(struct device *parent,
+ struct led_classdev_mc *mcled_cdev)
+{};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR) */
+#endif /* __LINUX_MULTICOLOR_LEDS_H_INCLUDED */
--
2.25.1
On 3/24/20 11:14 AM, Dan Murphy wrote:
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index d82f1dea3711..37dcdb06a29b 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -30,6 +30,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
or even MultiColor
or Multicolor
> + 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
--
~Randy
Randy
On 3/24/20 1:41 PM, Randy Dunlap wrote:
> On 3/24/20 11:14 AM, Dan Murphy wrote:
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index d82f1dea3711..37dcdb06a29b 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -30,6 +30,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
> or even MultiColor
> or Multicolor
Thanks for the spell check. I cannot believe this made it through so
many reviews. hehe
Dan
Hi Dan,
Thanks for the update. The picture would be more complete if
the patch set contained a client though.
Anyway, please find my review remarks below.
On 3/24/20 7:14 PM, Dan Murphy wrote:
> Introduce a multicolor class that groups colored LEDs
> within a LED node.
>
> The multi color class groups monochrome LEDs and allows controlling two
> aspects of the final combined color: hue and lightness. The former is
> controlled via color_intensity files and the latter is controlled
s/files/file/
> via brightness file.
>
> CC: Greg KH <[email protected]>
> Signed-off-by: Dan Murphy <[email protected]>
> ---
> A design alternative to having files that having multiple values written to a
> single file is here:
>
> https://lore.kernel.org/patchwork/patch/1186194/
>
> .../ABI/testing/sysfs-class-led-multicolor | 51 ++++
> Documentation/leds/index.rst | 1 +
> Documentation/leds/leds-class-multicolor.rst | 110 +++++++++
> drivers/leds/Kconfig | 10 +
> drivers/leds/Makefile | 1 +
> drivers/leds/led-class-multicolor.c | 224 ++++++++++++++++++
> include/linux/led-class-multicolor.h | 124 ++++++++++
> 7 files changed, 521 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor
> create mode 100644 Documentation/leds/leds-class-multicolor.rst
> create mode 100644 drivers/leds/led-class-multicolor.c
> create mode 100644 include/linux/led-class-multicolor.h
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
> new file mode 100644
> index 000000000000..a93772212c21
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
> @@ -0,0 +1,51 @@
> +What: /sys/class/leds/<led>/brightness
> +Date: March 2020
> +KernelVersion: 5.8
> +Contact: Dan Murphy <[email protected]>
> +Description: read/write
> + Writing to this file will update all LEDs within the group to a
> + calculated percentage of what each color LED intensity is set
> + to. The percentage is calculated for each grouped LED via the
> + equation below:
> +
> + led_brightness = brightness * <color>_intensity/<color>_max_intensity
We now have a single color_intensity file, so this needs to be rephrased.
> +
> + For additional details please refer to
> + Documentation/leds/leds-class-multicolor.rst.
> +
> + The value of the color is from 0 to
> + /sys/class/leds/<led>/max_brightness.
> +
> +What: /sys/class/leds/<led>/color_index
> +Date: March 2020
> +KernelVersion: 5.8
> +Contact: Dan Murphy <[email protected]>
> +Description: read
> + The color_index array, when read, will output the LED colors
> + by name as they are indexed in the color_intensity array.
> +
> +What: /sys/class/leds/<led>/num_color_leds
I would go for just num_colors.
> +Date: March 2020
> +KernelVersion: 5.8
> +Contact: Dan Murphy <[email protected]>
> +Description: read
> + The num_color_leds indicates the number of LEDs defined in the
> + color_intensity, color_max_intensity and color_index arrays.
> +
> +What: /sys/class/leds/<led>/color_max_intensity
> +Date: March 2020
> +KernelVersion: 5.8
> +Contact: Dan Murphy <[email protected]>
> +Description: read
> + Maximum intensity level for the LED color within the array.
> + The max intensities for each color must be entered based on the
> + color_index array.
I wonder if we should mention here that each LED within a cluster should
have the same maximum intensity for linear color lightness calculation
via brightness file.
> +
> +What: /sys/class/leds/<led>/color_intensity
> +Date: March 2020
> +KernelVersion: 5.8
> +Contact: Dan Murphy <[email protected]>
> +Description: read/write
> + Intensity level for the LED color within the array.
> + The intensities for each color must be entered based on the
> + color_index array.
> diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
> index 060f4e485897..bc70c6aa7138 100644
> --- a/Documentation/leds/index.rst
> +++ b/Documentation/leds/index.rst
> @@ -9,6 +9,7 @@ LEDs
>
> leds-class
> leds-class-flash
> + leds-class-multicolor
> ledtrig-oneshot
> ledtrig-transient
> ledtrig-usbport
> diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst
> new file mode 100644
> index 000000000000..902a281a44d4
> --- /dev/null
> +++ b/Documentation/leds/leds-class-multicolor.rst
> @@ -0,0 +1,110 @@
> +====================================
> +Multi Color LED handling under Linux
> +====================================
> +
> +Description
> +===========
> +The multi color class groups monochrome LEDs and allows controlling two
> +aspects of the final combined color: hue and lightness. The former is
> +controlled via the color_intensity array file and the latter is controlled
> +via brightness file.
> +
> +For more details on hue and lightness notions please refer to
> +https://en.wikipedia.org/wiki/CIECAM02.
> +
> +Note that intensity file only caches the written values and the actual
s/intensity/color_intensity/
> +change of hardware state occurs upon writing brightness file. This
> +allows for changing many factors of the perceived color in a virtually
> +unnoticeable way for the human observer.
It will have to be changed after we agree upon exact semantics and
interface of color_intensity, as I raised an issue below.
> +
> +Multicolor Class Control
> +========================
> +The multicolor class presents files that groups the colors as indexes in an
> +array. These files are children under the LED parent node created by the
> +led_class framework. The led_class framework is documented in led-class.rst
> +within this documentation directory.
> +
> +Each colored LED will be indexed under the color_* files. The order of the
> +colors are arbitrary the color_index file can be read to determine the color
> +to index value.
This paragraph seems to be a leftover from some older version of the
patch set.
> +
> +The color_index file is an array that contains the string list of the colors as
> +they are defined in each color_* array file.
> +
> +The color_intensity is an array that can be read or written to for the
> +individual color intensities. All elements within this array must be written in
> +order for the color LED intensities to be updated.
> +
> +The color_max_intensity is an array that can be read to indicate each color LED
> +maximum intensity value.
> +
> +The num_color_leds file returns the total number of color LEDs that are
> +presented in each color_* array.
> +
> +Directory Layout Example
> +========================
> +root:/sys/class/leds/multicolor:grouped_leds# ls -lR
> +-rw-r--r-- 1 root root 4096 Oct 19 16:16 brightness
> +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_index
> +-rw-r--r-- 1 root root 4096 Oct 19 16:16 color_intensity
> +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_max_intensity
> +-r--r--r-- 1 root root 4096 Oct 19 16:16 num_color_leds
> +
> +Multicolor Class Brightness Control
> +===================================
> +The multiclor class framework will calculate each monochrome LEDs intensity.
> +
> +The brightness level for each LED is calculated based on the color LED
> +intensity setting divided by the color LED max intensity setting multiplied by
> +the requested brightness.
> +
> +led_brightness = brightness * color_intensity/color_max_intensity
Maybe some pseudo code would allow for better understanding here:
for color in color_intensity
led[color].brightness = brightness *
led[color].intensity / led[color].max_intensity
> +
> +It is important to note that for each LED within the cluster the
> +color_max_intensity value should be the same. This will help maintain a
> +consistent hue across brightness levels.
> +
> +Example:
> +A user first writes the color LEDs intensity file with the brightness levels
s/color LEDs intensity/color_intensity/
> +that for each LED that is necessary to achieve a blueish violet output from a
> +RGB LED group.
> +
> +cat /sys/class/leds/multicolor:grouped_leds/color_index
We don't have "grouped_leds" LED function so let's better use
something available, like e.g "status".
> +green blue red
> +
> +echo 43 226 138 > /sys/class/leds/multicolor:grouped_leds/color_intensity
> +
> +red -
> + intensity = 138
> + max_intensity = 255
> +green -
> + intensity = 43
> + max_intensity = 255
> +blue -
> + intensity = 226
> + max_intensity = 255
> +
> +The user can control the brightness of that RGB group by writing the parent
> +'brightness' control. Assuming a parent max_brightness of 255 the user may want
> +to dim the LED color group to half. The user would write a value of 128 to the
> +parent brightness file then the values written to each LED will be adjusted
> +base on this value
> +
> +cat /sys/class/leds/multicolor:grouped_leds/max_brightness
> +255
> +echo 128 > /sys/class/leds/multicolor:grouped_leds/brightness
> +
> +adjusted_red_value = 128 * 138/255 = 69
> +adjusted_green_value = 128 * 43/255 = 21
> +adjusted_blue_value = 128 * 226/255 = 113
> +
> +Reading the parent brightness file will return the current brightness value of
> +the color LED group.
> +
> +cat /sys/class/leds/multicolor:grouped_leds/max_brightness
> +255
> +
> +echo 128 > /sys/class/leds/multicolor:grouped_leds/brightness
It was already written once above, no need to repeat.
> +
> +cat /sys/class/leds/multicolor:grouped_leds/brightness
> +128
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index d82f1dea3711..37dcdb06a29b 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -30,6 +30,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 d7e1107753fb..310b5518783a 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..0dbafede6e58
> --- /dev/null
> +++ b/drivers/leds/led-class-multicolor.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// LED Multi Color class interface
> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
I think that you deserve mentioning yourself as an author too.
> +
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#include "leds.h"
> +
> +int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
> + enum led_brightness brightness)
> +{
> + int i;
> +
> + for (i = 0; i < mcled_cdev->num_colors; i++)
> + mcled_cdev->color_brightness[i] = brightness *
> + mcled_cdev->color_led_intensity[i] /
> + mcled_cdev->color_led_max_intensity[i];
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(led_mc_calc_color_components);
> +
> +static ssize_t color_intensity_store(struct device *dev,
> + struct device_attribute *intensity_attr,
> + const char *buf, size_t size)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
> + int nrchars, offset = 0;
> + int intensity_value[LED_COLOR_ID_MAX];
> + int i;
> + ssize_t ret;
> +
> + mutex_lock(&led_cdev->led_access);
> +
> + for (i = 0; i < priv->num_colors; i++) {
> + ret = sscanf(buf + offset, "%i%n",
> + &intensity_value[i], &nrchars);
> + if (ret != 1) {
> + dev_err(led_cdev->dev,
> + "Incorrect number of LEDs expected %i values intensity was not applied\n",
> + priv->num_colors);
> + goto err_out;
> + }
> + offset += nrchars;
> + }
I've just realized that moving to single color_intensity file
doesn't allow setting all colors together with new brightness
atomically. In effect, we will need to pass brightness to this file too,
if we want to avoid "interesting" latching via brightenss file.
Then we would need to call led_set_brightness() from here as well.
> +
> + memcpy(&priv->color_led_intensity, intensity_value,
> + sizeof(intensity_value));
> +err_out:
> + ret = size;
> + mutex_unlock(&led_cdev->led_access);
> + return ret;
> +}
> +
> +static ssize_t color_intensity_show(struct device *dev,
> + struct device_attribute *intensity_attr,
> + char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
> + int len = 0;
> + int i;
> +
> + for (i = 0; i < priv->num_colors; i++)
> + len += sprintf(buf + len, "%d ", priv->color_led_intensity[i]);
> +
> + len += sprintf(buf + len, "%s", "\n");
> +
> + return len;
> +}
> +static DEVICE_ATTR_RW(color_intensity);
> +
> +static ssize_t color_index_show(struct device *dev,
> + struct device_attribute *color_index_attr,
> + char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
> + int len = 0;
> + int i;
> +
> + for (i = 0; i < priv->num_colors; i++)
> + len += sprintf(buf + len, "%s ", priv->color_index[i]);
> +
> + len += sprintf(buf + len, "%s", "\n");
> +
> + return len;
> +}
> +static DEVICE_ATTR_RO(color_index);
> +
> +static ssize_t color_max_intensity_show(struct device *dev,
> + struct device_attribute *max_intensity_attr,
> + char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
> + int len = 0;
> + int i;
> +
> + for (i = 0; i < priv->num_colors; i++)
> + len += sprintf(buf + len, "%d ",
> + priv->color_led_max_intensity[i]);
> +
> + len += sprintf(buf + len, "%s", "\n");
> +
> + return len;
> +}
> +static DEVICE_ATTR_RO(color_max_intensity);
> +
> +static ssize_t num_color_leds_show(struct device *dev,
> + struct device_attribute *max_intensity_attr,
> + char *buf)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
> +
> + return sprintf(buf, "%d\n", priv->num_colors);
> +}
> +static DEVICE_ATTR_RO(num_color_leds);
> +
> +static struct attribute *led_multicolor_attrs[] = {
> + &dev_attr_color_intensity.attr,
> + &dev_attr_color_index.attr,
> + &dev_attr_color_max_intensity.attr,
> + &dev_attr_num_color_leds.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(led_multicolor);
> +
> +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;
> + int i;
> +
> + if (!mcled_cdev)
> + return -EINVAL;
> +
> + if (!mcled_cdev->num_colors)
> + return -EINVAL;
> +
> + led_cdev = &mcled_cdev->led_cdev;
> +
> + mcled_cdev->led_cdev.groups = led_multicolor_groups;
> +
> + for (i = 0; i <= mcled_cdev->num_colors; i++)
> + if (!mcled_cdev->color_led_max_intensity[i])
> + mcled_cdev->color_led_max_intensity[i] = LED_FULL;
> +
> + return led_classdev_register_ext(parent, &mcled_cdev->led_cdev,
> + init_data);
> +}
> +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);
> +}
> +
> +int devm_led_classdev_multicolor_register_ext(struct device *parent,
> + struct led_classdev_mc *mcled_cdev,
> + struct led_init_data *init_data)
> +{
> + 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_ext(parent, mcled_cdev,
> + init_data);
> + if (ret) {
> + devres_free(dr);
> + return ret;
> + }
> +
> + *dr = mcled_cdev;
> + devres_add(parent, dr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_register_ext);
> +
> +static int devm_led_classdev_multicolor_match(struct device *dev,
> + void *res, void *data)
> +{
> + struct led_classdev_mc **p = res;
> +
> + if (WARN_ON(!p || !*p))
> + return 0;
> +
> + return *p == data;
> +}
> +
> +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 <[email protected]>");
> +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..bfbde2e98340
> --- /dev/null
> +++ b/include/linux/led-class-multicolor.h
> @@ -0,0 +1,124 @@
> +/* 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 {
> + /* led class device */
> + struct led_classdev led_cdev;
> +
> + struct device_attribute color_max_intensity_attr;
> + struct device_attribute color_intensity_attr;
> + struct device_attribute color_index_attr;
These are no longer needed as you define attrs statically.
> +
> + int color_brightness[LED_COLOR_ID_MAX];
> +
> + int color_led_intensity[LED_COLOR_ID_MAX];
> + int color_led_max_intensity[LED_COLOR_ID_MAX];
> + const char *color_index[LED_COLOR_ID_MAX];
I think that we should get back to the available_colors bitmask
and allow the framework to allocate arrays by itself.
And yes, all the above should be pointers.
Driver would only need to set led_mcdev->available_colors bits.
> +
> + int num_colors;
> +};
> +
> +static inline struct led_classdev_mc *lcdev_to_mccdev(
> + struct led_classdev *led_cdev)
> +{
> + return container_of(led_cdev, struct led_classdev_mc, led_cdev);
> +}
> +
> +#if IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR)
> +/**
> + * 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
> + */
> +int led_classdev_multicolor_register_ext(struct device *parent,
> + struct led_classdev_mc *mcled_cdev,
> + struct led_init_data *init_data);
> +
> +static inline int led_classdev_multicolor_register(struct device *parent,
> + struct led_classdev_mc *mcled_cdev)
> +{
> + return 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
> + */
> +void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev);
> +
> +/* Calculate brightness for the monochrome LED cluster */
> +int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
> + enum led_brightness brightness);
> +
> +int devm_led_classdev_multicolor_register_ext(struct device *parent,
> + struct led_classdev_mc *mcled_cdev,
> + struct led_init_data *init_data);
> +
> +static inline int devm_led_classdev_multicolor_register(struct device *parent,
> + struct led_classdev_mc *mcled_cdev)
> +{
> + return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev,
> + NULL);
> +}
> +
> +void devm_led_classdev_multicolor_unregister(struct device *parent,
> + struct led_classdev_mc *mcled_cdev);
> +#else
> +
> +static inline int led_classdev_multicolor_register_ext(struct device *parent,
> + struct led_classdev_mc *mcled_cdev,
> + struct led_init_data *init_data)
> +{
> + return -EINVAL;
> +}
> +
> +static inline int led_classdev_multicolor_register(struct device *parent,
> + struct led_classdev_mc *mcled_cdev)
> +{
> + return led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL);
> +}
> +
> +static inline void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) {};
> +static inline int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
> + enum led_brightness brightness,
> + struct led_mc_color_conversion color_components[])
> +{
> + return -EINVAL;
> +}
> +
> +static inline int devm_led_classdev_multicolor_register_ext(struct device *parent,
> + struct led_classdev_mc *mcled_cdev,
> + struct led_init_data *init_data)
> +{
> + return -EINVAL;
> +}
> +
> +static inline int devm_led_classdev_multicolor_register(struct device *parent,
> + struct led_classdev_mc *mcled_cdev)
> +{
> + return devm_led_classdev_multicolor_register_ext(parent, mcled_cdev,
> + NULL);
> +}
> +
> +static inline void devm_led_classdev_multicolor_unregister(struct device *parent,
> + struct led_classdev_mc *mcled_cdev)
> +{};
> +
> +#endif /* IS_ENABLED(CONFIG_LEDS_CLASS_MULTI_COLOR) */
> +#endif /* __LINUX_MULTICOLOR_LEDS_H_INCLUDED */
>
--
Best regards,
Jacek Anaszewski
Jacek
Thanks for the review
On 3/28/20 9:03 AM, Jacek Anaszewski wrote:
> Hi Dan,
>
> Thanks for the update. The picture would be more complete if
> the patch set contained a client though.
I was going to send the ones I did but they are pretty dirty from a code
stand point.
Besides I would expect the framework to change which then changes the
driver code.
> Anyway, please find my review remarks below.
>
> On 3/24/20 7:14 PM, Dan Murphy wrote:
>> Introduce a multicolor class that groups colored LEDs
>> within a LED node.
>>
>> The multi color class groups monochrome LEDs and allows controlling two
>> aspects of the final combined color: hue and lightness. The former is
>> controlled via color_intensity files and the latter is controlled
> s/files/file/
Ack
>> via brightness file.
>>
>> CC: Greg KH <[email protected]>
>> Signed-off-by: Dan Murphy <[email protected]>
>> ---
>> A design alternative to having files that having multiple values written to a
>> single file is here:
>>
>> https://lore.kernel.org/patchwork/patch/1186194/
>>
>> .../ABI/testing/sysfs-class-led-multicolor | 51 ++++
>> Documentation/leds/index.rst | 1 +
>> Documentation/leds/leds-class-multicolor.rst | 110 +++++++++
>> drivers/leds/Kconfig | 10 +
>> drivers/leds/Makefile | 1 +
>> drivers/leds/led-class-multicolor.c | 224 ++++++++++++++++++
>> include/linux/led-class-multicolor.h | 124 ++++++++++
>> 7 files changed, 521 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor
>> create mode 100644 Documentation/leds/leds-class-multicolor.rst
>> create mode 100644 drivers/leds/led-class-multicolor.c
>> create mode 100644 include/linux/led-class-multicolor.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor
>> new file mode 100644
>> index 000000000000..a93772212c21
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor
>> @@ -0,0 +1,51 @@
>> +What: /sys/class/leds/<led>/brightness
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <[email protected]>
>> +Description: read/write
>> + Writing to this file will update all LEDs within the group to a
>> + calculated percentage of what each color LED intensity is set
>> + to. The percentage is calculated for each grouped LED via the
>> + equation below:
>> +
>> + led_brightness = brightness * <color>_intensity/<color>_max_intensity
> We now have a single color_intensity file, so this needs to be rephrased.
Ack
>> +
>> + For additional details please refer to
>> + Documentation/leds/leds-class-multicolor.rst.
>> +
>> + The value of the color is from 0 to
>> + /sys/class/leds/<led>/max_brightness.
>> +
>> +What: /sys/class/leds/<led>/color_index
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <[email protected]>
>> +Description: read
>> + The color_index array, when read, will output the LED colors
>> + by name as they are indexed in the color_intensity array.
>> +
>> +What: /sys/class/leds/<led>/num_color_leds
> I would go for just num_colors.
Ack
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <[email protected]>
>> +Description: read
>> + The num_color_leds indicates the number of LEDs defined in the
>> + color_intensity, color_max_intensity and color_index arrays.
>> +
>> +What: /sys/class/leds/<led>/color_max_intensity
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <[email protected]>
>> +Description: read
>> + Maximum intensity level for the LED color within the array.
>> + The max intensities for each color must be entered based on the
>> + color_index array.
> I wonder if we should mention here that each LED within a cluster should
> have the same maximum intensity for linear color lightness calculation
> via brightness file.
Does it really have to?
>> +
>> +What: /sys/class/leds/<led>/color_intensity
>> +Date: March 2020
>> +KernelVersion: 5.8
>> +Contact: Dan Murphy <[email protected]>
>> +Description: read/write
>> + Intensity level for the LED color within the array.
>> + The intensities for each color must be entered based on the
>> + color_index array.
>> diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst
>> index 060f4e485897..bc70c6aa7138 100644
>> --- a/Documentation/leds/index.rst
>> +++ b/Documentation/leds/index.rst
>> @@ -9,6 +9,7 @@ LEDs
>>
>> leds-class
>> leds-class-flash
>> + leds-class-multicolor
>> ledtrig-oneshot
>> ledtrig-transient
>> ledtrig-usbport
>> diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst
>> new file mode 100644
>> index 000000000000..902a281a44d4
>> --- /dev/null
>> +++ b/Documentation/leds/leds-class-multicolor.rst
>> @@ -0,0 +1,110 @@
>> +====================================
>> +Multi Color LED handling under Linux
>> +====================================
>> +
>> +Description
>> +===========
>> +The multi color class groups monochrome LEDs and allows controlling two
>> +aspects of the final combined color: hue and lightness. The former is
>> +controlled via the color_intensity array file and the latter is controlled
>> +via brightness file.
>> +
>> +For more details on hue and lightness notions please refer to
>> +https://en.wikipedia.org/wiki/CIECAM02.
>> +
>> +Note that intensity file only caches the written values and the actual
> s/intensity/color_intensity/
Ack
>
>> +change of hardware state occurs upon writing brightness file. This
>> +allows for changing many factors of the perceived color in a virtually
>> +unnoticeable way for the human observer.
> It will have to be changed after we agree upon exact semantics and
> interface of color_intensity, as I raised an issue below.
OK
>> +
>> +Multicolor Class Control
>> +========================
>> +The multicolor class presents files that groups the colors as indexes in an
>> +array. These files are children under the LED parent node created by the
>> +led_class framework. The led_class framework is documented in led-class.rst
>> +within this documentation directory.
>> +
>> +Each colored LED will be indexed under the color_* files. The order of the
>> +colors are arbitrary the color_index file can be read to determine the color
>> +to index value.
> This paragraph seems to be a leftover from some older version of the
> patch set.
>
Ack. I thought I vetted the file for completeness.
>> +
>> +The color_index file is an array that contains the string list of the colors as
>> +they are defined in each color_* array file.
>> +
>> +The color_intensity is an array that can be read or written to for the
>> +individual color intensities. All elements within this array must be written in
>> +order for the color LED intensities to be updated.
>> +
>> +The color_max_intensity is an array that can be read to indicate each color LED
>> +maximum intensity value.
>> +
>> +The num_color_leds file returns the total number of color LEDs that are
>> +presented in each color_* array.
>> +
>> +Directory Layout Example
>> +========================
>> +root:/sys/class/leds/multicolor:grouped_leds# ls -lR
>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16 brightness
>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_index
>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16 color_intensity
>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_max_intensity
>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 num_color_leds
>> +
>> +Multicolor Class Brightness Control
>> +===================================
>> +The multiclor class framework will calculate each monochrome LEDs intensity.
>> +
>> +The brightness level for each LED is calculated based on the color LED
>> +intensity setting divided by the color LED max intensity setting multiplied by
>> +the requested brightness.
>> +
>> +led_brightness = brightness * color_intensity/color_max_intensity
> Maybe some pseudo code would allow for better understanding here:
>
> for color in color_intensity
> led[color].brightness = brightness *
> led[color].intensity / led[color].max_intensity
I think this would be fine at least there is a documented equation. I
don't think we need to document the code.
>> +
>> +It is important to note that for each LED within the cluster the
>> +color_max_intensity value should be the same. This will help maintain a
>> +consistent hue across brightness levels.
>> +
>> +Example:
>> +A user first writes the color LEDs intensity file with the brightness levels
> s/color LEDs intensity/color_intensity/
Ack
>
>> +that for each LED that is necessary to achieve a blueish violet output from a
>> +RGB LED group.
>> +
>> +cat /sys/class/leds/multicolor:grouped_leds/color_index
> We don't have "grouped_leds" LED function so let's better use
> something available, like e.g "status".
Ack.
>
>> +green blue red
>> +
>> +echo 43 226 138 > /sys/class/leds/multicolor:grouped_leds/color_intensity
>> +
>> +red -
>> + intensity = 138
>> + max_intensity = 255
>> +green -
>> + intensity = 43
>> + max_intensity = 255
>> +blue -
>> + intensity = 226
>> + max_intensity = 255
>> +
>> +The user can control the brightness of that RGB group by writing the parent
>> +'brightness' control. Assuming a parent max_brightness of 255 the user may want
>> +to dim the LED color group to half. The user would write a value of 128 to the
>> +parent brightness file then the values written to each LED will be adjusted
>> +base on this value
>> +
>> +cat /sys/class/leds/multicolor:grouped_leds/max_brightness
>> +255
>> +echo 128 > /sys/class/leds/multicolor:grouped_leds/brightness
>> +
>> +adjusted_red_value = 128 * 138/255 = 69
>> +adjusted_green_value = 128 * 43/255 = 21
>> +adjusted_blue_value = 128 * 226/255 = 113
>> +
>> +Reading the parent brightness file will return the current brightness value of
>> +the color LED group.
>> +
>> +cat /sys/class/leds/multicolor:grouped_leds/max_brightness
>> +255
>> +
>> +echo 128 > /sys/class/leds/multicolor:grouped_leds/brightness
> It was already written once above, no need to repeat.
Copy paste issue
>> +
>> +cat /sys/class/leds/multicolor:grouped_leds/brightness
>> +128
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index d82f1dea3711..37dcdb06a29b 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -30,6 +30,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 d7e1107753fb..310b5518783a 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..0dbafede6e58
>> --- /dev/null
>> +++ b/drivers/leds/led-class-multicolor.c
>> @@ -0,0 +1,224 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// LED Multi Color class interface
>> +// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> I think that you deserve mentioning yourself as an author too.
Meh.
>> +
>> +#include <linux/device.h>
>> +#include <linux/init.h>
>> +#include <linux/led-class-multicolor.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include "leds.h"
>> +
>> +int led_mc_calc_color_components(struct led_classdev_mc *mcled_cdev,
>> + enum led_brightness brightness)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < mcled_cdev->num_colors; i++)
>> + mcled_cdev->color_brightness[i] = brightness *
>> + mcled_cdev->color_led_intensity[i] /
>> + mcled_cdev->color_led_max_intensity[i];
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(led_mc_calc_color_components);
>> +
>> +static ssize_t color_intensity_store(struct device *dev,
>> + struct device_attribute *intensity_attr,
>> + const char *buf, size_t size)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
>> + int nrchars, offset = 0;
>> + int intensity_value[LED_COLOR_ID_MAX];
>> + int i;
>> + ssize_t ret;
>> +
>> + mutex_lock(&led_cdev->led_access);
>> +
>> + for (i = 0; i < priv->num_colors; i++) {
>> + ret = sscanf(buf + offset, "%i%n",
>> + &intensity_value[i], &nrchars);
>> + if (ret != 1) {
>> + dev_err(led_cdev->dev,
>> + "Incorrect number of LEDs expected %i values intensity was not applied\n",
>> + priv->num_colors);
>> + goto err_out;
>> + }
>> + offset += nrchars;
>> + }
> I've just realized that moving to single color_intensity file
> doesn't allow setting all colors together with new brightness
> atomically. In effect, we will need to pass brightness to this file too,
> if we want to avoid "interesting" latching via brightenss file.
>
> Then we would need to call led_set_brightness() from here as well.
Why? This just caches the intensity of each colored LED. Then the
actual brightness is calculated only when the brightness file is updated.
This would be an automatic update of the LED and that is not the intent
of the intensity file per the documentation.
The user should be able to set the colors x number of times before the
LED group is actually updated with the brightness.
>> +
>> + memcpy(&priv->color_led_intensity, intensity_value,
>> + sizeof(intensity_value));
>> +err_out:
>> + ret = size;
>> + mutex_unlock(&led_cdev->led_access);
>> + return ret;
>> +}
>> +
>> +static ssize_t color_intensity_show(struct device *dev,
>> + struct device_attribute *intensity_attr,
>> + char *buf)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
>> + int len = 0;
>> + int i;
>> +
>> + for (i = 0; i < priv->num_colors; i++)
>> + len += sprintf(buf + len, "%d ", priv->color_led_intensity[i]);
>> +
>> + len += sprintf(buf + len, "%s", "\n");
>> +
>> + return len;
>> +}
>> +static DEVICE_ATTR_RW(color_intensity);
>> +
>> +static ssize_t color_index_show(struct device *dev,
>> + struct device_attribute *color_index_attr,
>> + char *buf)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
>> + int len = 0;
>> + int i;
>> +
>> + for (i = 0; i < priv->num_colors; i++)
>> + len += sprintf(buf + len, "%s ", priv->color_index[i]);
>> +
>> + len += sprintf(buf + len, "%s", "\n");
>> +
>> + return len;
>> +}
>> +static DEVICE_ATTR_RO(color_index);
>> +
>> +static ssize_t color_max_intensity_show(struct device *dev,
>> + struct device_attribute *max_intensity_attr,
>> + char *buf)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
>> + int len = 0;
>> + int i;
>> +
>> + for (i = 0; i < priv->num_colors; i++)
>> + len += sprintf(buf + len, "%d ",
>> + priv->color_led_max_intensity[i]);
>> +
>> + len += sprintf(buf + len, "%s", "\n");
>> +
>> + return len;
>> +}
>> +static DEVICE_ATTR_RO(color_max_intensity);
>> +
>> +static ssize_t num_color_leds_show(struct device *dev,
>> + struct device_attribute *max_intensity_attr,
>> + char *buf)
>> +{
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
>> +
>> + return sprintf(buf, "%d\n", priv->num_colors);
>> +}
>> +static DEVICE_ATTR_RO(num_color_leds);
>> +
>> +static struct attribute *led_multicolor_attrs[] = {
>> + &dev_attr_color_intensity.attr,
>> + &dev_attr_color_index.attr,
>> + &dev_attr_color_max_intensity.attr,
>> + &dev_attr_num_color_leds.attr,
>> + NULL,
>> +};
>> +ATTRIBUTE_GROUPS(led_multicolor);
>> +
>> +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;
>> + int i;
>> +
>> + if (!mcled_cdev)
>> + return -EINVAL;
>> +
>> + if (!mcled_cdev->num_colors)
>> + return -EINVAL;
>> +
>> + led_cdev = &mcled_cdev->led_cdev;
>> +
>> + mcled_cdev->led_cdev.groups = led_multicolor_groups;
>> +
>> + for (i = 0; i <= mcled_cdev->num_colors; i++)
>> + if (!mcled_cdev->color_led_max_intensity[i])
>> + mcled_cdev->color_led_max_intensity[i] = LED_FULL;
>> +
>> + return led_classdev_register_ext(parent, &mcled_cdev->led_cdev,
>> + init_data);
>> +}
>> +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);
>> +}
>> +
>> +int devm_led_classdev_multicolor_register_ext(struct device *parent,
>> + struct led_classdev_mc *mcled_cdev,
>> + struct led_init_data *init_data)
>> +{
>> + 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_ext(parent, mcled_cdev,
>> + init_data);
>> + if (ret) {
>> + devres_free(dr);
>> + return ret;
>> + }
>> +
>> + *dr = mcled_cdev;
>> + devres_add(parent, dr);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_led_classdev_multicolor_register_ext);
>> +
>> +static int devm_led_classdev_multicolor_match(struct device *dev,
>> + void *res, void *data)
>> +{
>> + struct led_classdev_mc **p = res;
>> +
>> + if (WARN_ON(!p || !*p))
>> + return 0;
>> +
>> + return *p == data;
>> +}
>> +
>> +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 <[email protected]>");
>> +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..bfbde2e98340
>> --- /dev/null
>> +++ b/include/linux/led-class-multicolor.h
>> @@ -0,0 +1,124 @@
>> +/* 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 {
>> + /* led class device */
>> + struct led_classdev led_cdev;
>> +
>> + struct device_attribute color_max_intensity_attr;
>> + struct device_attribute color_intensity_attr;
>> + struct device_attribute color_index_attr;
> These are no longer needed as you define attrs statically.
Ack
>> +
>> + int color_brightness[LED_COLOR_ID_MAX];
>> +
>> + int color_led_intensity[LED_COLOR_ID_MAX];
>> + int color_led_max_intensity[LED_COLOR_ID_MAX];
>> + const char *color_index[LED_COLOR_ID_MAX];
> I think that we should get back to the available_colors bitmask
> and allow the framework to allocate arrays by itself.
> And yes, all the above should be pointers.
>
> Driver would only need to set led_mcdev->available_colors bits.
Nack to the available_colors. I did this originally and the issue is
that the driver sets the bits in available_colors and no matter what the
order is in the DT file the indexing is always red green and blue per
the LED_COLORS array. The framework has no legitimate way to know the
order in which the colors were added.
This posed an issue with the LP55xx code as the RGB was defined with
different colors assigned to different channels. Green was 0 blue was 2
and red was 6. So the driver would have to map the channels to the
colors. In forcing the device driver to set the color index it can then
map the output channels itself. The framework should not care what
channel is for what color. In either case the device driver will need
to store the color index mapped to the channel output but having the
index to color being a 1-1 mapping made the code much simpler for the
device driver.
Basically it turned out to be a simple for loop that just stored both
channel and color as opposed to having to re-map the colors to indexes.
So for the LP55xx I can get an index of green, blue red and that maps to
the channels per the DT. I don't think the framework should enforce a
standard color index array ordering.
If we use the available_colors we don't even need the color_index and we
can just pass the available_colors to the user space as a u32 and let
the user space figure what colors are available. Which means the user
space would assume the order per the LED_COLORS array.
Dan
Dan,
On 3/28/20 10:31 PM, Dan Murphy wrote:
> Jacek
>
> Thanks for the review
>
> On 3/28/20 9:03 AM, Jacek Anaszewski wrote:
>> Hi Dan,
>>
>> Thanks for the update. The picture would be more complete if
>> the patch set contained a client though.
>
> I was going to send the ones I did but they are pretty dirty from a code
> stand point.
>
> Besides I would expect the framework to change which then changes the
> driver code.
>
>> Anyway, please find my review remarks below.
>>
>> On 3/24/20 7:14 PM, Dan Murphy wrote:
>>> Introduce a multicolor class that groups colored LEDs
>>> within a LED node.
>>>
[...]
>>> +What: /sys/class/leds/<led>/color_max_intensity
>>> +Date: March 2020
>>> +KernelVersion: 5.8
>>> +Contact: Dan Murphy <[email protected]>
>>> +Description: read
>>> + Maximum intensity level for the LED color within the array.
>>> + The max intensities for each color must be entered based on the
>>> + color_index array.
>> I wonder if we should mention here that each LED within a cluster should
>> have the same maximum intensity for linear color lightness calculation
>> via brightness file.
>
> Does it really have to?
Say we have two LEDs:
red, intensity = 255, max_intensity = 255
green, intensity = 15, max_intensity = 15
If setting global brightness to 127 we have:
led[red].brightness = 127 * 255 / 255 = 127
led[green].brightness = 127 * 15 / 15 = 15 (cut down to max_intensity)
Clearly for green LED you're not getting a value being a half of
the intensity range.
In addition to my previous statement, global max_brightness
should also have the same value as all max color intensities.
[...]
>>> +Directory Layout Example
>>> +========================
>>> +root:/sys/class/leds/multicolor:grouped_leds# ls -lR
>>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16 brightness
>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_index
>>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16
>>> color_intensity
>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16
>>> color_max_intensity
>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 num_color_leds
>>> +
>>> +Multicolor Class Brightness Control
>>> +===================================
>>> +The multiclor class framework will calculate each monochrome LEDs
>>> intensity.
>>> +
>>> +The brightness level for each LED is calculated based on the color LED
>>> +intensity setting divided by the color LED max intensity setting
>>> multiplied by
>>> +the requested brightness.
>>> +
>>> +led_brightness = brightness * color_intensity/color_max_intensity
>> Maybe some pseudo code would allow for better understanding here:
>>
>> for color in color_intensity
>> led[color].brightness = brightness *
>> led[color].intensity / led[color].max_intensity
> I think this would be fine at least there is a documented equation. I
> don't think we need to document the code.
You mean what would be fine - my or your solution ? :-)
[...]
>>> +static ssize_t color_intensity_store(struct device *dev,
>>> + struct device_attribute *intensity_attr,
>>> + const char *buf, size_t size)
>>> +{
>>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
>>> + int nrchars, offset = 0;
>>> + int intensity_value[LED_COLOR_ID_MAX];
>>> + int i;
>>> + ssize_t ret;
>>> +
>>> + mutex_lock(&led_cdev->led_access);
>>> +
>>> + for (i = 0; i < priv->num_colors; i++) {
>>> + ret = sscanf(buf + offset, "%i%n",
>>> + &intensity_value[i], &nrchars);
>>> + if (ret != 1) {
>>> + dev_err(led_cdev->dev,
>>> + "Incorrect number of LEDs expected %i values
>>> intensity was not applied\n",
>>> + priv->num_colors);
>>> + goto err_out;
>>> + }
>>> + offset += nrchars;
>>> + }
>> I've just realized that moving to single color_intensity file
>> doesn't allow setting all colors together with new brightness
>> atomically. In effect, we will need to pass brightness to this file too,
>> if we want to avoid "interesting" latching via brightenss file.
>>
>> Then we would need to call led_set_brightness() from here as well.
>
> Why? This just caches the intensity of each colored LED. Then the
> actual brightness is calculated only when the brightness file is updated.
And this is wrong. We should be able to set the color with a single
write.
> This would be an automatic update of the LED and that is not the intent
> of the intensity file per the documentation.
Documentation needs to be changed then.
> The user should be able to set the colors x number of times before the
> LED group is actually updated with the brightness.
What benefit would stem from that? In fact we should be able to
atomically set color in two ways, either via brightness or via
color_intensity file.
But in previous message I unnecessarily proposed the addition
of brightness to the color_intensity interface. It is not needed
since updating color intensities should be considered as setting
entirely new color and that should reset global brightness to
max_brightness.
Therefore here we should call at the end:
led_set_brightness(led_cdev, led_cdev->max_brightness);
That will update each color LED with new brightness values which
will correspond exactly to the color intensities just written.
[...]
>>> diff --git a/include/linux/led-class-multicolor.h
>>> b/include/linux/led-class-multicolor.h
>>> new file mode 100644
>>> index 000000000000..bfbde2e98340
>>> --- /dev/null
>>> +++ b/include/linux/led-class-multicolor.h
>>> @@ -0,0 +1,124 @@
>>> +/* 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 {
>>> + /* led class device */
>>> + struct led_classdev led_cdev;
>>> +
>>> + struct device_attribute color_max_intensity_attr;
>>> + struct device_attribute color_intensity_attr;
>>> + struct device_attribute color_index_attr;
>> These are no longer needed as you define attrs statically.
> Ack
>>> +
>>> + int color_brightness[LED_COLOR_ID_MAX];
>>> +
>>> + int color_led_intensity[LED_COLOR_ID_MAX];
>>> + int color_led_max_intensity[LED_COLOR_ID_MAX];
>>> + const char *color_index[LED_COLOR_ID_MAX];
>> I think that we should get back to the available_colors bitmask
>> and allow the framework to allocate arrays by itself.
>> And yes, all the above should be pointers.
>>
>> Driver would only need to set led_mcdev->available_colors bits.
>
> Nack to the available_colors. I did this originally and the issue is
> that the driver sets the bits in available_colors and no matter what the
> order is in the DT file the indexing is always red green and blue per
> the LED_COLORS array. The framework has no legitimate way to know the
> order in which the colors were added.
>
> This posed an issue with the LP55xx code as the RGB was defined with
> different colors assigned to different channels. Green was 0 blue was 2
> and red was 6. So the driver would have to map the channels to the
> colors. In forcing the device driver to set the color index it can then
> map the output channels itself. The framework should not care what
> channel is for what color. In either case the device driver will need
> to store the color index mapped to the channel output but having the
> index to color being a 1-1 mapping made the code much simpler for the
> device driver.
>
> Basically it turned out to be a simple for loop that just stored both
> channel and color as opposed to having to re-map the colors to indexes.
>
> So for the LP55xx I can get an index of green, blue red and that maps to
> the channels per the DT. I don't think the framework should enforce a
> standard color index array ordering.
OK, if that indeed helps simplifying the code on the driver side.
But maybe it would be possible to come up with some generic helpers
for color sub-LEDs initialization?
> If we use the available_colors we don't even need the color_index and we
> can just pass the available_colors to the user space as a u32 and let
> the user space figure what colors are available. Which means the user
> space would assume the order per the LED_COLORS array.
Sysfs should be rather human readable so this would not necessarily
need to be the case.
--
Best regards,
Jacek Anaszewski
Jacek
On 3/29/20 7:47 AM, Jacek Anaszewski wrote:
> Dan,
>
> On 3/28/20 10:31 PM, Dan Murphy wrote:
>> Jacek
>>
>> Thanks for the review
>>
>> On 3/28/20 9:03 AM, Jacek Anaszewski wrote:
>>> Hi Dan,
>>>
>>> Thanks for the update. The picture would be more complete if
>>> the patch set contained a client though.
>> I was going to send the ones I did but they are pretty dirty from a code
>> stand point.
>>
>> Besides I would expect the framework to change which then changes the
>> driver code.
>>
>>> Anyway, please find my review remarks below.
>>>
>>> On 3/24/20 7:14 PM, Dan Murphy wrote:
>>>> Introduce a multicolor class that groups colored LEDs
>>>> within a LED node.
>>>>
> [...]
>>>> +What: /sys/class/leds/<led>/color_max_intensity
>>>> +Date: March 2020
>>>> +KernelVersion: 5.8
>>>> +Contact: Dan Murphy <[email protected]>
>>>> +Description: read
>>>> + Maximum intensity level for the LED color within the array.
>>>> + The max intensities for each color must be entered based on the
>>>> + color_index array.
>>> I wonder if we should mention here that each LED within a cluster should
>>> have the same maximum intensity for linear color lightness calculation
>>> via brightness file.
>> Does it really have to?
> Say we have two LEDs:
>
> red, intensity = 255, max_intensity = 255
> green, intensity = 15, max_intensity = 15
>
> If setting global brightness to 127 we have:
>
> led[red].brightness = 127 * 255 / 255 = 127
> led[green].brightness = 127 * 15 / 15 = 15 (cut down to max_intensity)
>
> Clearly for green LED you're not getting a value being a half of
> the intensity range.
>
> In addition to my previous statement, global max_brightness
> should also have the same value as all max color intensities.
OK I will update the code and docs to indicate max_intensity should
equal max_brightness.
>
> [...]
>>>> +Directory Layout Example
>>>> +========================
>>>> +root:/sys/class/leds/multicolor:grouped_leds# ls -lR
>>>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16 brightness
>>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_index
>>>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16
>>>> color_intensity
>>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16
>>>> color_max_intensity
>>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 num_color_leds
>>>> +
>>>> +Multicolor Class Brightness Control
>>>> +===================================
>>>> +The multiclor class framework will calculate each monochrome LEDs
>>>> intensity.
>>>> +
>>>> +The brightness level for each LED is calculated based on the color LED
>>>> +intensity setting divided by the color LED max intensity setting
>>>> multiplied by
>>>> +the requested brightness.
>>>> +
>>>> +led_brightness = brightness * color_intensity/color_max_intensity
>>> Maybe some pseudo code would allow for better understanding here:
>>>
>>> for color in color_intensity
>>> led[color].brightness = brightness *
>>> led[color].intensity / led[color].max_intensity
>> I think this would be fine at least there is a documented equation. I
>> don't think we need to document the code.
> You mean what would be fine - my or your solution ? :-)
My solution is probably good enough for documentation
>
> [...]
>>>> +static ssize_t color_intensity_store(struct device *dev,
>>>> + struct device_attribute *intensity_attr,
>>>> + const char *buf, size_t size)
>>>> +{
>>>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
>>>> + int nrchars, offset = 0;
>>>> + int intensity_value[LED_COLOR_ID_MAX];
>>>> + int i;
>>>> + ssize_t ret;
>>>> +
>>>> + mutex_lock(&led_cdev->led_access);
>>>> +
>>>> + for (i = 0; i < priv->num_colors; i++) {
>>>> + ret = sscanf(buf + offset, "%i%n",
>>>> + &intensity_value[i], &nrchars);
>>>> + if (ret != 1) {
>>>> + dev_err(led_cdev->dev,
>>>> + "Incorrect number of LEDs expected %i values
>>>> intensity was not applied\n",
>>>> + priv->num_colors);
>>>> + goto err_out;
>>>> + }
>>>> + offset += nrchars;
>>>> + }
>>> I've just realized that moving to single color_intensity file
>>> doesn't allow setting all colors together with new brightness
>>> atomically. In effect, we will need to pass brightness to this file too,
>>> if we want to avoid "interesting" latching via brightenss file.
>>>
>>> Then we would need to call led_set_brightness() from here as well.
>> Why? This just caches the intensity of each colored LED. Then the
>> actual brightness is calculated only when the brightness file is updated.
> And this is wrong. We should be able to set the color with a single
> write.
Well that is a change in philosophy and direction from the prior patches.
You only wanted the LEDs to update when the brightness file was written.
Why the change in stance?
>> This would be an automatic update of the LED and that is not the intent
>> of the intensity file per the documentation.
> Documentation needs to be changed then.
>
>> The user should be able to set the colors x number of times before the
>> LED group is actually updated with the brightness.
> What benefit would stem from that? In fact we should be able to
> atomically set color in two ways, either via brightness or via
> color_intensity file.
>
> But in previous message I unnecessarily proposed the addition
> of brightness to the color_intensity interface. It is not needed
> since updating color intensities should be considered as setting
> entirely new color and that should reset global brightness to
> max_brightness.
>
> Therefore here we should call at the end:
>
> led_set_brightness(led_cdev, led_cdev->max_brightness);
>
> That will update each color LED with new brightness values which
> will correspond exactly to the color intensities just written.
>
> [...]
>>>> diff --git a/include/linux/led-class-multicolor.h
>>>> b/include/linux/led-class-multicolor.h
>>>> new file mode 100644
>>>> index 000000000000..bfbde2e98340
>>>> --- /dev/null
>>>> +++ b/include/linux/led-class-multicolor.h
>>>> @@ -0,0 +1,124 @@
>>>> +/* 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 {
>>>> + /* led class device */
>>>> + struct led_classdev led_cdev;
>>>> +
>>>> + struct device_attribute color_max_intensity_attr;
>>>> + struct device_attribute color_intensity_attr;
>>>> + struct device_attribute color_index_attr;
>>> These are no longer needed as you define attrs statically.
>> Ack
>>>> +
>>>> + int color_brightness[LED_COLOR_ID_MAX];
>>>> +
>>>> + int color_led_intensity[LED_COLOR_ID_MAX];
>>>> + int color_led_max_intensity[LED_COLOR_ID_MAX];
>>>> + const char *color_index[LED_COLOR_ID_MAX];
>>> I think that we should get back to the available_colors bitmask
>>> and allow the framework to allocate arrays by itself.
>>> And yes, all the above should be pointers.
>>>
>>> Driver would only need to set led_mcdev->available_colors bits.
>> Nack to the available_colors. I did this originally and the issue is
>> that the driver sets the bits in available_colors and no matter what the
>> order is in the DT file the indexing is always red green and blue per
>> the LED_COLORS array. The framework has no legitimate way to know the
>> order in which the colors were added.
>>
>> This posed an issue with the LP55xx code as the RGB was defined with
>> different colors assigned to different channels. Green was 0 blue was 2
>> and red was 6. So the driver would have to map the channels to the
>> colors. In forcing the device driver to set the color index it can then
>> map the output channels itself. The framework should not care what
>> channel is for what color. In either case the device driver will need
>> to store the color index mapped to the channel output but having the
>> index to color being a 1-1 mapping made the code much simpler for the
>> device driver.
>>
>> Basically it turned out to be a simple for loop that just stored both
>> channel and color as opposed to having to re-map the colors to indexes.
>>
>> So for the LP55xx I can get an index of green, blue red and that maps to
>> the channels per the DT. I don't think the framework should enforce a
>> standard color index array ordering.
> OK, if that indeed helps simplifying the code on the driver side.
> But maybe it would be possible to come up with some generic helpers
> for color sub-LEDs initialization?
Well the only helpers I can think of is a DT helper to parse the
Multicolor child nodes. But that maybe more complicated for the LP55xx
as the children have additional
properties to parse through. But the multicolor framework can parse the
color and channel which means we would have to mandate the use of a
specific common property node like
led_sources even if it is just a single channel.
>> If we use the available_colors we don't even need the color_index and we
>> can just pass the available_colors to the user space as a u32 and let
>> the user space figure what colors are available. Which means the user
>> space would assume the order per the LED_COLORS array.
> Sysfs should be rather human readable so this would not necessarily
> need to be the case.
>
That is what I was thinking as well.
Dan
Dan,
On 3/29/20 5:14 PM, Dan Murphy wrote:
> Jacek
>
> On 3/29/20 7:47 AM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 3/28/20 10:31 PM, Dan Murphy wrote:
>>> Jacek
>>>
>>> Thanks for the review
>>>
>>> On 3/28/20 9:03 AM, Jacek Anaszewski wrote:
>>>> Hi Dan,
>>>>
>>>> Thanks for the update. The picture would be more complete if
>>>> the patch set contained a client though.
>>> I was going to send the ones I did but they are pretty dirty from a code
>>> stand point.
>>>
>>> Besides I would expect the framework to change which then changes the
>>> driver code.
>>>
>>>> Anyway, please find my review remarks below.
>>>>
>>>> On 3/24/20 7:14 PM, Dan Murphy wrote:
>>>>> Introduce a multicolor class that groups colored LEDs
>>>>> within a LED node.
>>>>>
>> [...]
>>>>> +What: /sys/class/leds/<led>/color_max_intensity
>>>>> +Date: March 2020
>>>>> +KernelVersion: 5.8
>>>>> +Contact: Dan Murphy <[email protected]>
>>>>> +Description: read
>>>>> + Maximum intensity level for the LED color within the array.
>>>>> + The max intensities for each color must be entered based
>>>>> on the
>>>>> + color_index array.
>>>> I wonder if we should mention here that each LED within a cluster
>>>> should
>>>> have the same maximum intensity for linear color lightness calculation
>>>> via brightness file.
>>> Does it really have to?
>> Say we have two LEDs:
>>
>> red, intensity = 255, max_intensity = 255
>> green, intensity = 15, max_intensity = 15
>>
>> If setting global brightness to 127 we have:
>>
>> led[red].brightness = 127 * 255 / 255 = 127
>> led[green].brightness = 127 * 15 / 15 = 15 (cut down to max_intensity)
>>
>> Clearly for green LED you're not getting a value being a half of
>> the intensity range.
>>
>> In addition to my previous statement, global max_brightness
>> should also have the same value as all max color intensities.
> OK I will update the code and docs to indicate max_intensity should
> equal max_brightness.
>>
>> [...]
>>>>> +Directory Layout Example
>>>>> +========================
>>>>> +root:/sys/class/leds/multicolor:grouped_leds# ls -lR
>>>>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16 brightness
>>>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16 color_index
>>>>> +-rw-r--r-- 1 root root 4096 Oct 19 16:16
>>>>> color_intensity
>>>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16
>>>>> color_max_intensity
>>>>> +-r--r--r-- 1 root root 4096 Oct 19 16:16
>>>>> num_color_leds
>>>>> +
>>>>> +Multicolor Class Brightness Control
>>>>> +===================================
>>>>> +The multiclor class framework will calculate each monochrome LEDs
>>>>> intensity.
>>>>> +
>>>>> +The brightness level for each LED is calculated based on the color
>>>>> LED
>>>>> +intensity setting divided by the color LED max intensity setting
>>>>> multiplied by
>>>>> +the requested brightness.
>>>>> +
>>>>> +led_brightness = brightness * color_intensity/color_max_intensity
>>>> Maybe some pseudo code would allow for better understanding here:
>>>>
>>>> for color in color_intensity
>>>> led[color].brightness = brightness *
>>>> led[color].intensity / led[color].max_intensity
>>> I think this would be fine at least there is a documented equation. I
>>> don't think we need to document the code.
>> You mean what would be fine - my or your solution ? :-)
> My solution is probably good enough for documentation
OK, this description actually mentions "The brightness level for each
LED" so it should be fine as is.
>> [...]
>>>>> +static ssize_t color_intensity_store(struct device *dev,
>>>>> + struct device_attribute *intensity_attr,
>>>>> + const char *buf, size_t size)
>>>>> +{
>>>>> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>>>> + struct led_classdev_mc *priv = lcdev_to_mccdev(led_cdev);
>>>>> + int nrchars, offset = 0;
>>>>> + int intensity_value[LED_COLOR_ID_MAX];
>>>>> + int i;
>>>>> + ssize_t ret;
>>>>> +
>>>>> + mutex_lock(&led_cdev->led_access);
>>>>> +
>>>>> + for (i = 0; i < priv->num_colors; i++) {
>>>>> + ret = sscanf(buf + offset, "%i%n",
>>>>> + &intensity_value[i], &nrchars);
>>>>> + if (ret != 1) {
>>>>> + dev_err(led_cdev->dev,
>>>>> + "Incorrect number of LEDs expected %i values
>>>>> intensity was not applied\n",
>>>>> + priv->num_colors);
>>>>> + goto err_out;
>>>>> + }
>>>>> + offset += nrchars;
>>>>> + }
>>>> I've just realized that moving to single color_intensity file
>>>> doesn't allow setting all colors together with new brightness
>>>> atomically. In effect, we will need to pass brightness to this file
>>>> too,
>>>> if we want to avoid "interesting" latching via brightenss file.
>>>>
>>>> Then we would need to call led_set_brightness() from here as well.
>>> Why? This just caches the intensity of each colored LED. Then the
>>> actual brightness is calculated only when the brightness file is
>>> updated.
>> And this is wrong. We should be able to set the color with a single
>> write.
>
> Well that is a change in philosophy and direction from the prior patches.
>
> You only wanted the LEDs to update when the brightness file was written.
> Why the change in stance?
It was devised for an interface with separate sysfs file per color.
Now we can leverage the fact that we will have single file for setting
all color intensities.
>>> This would be an automatic update of the LED and that is not the intent
>>> of the intensity file per the documentation.
>> Documentation needs to be changed then.
>>
>>> The user should be able to set the colors x number of times before the
>>> LED group is actually updated with the brightness.
>> What benefit would stem from that? In fact we should be able to
>> atomically set color in two ways, either via brightness or via
>> color_intensity file.
>>
>> But in previous message I unnecessarily proposed the addition
>> of brightness to the color_intensity interface. It is not needed
>> since updating color intensities should be considered as setting
>> entirely new color and that should reset global brightness to
>> max_brightness.
>>
>> Therefore here we should call at the end:
>>
>> led_set_brightness(led_cdev, led_cdev->max_brightness);
>>
>> That will update each color LED with new brightness values which
>> will correspond exactly to the color intensities just written.
>>
>> [...]
>>>>> diff --git a/include/linux/led-class-multicolor.h
>>>>> b/include/linux/led-class-multicolor.h
>>>>> new file mode 100644
>>>>> index 000000000000..bfbde2e98340
>>>>> --- /dev/null
>>>>> +++ b/include/linux/led-class-multicolor.h
>>>>> @@ -0,0 +1,124 @@
>>>>> +/* 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 {
>>>>> + /* led class device */
>>>>> + struct led_classdev led_cdev;
>>>>> +
>>>>> + struct device_attribute color_max_intensity_attr;
>>>>> + struct device_attribute color_intensity_attr;
>>>>> + struct device_attribute color_index_attr;
>>>> These are no longer needed as you define attrs statically.
>>> Ack
>>>>> +
>>>>> + int color_brightness[LED_COLOR_ID_MAX];
>>>>> +
>>>>> + int color_led_intensity[LED_COLOR_ID_MAX];
>>>>> + int color_led_max_intensity[LED_COLOR_ID_MAX];
>>>>> + const char *color_index[LED_COLOR_ID_MAX];
>>>> I think that we should get back to the available_colors bitmask
>>>> and allow the framework to allocate arrays by itself.
>>>> And yes, all the above should be pointers.
>>>>
>>>> Driver would only need to set led_mcdev->available_colors bits.
>>> Nack to the available_colors. I did this originally and the issue is
>>> that the driver sets the bits in available_colors and no matter what the
>>> order is in the DT file the indexing is always red green and blue per
>>> the LED_COLORS array. The framework has no legitimate way to know the
>>> order in which the colors were added.
>>>
>>> This posed an issue with the LP55xx code as the RGB was defined with
>>> different colors assigned to different channels. Green was 0 blue was 2
>>> and red was 6. So the driver would have to map the channels to the
>>> colors. In forcing the device driver to set the color index it can then
>>> map the output channels itself. The framework should not care what
>>> channel is for what color. In either case the device driver will need
>>> to store the color index mapped to the channel output but having the
>>> index to color being a 1-1 mapping made the code much simpler for the
>>> device driver.
>>>
>>> Basically it turned out to be a simple for loop that just stored both
>>> channel and color as opposed to having to re-map the colors to indexes.
>>>
>>> So for the LP55xx I can get an index of green, blue red and that maps to
>>> the channels per the DT. I don't think the framework should enforce a
>>> standard color index array ordering.
>> OK, if that indeed helps simplifying the code on the driver side.
>> But maybe it would be possible to come up with some generic helpers
>> for color sub-LEDs initialization?
>
> Well the only helpers I can think of is a DT helper to parse the
> Multicolor child nodes. But that maybe more complicated for the LP55xx
> as the children have additional
>
> properties to parse through. But the multicolor framework can parse the
> color and channel which means we would have to mandate the use of a
> specific common property node like
>
> led_sources even if it is just a single channel.
Let's see how the driver will look like after adding array allocations.
Maybe something will come to one's mind after seeing the code.
>>> If we use the available_colors we don't even need the color_index and we
>>> can just pass the available_colors to the user space as a u32 and let
>>> the user space figure what colors are available. Which means the user
>>> space would assume the order per the LED_COLORS array.
>> Sysfs should be rather human readable so this would not necessarily
>> need to be the case.
>>
> That is what I was thinking as well.
>
> Dan
>
--
Best regards,
Jacek Anaszewski
Jacek
On 3/29/20 1:43 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 3/29/20 5:14 PM, Dan Murphy wrote:
<snip>
>
>> Dan
>>
I will wait to hear from Pavel on this patchset since the array
definition was something he was asking for.
Need to make sure this was his intent on wanting the array vs the sysfs
interfaces otherwise the changes maybe futile in nature.
Dan
Hi!
> > > +Description: read
> > > + Maximum intensity level for the LED color within the array.
> > > + The max intensities for each color must be entered based on the
> > > + color_index array.
> > I wonder if we should mention here that each LED within a cluster should
> > have the same maximum intensity for linear color lightness calculation
> > via brightness file.
>
> Does it really have to?
Yes it does. Anything else is unneccessarily complex.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html