| Message ID | 20190925174616.3714-1-dmurphy@ti.com |
|---|---|
| Headers | show |
| Series | Multicolor Framework | expand |
Hi Dan, On Wed, Sep 25, 2019 at 2:41 PM Dan Murphy <dmurphy@ti.com> wrote: > > chan0 { This should be chan@0 > chan-name = "R"; > led-cur = /bits/ 8 <0x20>; > max-cur = /bits/ 8 <0x60>; > + reg = <0>; Passing reg without its corresponding @ entry gives a dtc warning when building with W=1.
Fabio On 9/25/19 1:00 PM, Fabio Estevam wrote: > Hi Dan, > > On Wed, Sep 25, 2019 at 2:41 PM Dan Murphy <dmurphy@ti.com> wrote: > >> chan0 { > This should be chan@0 > >> chan-name = "R"; >> led-cur = /bits/ 8 <0x20>; >> max-cur = /bits/ 8 <0x60>; >> + reg = <0>; > Passing reg without its corresponding @ entry gives a dtc warning when > building with W=1. Ack
Hi Dan, Thank you for the v9. This patch should me melded with 5/15. On 9/25/19 7:46 PM, Dan Murphy wrote: > Add a documentation of LED Multicolor LED class specific > sysfs attributes. > > Add multicolor framework class documentation describing the > usage of the files created. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > .../ABI/testing/sysfs-class-led-multicolor | 35 +++++++ > Documentation/leds/index.rst | 1 + > Documentation/leds/leds-class-multicolor.rst | 96 +++++++++++++++++++ > 3 files changed, 132 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-led-multicolor > create mode 100644 Documentation/leds/leds-class-multicolor.rst > > diff --git a/Documentation/ABI/testing/sysfs-class-led-multicolor b/Documentation/ABI/testing/sysfs-class-led-multicolor > new file mode 100644 > index 000000000000..65cb43de26e6 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor > @@ -0,0 +1,35 @@ > +What: /sys/class/leds/<led>/brightness > +Date: Sept 2019 > +KernelVersion: 5.5 > +Contact: Dan Murphy <dmurphy@ti.com> > +Description: read/write > + Writing to this file will update all LEDs within the group to a > + calculated percentage of what each color LED intensity is set > + to. The percentage is calculated via the equation below: > + > + led_brightness = brightness * <color>_intensity/<color>_max_intensity > + > + For additional details please refer to > + Documentation/leds/leds-class-multicolor.rst. > + > + The value of the color is from 0 to > + /sys/class/leds/<led>/max_brightness. > + > +What: /sys/class/leds/<led>/colors/<color>_intensity > +Date: Sept 2019 > +KernelVersion: 5.5 > +Contact: Dan Murphy <dmurphy@ti.com> > +Description: read/write > + The <color>_intensity file is created based on the color > + defined by the registrar of the class. > + There is one file per color presented. > + > + The value of the color is from 0 to > + /sys/class/leds/<led>/colors/<color>_max_intensity. > + > +What: /sys/class/leds/<led>/colors/<color>_max_intensity > +Date: Sept 2019 > +KernelVersion: 5.5 > +Contact: Dan Murphy <dmurphy@ti.com> > +Description: read only > + Maximum intensity level for the LED color. > diff --git a/Documentation/leds/index.rst b/Documentation/leds/index.rst > index 060f4e485897..bc70c6aa7138 100644 > --- a/Documentation/leds/index.rst > +++ b/Documentation/leds/index.rst > @@ -9,6 +9,7 @@ LEDs > > leds-class > leds-class-flash > + leds-class-multicolor > ledtrig-oneshot > ledtrig-transient > ledtrig-usbport > diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst > new file mode 100644 > index 000000000000..87a1588d7619 > --- /dev/null > +++ b/Documentation/leds/leds-class-multicolor.rst > @@ -0,0 +1,96 @@ > +==================================== > +Multi Color LED handling under Linux > +==================================== > + > +Description > +=========== > +The multi color class groups monochrome LEDs and allows controlling two > +aspects of the final combined color: hue and lightness. The former is > +controlled via <color>_intensity files and the latter is controlled > +via brightness file. > + > +For more details on hue and lightness notions please refer to > +https://en.wikipedia.org/wiki/CIECAM02. > + > +Note that intensity files only cache the written value and the actual > +change of hardware state occurs upon writing brightness file. This > +allows for changing many factors of the perceived color in a virtually > +unnoticeable way for the human observer. > + > +Multicolor Class Control > +======================== > +The multicolor class presents the LED groups under a directory called "colors". > +This directory is a child under the LED parent node created by the led_class > +framework. The led_class framework is documented in led-class.rst within this > +documentation directory. > + > +Each colored LED will have two files created under the colors directory > +<color>_intensity and <color>_max_intensity. These files will contain > +one of LED_COLOR_ID_* definitions from the header > +include/dt-bindings/leds/common.h. > + > +Directory Layout Example > +======================== > +root:/sys/class/leds/rgb:grouped_leds# ls -lR colors/ > +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 red_max_intensity > +--w--wx-w- 1 root root 4096 Jul 7 03:10 red_intensity > +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 green_max_intensity > +--w--wx-w- 1 root root 4096 Jul 7 03:10 green_intensity > +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 blue_max_intensity > +--w--wx-w- 1 root root 4096 Jul 7 03:10 blue_intensity > + > +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 > + > +Example: > +Three LEDs are present in the group as defined in "Directory Layout Example" > +within this document. > + > +A user first writes the color LED brightness file with the brightness level that > +is necessary to achieve a blueish violet output from the RGB LED group. > + > +echo 138 > /sys/class/leds/rgb:grouped_leds/red_intensity > +echo 43 > /sys/class/leds/rgb:grouped_leds/green_intensity > +echo 226 > /sys/class/leds/rgb:grouped_leds/blue_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/rgb:grouped_leds/max_brightness > +255 > +echo 128 > /sys/class/leds/rgb: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/rgb:grouped_leds/max_brightness > +255 > + > +echo 128 > /sys/class/leds/rgb:grouped_leds/brightness > + > +cat /sys/class/leds/rgb:grouped_leds/brightness > +128 > -- Best regards, Jacek Anaszewski
Dan, On 9/25/19 7:46 PM, Dan Murphy wrote: > Introduce a multicolor class that groups colored LEDs > within a LED node. > > The multi color class groups monochrome LEDs and allows controlling two > aspects of the final combined color: hue and lightness. The former is > controlled via <color>_intensity files and the latter is controlled > via brightness file. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > drivers/leds/Kconfig | 10 ++ > drivers/leds/Makefile | 1 + > drivers/leds/led-class-multicolor.c | 220 +++++++++++++++++++++++++++ > include/linux/led-class-multicolor.h | 74 +++++++++ > 4 files changed, 305 insertions(+) > create mode 100644 drivers/leds/led-class-multicolor.c > create mode 100644 include/linux/led-class-multicolor.h > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 6e7703fd03d0..cfb1ebb6517f 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 2da39e896ce8..841038cfe35b 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..25371bd9a860 > --- /dev/null > +++ b/drivers/leds/led-class-multicolor.c > @@ -0,0 +1,220 @@ > +// 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" > + > +#define INTENSITY_NAME "_intensity" > +#define MAX_INTENSITY_NAME "_max_intensity" > + > +void led_mc_calc_brightness(struct led_classdev_mc *mcled_cdev, > + enum led_brightness brightness, > + int brightness_val[]) > +{ > + struct led_mc_color_entry *priv; > + int i = 0; > + > + list_for_each_entry(priv, &mcled_cdev->color_list, list) { I think we should have some way to let the caller know exact mapping of brightness_val to color_id. Possibly LED mc core should provide a helper to get color_id by color entry index. But this remark is actually more relevant to the place of calling. I'll try to propose something there. And regarding brightness_val name - how about: s/brightness_val/brightness_component/ ? > + brightness_val[i] = brightness * > + priv->intensity / priv->max_intensity; > + i++; > + } > +} > +EXPORT_SYMBOL_GPL(led_mc_calc_brightness); > + > +static ssize_t intensity_store(struct device *dev, > + struct device_attribute *intensity_attr, > + const char *buf, size_t size) > +{ > + struct led_mc_color_entry *priv = container_of(intensity_attr, > + struct led_mc_color_entry, > + intensity_attr); > + struct led_classdev *led_cdev = priv->mcled_cdev->led_cdev; > + unsigned long value; > + ssize_t ret; > + > + mutex_lock(&led_cdev->led_access); > + > + ret = kstrtoul(buf, 10, &value); > + if (ret) > + goto unlock; > + > + if (value > priv->max_intensity) { > + ret = -EINVAL; > + goto unlock; > + } > + > + priv->intensity = value; > + ret = size; > + > +unlock: > + mutex_unlock(&led_cdev->led_access); > + return ret; > +} > + > +static ssize_t intensity_show(struct device *dev, > + struct device_attribute *intensity_attr, > + char *buf) > +{ > + struct led_mc_color_entry *priv = container_of(intensity_attr, > + struct led_mc_color_entry, > + intensity_attr); > + > + return sprintf(buf, "%d\n", priv->intensity); > +} > + > +static ssize_t max_intensity_show(struct device *dev, > + struct device_attribute *max_intensity_attr, > + char *buf) > +{ > + struct led_mc_color_entry *priv = container_of(max_intensity_attr, > + struct led_mc_color_entry, > + max_intensity_attr); > + > + return sprintf(buf, "%d\n", priv->max_intensity); > +} > + > +static struct attribute *led_color_attrs[] = { > + NULL, > +}; > + > +static struct attribute_group led_color_group = { > + .name = "colors", > + .attrs = led_color_attrs, > +}; > + > +static int led_multicolor_init_color(struct led_classdev_mc *mcled_cdev, > + int color_id, int color_index) > +{ > + struct led_classdev *led_cdev = mcled_cdev->led_cdev; > + struct led_mc_color_entry *mc_priv; > + char *intensity_file_name; > + char *max_intensity_file_name; > + size_t len; > + int ret; > + > + mc_priv = devm_kzalloc(led_cdev->dev, sizeof(*mc_priv), GFP_KERNEL); > + if (!mc_priv) > + return -ENOMEM; > + > + mc_priv->led_color_id = color_id; > + mc_priv->mcled_cdev = mcled_cdev; > + > + sysfs_attr_init(&mc_priv->intensity_attr.attr); > + len = strlen(led_colors[color_id]) + strlen(INTENSITY_NAME) + 1; > + intensity_file_name = kzalloc(len, GFP_KERNEL); > + if (!intensity_file_name) > + return -ENOMEM; > + > + snprintf(intensity_file_name, len, "%s%s", > + led_colors[color_id], INTENSITY_NAME); > + mc_priv->intensity_attr.attr.name = intensity_file_name; > + mc_priv->intensity_attr.attr.mode = 644; Proper octal value should begin with 0. But please use combinations of dedicated S_I* definitions from include/uapi/linux/stat.h. > + mc_priv->intensity_attr.store = intensity_store; > + mc_priv->intensity_attr.show = intensity_show; > + ret = sysfs_add_file_to_group(&led_cdev->dev->kobj, > + &mc_priv->intensity_attr.attr, > + led_color_group.name); > + if (ret) > + goto intensity_err_out; > + > + sysfs_attr_init(&mc_priv->max_intensity_attr.attr); > + len = strlen(led_colors[color_id]) + strlen(MAX_INTENSITY_NAME) + 1; > + max_intensity_file_name = kzalloc(len, GFP_KERNEL); > + if (!max_intensity_file_name) { > + ret = -ENOMEM; > + goto intensity_err_out; > + } > + > + snprintf(max_intensity_file_name, len, "%s%s", > + led_colors[color_id], MAX_INTENSITY_NAME); > + mc_priv->max_intensity_attr.attr.name = max_intensity_file_name; > + mc_priv->max_intensity_attr.attr.mode = 444; > + mc_priv->max_intensity_attr.show = max_intensity_show; > + ret = sysfs_add_file_to_group(&led_cdev->dev->kobj, > + &mc_priv->max_intensity_attr.attr, > + led_color_group.name); > + if (ret) > + goto max_intensity_err_out; > + > + mc_priv->max_intensity = LED_FULL; > + list_add_tail(&mc_priv->list, &mcled_cdev->color_list); > + > +max_intensity_err_out: > + kfree(max_intensity_file_name); > +intensity_err_out: > + kfree(intensity_file_name); > + return ret; > +} > + > +static int led_multicolor_init_color_dir(struct led_classdev_mc *mcled_cdev) > +{ > + struct led_classdev *led_cdev = mcled_cdev->led_cdev; > + int ret; > + int i, color_index = 0; > + > + ret = sysfs_create_group(&led_cdev->dev->kobj, &led_color_group); > + if (ret) > + return ret; > + > + for (i = 0; i < LED_COLOR_ID_MAX; i++) { > + if (test_bit(i, &mcled_cdev->available_colors)) { > + ret = led_multicolor_init_color(mcled_cdev, i, > + color_index); color_index is now unused AFAICS. > + if (ret) > + break; > + > + color_index++; > + } > + } > + > + return ret; > +} > + > +int led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data) > +{ > + struct led_classdev *led_cdev; > + int ret; > + > + if (!mcled_cdev) > + return -EINVAL; > + > + led_cdev = mcled_cdev->led_cdev; > + INIT_LIST_HEAD(&mcled_cdev->color_list); > + > + /* Register led class device */ > + ret = led_classdev_register_ext(parent, led_cdev, init_data); > + if (ret) > + return ret; > + > + return led_multicolor_init_color_dir(mcled_cdev); > +} > +EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext); Why devm_* versions are missing now? > + > +void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) > +{ > + struct led_mc_color_entry *priv, *next; > + > + if (!mcled_cdev) > + return; > + > + list_for_each_entry_safe(priv, next, &mcled_cdev->color_list, list) > + list_del(&priv->list); > + > + sysfs_remove_group(&mcled_cdev->led_cdev->dev->kobj, &led_color_group); > + led_classdev_unregister(mcled_cdev->led_cdev); > +} > +EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister); > + > +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); > +MODULE_DESCRIPTION("Multi Color LED class interface"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h > new file mode 100644 > index 000000000000..280ba5a614b4 > --- /dev/null > +++ b/include/linux/led-class-multicolor.h > @@ -0,0 +1,74 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* LED Multicolor class interface > + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ > + */ > + > +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED > +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED > + > +#include <linux/leds.h> > +#include <dt-bindings/leds/common.h> > + > +struct led_classdev_mc; > + > +struct led_mc_color_entry { > + struct led_classdev_mc *mcled_cdev; > + > + struct device_attribute max_intensity_attr; > + struct device_attribute intensity_attr; > + > + enum led_brightness max_intensity; > + enum led_brightness intensity; > + > + struct list_head list; > + > + int led_color_id; > +}; > + > +struct led_classdev_mc { > + /* led class device */ > + struct led_classdev *led_cdev; > + struct list_head color_list; > + > + unsigned long available_colors; > + int num_leds; > +}; > + > +static inline struct led_classdev_mc *lcdev_to_mccdev( > + struct led_classdev *lcdev) > +{ > + return container_of(lcdev, struct led_classdev_mc, led_cdev); > +} > + > +/** > + * led_classdev_multicolor_register_ext - register a new object of led_classdev > + * class with support for multicolor LEDs > + * @parent: the multicolor LED to register > + * @mcled_cdev: the led_classdev_mc structure for this device > + * @init_data: the LED class Multi color device initialization data > + * > + * Returns: 0 on success or negative error value on failure > + */ > +int led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data); > + > +#define led_classdev_multicolor_register(parent, mcled_cdev) \ > + led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL) Please turn it into inline. > + > +/** > + * 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 */ > +void led_mc_calc_brightness(struct led_classdev_mc *mcled_cdev, > + enum led_brightness brightness, > + int brightness_val[]); > + > +#endif /* __LINUX_MULTICOLOR_LEDS_H_INCLUDED */ > -- Best regards, Jacek Anaszewski
Dan, On 9/25/19 7:46 PM, Dan Murphy wrote: > Update the DT binding to include the properties to use the > multicolor framework for the devices that use the LP55xx > framework. > > Signed-off-by: Dan Murphy <dmurphy@ti.com> > CC: Tony Lindgren <tony@atomide.com> > CC: "Benoît Cousson" <bcousson@baylibre.com> > CC: Linus Walleij <linus.walleij@linaro.org> > CC: Shawn Guo <shawnguo@kernel.org> > CC: Sascha Hauer <s.hauer@pengutronix.de> > CC: Pengutronix Kernel Team <kernel@pengutronix.de> > CC: Fabio Estevam <festevam@gmail.com> > CC: NXP Linux Team <linux-imx@nxp.com> > --- > .../devicetree/bindings/leds/leds-lp55xx.txt | 99 +++++++++++++++++++ > 1 file changed, 99 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/leds-lp55xx.txt b/Documentation/devicetree/bindings/leds/leds-lp55xx.txt > index bfe2805c5534..c120d2bde3bd 100644 > --- a/Documentation/devicetree/bindings/leds/leds-lp55xx.txt > +++ b/Documentation/devicetree/bindings/leds/leds-lp55xx.txt > @@ -1,6 +1,8 @@ > Binding for TI/National Semiconductor LP55xx LED Drivers > > Required properties: > +- #address-cells: 1 > +- #size-cells: 0 > - compatible: one of > national,lp5521 > national,lp5523 > @@ -14,6 +16,18 @@ Required properties: > Each child has own specific current settings > - led-cur: Current setting at each LED channel (mA x10, 0 if LED is not connected) > - max-cur: Maximun current at each LED channel. > +- reg: Output channel for the LED. This is zero based channel identifier and > + the data sheet is a one based channel identifier. > + reg value to output to LED output number > + D1 = reg value is 0 > + D2 = reg value is 1 > + D3 = reg value is 2 > + D4 = reg value is 3 > + D5 = reg value is 4 > + D6 = reg value is 5 > + D7 = reg value is 6 > + D8 = reg value is 7 > + D9 = reg value is 8 > > Optional properties: > - enable-gpio: GPIO attached to the chip's enable pin > @@ -35,23 +49,28 @@ example 1) LP5521 > on channel 0. > > lp5521@32 { > + #address-cells = <1>; > + #size-cells = <0>; > compatible = "national,lp5521"; > reg = <0x32>; > label = "lp5521_pri"; > clock-mode = /bits/ 8 <2>; > > chan0 { s/chan0/chan@0/ The same applies to the remaining occurrences in this file and to the other patches from the patch set as already pointed out. > + reg = <0>; > led-cur = /bits/ 8 <0x2f>; > max-cur = /bits/ 8 <0x5f>; > linux,default-trigger = "heartbeat"; > }; > -- Best regards, Jacek Anaszewski
Hi! > diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst > new file mode 100644 > index 000000000000..87a1588d7619 > --- /dev/null > +++ b/Documentation/leds/leds-class-multicolor.rst > @@ -0,0 +1,96 @@ > +==================================== > +Multi Color LED handling under Linux > +==================================== > + > +Description > +=========== > +The multi color class groups monochrome LEDs and allows controlling two > +aspects of the final combined color: hue and lightness. The former is > +controlled via <color>_intensity files and the latter is controlled > +via brightness file. > + > +For more details on hue and lightness notions please refer to > +https://en.wikipedia.org/wiki/CIECAM02. > + > +Note that intensity files only cache the written value and the actual > +change of hardware state occurs upon writing brightness file. This > +allows for changing many factors of the perceived color in a virtually > +unnoticeable way for the human observer. So unlike previous versions, userspace will need to write 4 files instead of one in the common case. > +Directory Layout Example > +======================== > +root:/sys/class/leds/rgb:grouped_leds# ls -lR colors/ > +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 red_max_intensity > +--w--wx-w- 1 root root 4096 Jul 7 03:10 red_intensity > +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 green_max_intensity > +--w--wx-w- 1 root root 4096 Jul 7 03:10 green_intensity > +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 blue_max_intensity > +--w--wx-w- 1 root root 4096 Jul 7 03:10 blue_intensity Permissions are way off here. > +A user first writes the color LED brightness file with the brightness level that > +is necessary to achieve a blueish violet output from the RGB LED group. > + > +echo 138 > /sys/class/leds/rgb:grouped_leds/red_intensity > +echo 43 > /sys/class/leds/rgb:grouped_leds/green_intensity > +echo 226 > /sys/class/leds/rgb:grouped_leds/blue_intensity No, you can't tell what kind of color this will result in. Will you be on ELCE/OSS in Lyon? Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Jacek On 9/25/19 4:12 PM, Jacek Anaszewski wrote: > Dan, > > On 9/25/19 7:46 PM, Dan Murphy wrote: >> Introduce a multicolor class that groups colored LEDs >> within a LED node. >> >> The multi color class groups monochrome LEDs and allows controlling two >> aspects of the final combined color: hue and lightness. The former is >> controlled via <color>_intensity files and the latter is controlled >> via brightness file. >> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> drivers/leds/Kconfig | 10 ++ >> drivers/leds/Makefile | 1 + >> drivers/leds/led-class-multicolor.c | 220 +++++++++++++++++++++++++++ >> include/linux/led-class-multicolor.h | 74 +++++++++ >> 4 files changed, 305 insertions(+) >> create mode 100644 drivers/leds/led-class-multicolor.c >> create mode 100644 include/linux/led-class-multicolor.h >> >> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >> index 6e7703fd03d0..cfb1ebb6517f 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 2da39e896ce8..841038cfe35b 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..25371bd9a860 >> --- /dev/null >> +++ b/drivers/leds/led-class-multicolor.c >> @@ -0,0 +1,220 @@ >> +// 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" >> + >> +#define INTENSITY_NAME "_intensity" >> +#define MAX_INTENSITY_NAME "_max_intensity" >> + >> +void led_mc_calc_brightness(struct led_classdev_mc *mcled_cdev, >> + enum led_brightness brightness, >> + int brightness_val[]) >> +{ >> + struct led_mc_color_entry *priv; >> + int i = 0; >> + >> + list_for_each_entry(priv, &mcled_cdev->color_list, list) { > I think we should have some way to let the caller know exact mapping > of brightness_val to color_id. Possibly LED mc core should provide > a helper to get color_id by color entry index. Why would we need that? > > But this remark is actually more relevant to the place of calling. > I'll try to propose something there. > > And regarding brightness_val name - how about: > > s/brightness_val/brightness_component/ ? component does not make sense to me in this context. Actually brightness_val does not make sense either since it is an adjusted intensity and brightness is passed in in the second arg I think intensity_values make more sense. > > >> + brightness_val[i] = brightness * >> + priv->intensity / priv->max_intensity; >> + i++; >> + } >> +} >> +EXPORT_SYMBOL_GPL(led_mc_calc_brightness); >> + >> +static ssize_t intensity_store(struct device *dev, >> + struct device_attribute *intensity_attr, >> + const char *buf, size_t size) >> +{ >> + struct led_mc_color_entry *priv = container_of(intensity_attr, >> + struct led_mc_color_entry, >> + intensity_attr); >> + struct led_classdev *led_cdev = priv->mcled_cdev->led_cdev; >> + unsigned long value; >> + ssize_t ret; >> + >> + mutex_lock(&led_cdev->led_access); >> + >> + ret = kstrtoul(buf, 10, &value); >> + if (ret) >> + goto unlock; >> + >> + if (value > priv->max_intensity) { >> + ret = -EINVAL; >> + goto unlock; >> + } >> + >> + priv->intensity = value; >> + ret = size; >> + >> +unlock: >> + mutex_unlock(&led_cdev->led_access); >> + return ret; >> +} >> + >> +static ssize_t intensity_show(struct device *dev, >> + struct device_attribute *intensity_attr, >> + char *buf) >> +{ >> + struct led_mc_color_entry *priv = container_of(intensity_attr, >> + struct led_mc_color_entry, >> + intensity_attr); >> + >> + return sprintf(buf, "%d\n", priv->intensity); >> +} >> + >> +static ssize_t max_intensity_show(struct device *dev, >> + struct device_attribute *max_intensity_attr, >> + char *buf) >> +{ >> + struct led_mc_color_entry *priv = container_of(max_intensity_attr, >> + struct led_mc_color_entry, >> + max_intensity_attr); >> + >> + return sprintf(buf, "%d\n", priv->max_intensity); >> +} >> + >> +static struct attribute *led_color_attrs[] = { >> + NULL, >> +}; >> + >> +static struct attribute_group led_color_group = { >> + .name = "colors", >> + .attrs = led_color_attrs, >> +}; >> + >> +static int led_multicolor_init_color(struct led_classdev_mc *mcled_cdev, >> + int color_id, int color_index) >> +{ >> + struct led_classdev *led_cdev = mcled_cdev->led_cdev; >> + struct led_mc_color_entry *mc_priv; >> + char *intensity_file_name; >> + char *max_intensity_file_name; >> + size_t len; >> + int ret; >> + >> + mc_priv = devm_kzalloc(led_cdev->dev, sizeof(*mc_priv), GFP_KERNEL); >> + if (!mc_priv) >> + return -ENOMEM; >> + >> + mc_priv->led_color_id = color_id; >> + mc_priv->mcled_cdev = mcled_cdev; >> + >> + sysfs_attr_init(&mc_priv->intensity_attr.attr); >> + len = strlen(led_colors[color_id]) + strlen(INTENSITY_NAME) + 1; >> + intensity_file_name = kzalloc(len, GFP_KERNEL); >> + if (!intensity_file_name) >> + return -ENOMEM; >> + >> + snprintf(intensity_file_name, len, "%s%s", >> + led_colors[color_id], INTENSITY_NAME); >> + mc_priv->intensity_attr.attr.name = intensity_file_name; >> + mc_priv->intensity_attr.attr.mode = 644; > Proper octal value should begin with 0. > But please use combinations of dedicated S_I* definitions > from include/uapi/linux/stat.h. Using the S_I* causes checkpatch warnings WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'. #139: FILE: drivers/leds/led-class-multicolor.c:139: + mc_priv->max_intensity_attr.attr.mode = S_IRUGO; > >> + mc_priv->intensity_attr.store = intensity_store; >> + mc_priv->intensity_attr.show = intensity_show; >> + ret = sysfs_add_file_to_group(&led_cdev->dev->kobj, >> + &mc_priv->intensity_attr.attr, >> + led_color_group.name); >> + if (ret) >> + goto intensity_err_out; >> + >> + sysfs_attr_init(&mc_priv->max_intensity_attr.attr); >> + len = strlen(led_colors[color_id]) + strlen(MAX_INTENSITY_NAME) + 1; >> + max_intensity_file_name = kzalloc(len, GFP_KERNEL); >> + if (!max_intensity_file_name) { >> + ret = -ENOMEM; >> + goto intensity_err_out; >> + } >> + >> + snprintf(max_intensity_file_name, len, "%s%s", >> + led_colors[color_id], MAX_INTENSITY_NAME); >> + mc_priv->max_intensity_attr.attr.name = max_intensity_file_name; >> + mc_priv->max_intensity_attr.attr.mode = 444; >> + mc_priv->max_intensity_attr.show = max_intensity_show; >> + ret = sysfs_add_file_to_group(&led_cdev->dev->kobj, >> + &mc_priv->max_intensity_attr.attr, >> + led_color_group.name); >> + if (ret) >> + goto max_intensity_err_out; >> + >> + mc_priv->max_intensity = LED_FULL; >> + list_add_tail(&mc_priv->list, &mcled_cdev->color_list); >> + >> +max_intensity_err_out: >> + kfree(max_intensity_file_name); >> +intensity_err_out: >> + kfree(intensity_file_name); >> + return ret; >> +} >> + >> +static int led_multicolor_init_color_dir(struct led_classdev_mc *mcled_cdev) >> +{ >> + struct led_classdev *led_cdev = mcled_cdev->led_cdev; >> + int ret; >> + int i, color_index = 0; >> + >> + ret = sysfs_create_group(&led_cdev->dev->kobj, &led_color_group); >> + if (ret) >> + return ret; >> + >> + for (i = 0; i < LED_COLOR_ID_MAX; i++) { >> + if (test_bit(i, &mcled_cdev->available_colors)) { >> + ret = led_multicolor_init_color(mcled_cdev, i, >> + color_index); > color_index is now unused AFAICS. Ack > >> + if (ret) >> + break; >> + >> + color_index++; >> + } >> + } >> + >> + return ret; >> +} >> + >> +int led_classdev_multicolor_register_ext(struct device *parent, >> + struct led_classdev_mc *mcled_cdev, >> + struct led_init_data *init_data) >> +{ >> + struct led_classdev *led_cdev; >> + int ret; >> + >> + if (!mcled_cdev) >> + return -EINVAL; >> + >> + led_cdev = mcled_cdev->led_cdev; >> + INIT_LIST_HEAD(&mcled_cdev->color_list); >> + >> + /* Register led class device */ >> + ret = led_classdev_register_ext(parent, led_cdev, init_data); >> + if (ret) >> + return ret; >> + >> + return led_multicolor_init_color_dir(mcled_cdev); >> +} >> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext); > Why devm_* versions are missing now? I was using the led-class-flash.c as an example and that class does not have the devm_* versions either. Tried to make the 2 child classes look the same. If they are missing from the led-class-flash code then that needs to be fixed as well. > >> + >> +void led_classdev_multicolor_unregister(struct led_classdev_mc *mcled_cdev) >> +{ >> + struct led_mc_color_entry *priv, *next; >> + >> + if (!mcled_cdev) >> + return; >> + >> + list_for_each_entry_safe(priv, next, &mcled_cdev->color_list, list) >> + list_del(&priv->list); >> + >> + sysfs_remove_group(&mcled_cdev->led_cdev->dev->kobj, &led_color_group); >> + led_classdev_unregister(mcled_cdev->led_cdev); >> +} >> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister); >> + >> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); >> +MODULE_DESCRIPTION("Multi Color LED class interface"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/include/linux/led-class-multicolor.h b/include/linux/led-class-multicolor.h >> new file mode 100644 >> index 000000000000..280ba5a614b4 >> --- /dev/null >> +++ b/include/linux/led-class-multicolor.h >> @@ -0,0 +1,74 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* LED Multicolor class interface >> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/ >> + */ >> + >> +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED >> +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED >> + >> +#include <linux/leds.h> >> +#include <dt-bindings/leds/common.h> >> + >> +struct led_classdev_mc; >> + >> +struct led_mc_color_entry { >> + struct led_classdev_mc *mcled_cdev; >> + >> + struct device_attribute max_intensity_attr; >> + struct device_attribute intensity_attr; >> + >> + enum led_brightness max_intensity; >> + enum led_brightness intensity; >> + >> + struct list_head list; >> + >> + int led_color_id; >> +}; >> + >> +struct led_classdev_mc { >> + /* led class device */ >> + struct led_classdev *led_cdev; >> + struct list_head color_list; >> + >> + unsigned long available_colors; >> + int num_leds; >> +}; >> + >> +static inline struct led_classdev_mc *lcdev_to_mccdev( >> + struct led_classdev *lcdev) >> +{ >> + return container_of(lcdev, struct led_classdev_mc, led_cdev); >> +} >> + >> +/** >> + * led_classdev_multicolor_register_ext - register a new object of led_classdev >> + * class with support for multicolor LEDs >> + * @parent: the multicolor LED to register >> + * @mcled_cdev: the led_classdev_mc structure for this device >> + * @init_data: the LED class Multi color device initialization data >> + * >> + * Returns: 0 on success or negative error value on failure >> + */ >> +int led_classdev_multicolor_register_ext(struct device *parent, >> + struct led_classdev_mc *mcled_cdev, >> + struct led_init_data *init_data); >> + >> +#define led_classdev_multicolor_register(parent, mcled_cdev) \ >> + led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL) > Please turn it into inline. Again same statement as above on the led-class-flash. This is how this is defined there. If that is not correct in the flash class then that needs to be fixed as well. Dan > >> + >> +/** >> + * 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 */ >> +void led_mc_calc_brightness(struct led_classdev_mc *mcled_cdev, >> + enum led_brightness brightness, >> + int brightness_val[]); >> + >> +#endif /* __LINUX_MULTICOLOR_LEDS_H_INCLUDED */ >>
Pavel On 9/26/19 6:10 AM, Pavel Machek wrote: > Hi! > >> diff --git a/Documentation/leds/leds-class-multicolor.rst b/Documentation/leds/leds-class-multicolor.rst >> new file mode 100644 >> index 000000000000..87a1588d7619 >> --- /dev/null >> +++ b/Documentation/leds/leds-class-multicolor.rst >> @@ -0,0 +1,96 @@ >> +==================================== >> +Multi Color LED handling under Linux >> +==================================== >> + >> +Description >> +=========== >> +The multi color class groups monochrome LEDs and allows controlling two >> +aspects of the final combined color: hue and lightness. The former is >> +controlled via <color>_intensity files and the latter is controlled >> +via brightness file. >> + >> +For more details on hue and lightness notions please refer to >> +https://en.wikipedia.org/wiki/CIECAM02. >> + >> +Note that intensity files only cache the written value and the actual >> +change of hardware state occurs upon writing brightness file. This >> +allows for changing many factors of the perceived color in a virtually >> +unnoticeable way for the human observer. > So unlike previous versions, userspace will need to write 4 files > instead of one in the common case. Request was made here in v5 of the patchset https://lore.kernel.org/patchwork/patch/1126685/ > >> +Directory Layout Example >> +======================== >> +root:/sys/class/leds/rgb:grouped_leds# ls -lR colors/ >> +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 red_max_intensity >> +--w--wx-w- 1 root root 4096 Jul 7 03:10 red_intensity >> +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 green_max_intensity >> +--w--wx-w- 1 root root 4096 Jul 7 03:10 green_intensity >> +-rw-rwxr-- 1 root root 4096 Jul 7 03:10 blue_max_intensity >> +--w--wx-w- 1 root root 4096 Jul 7 03:10 blue_intensity > Permissions are way off here. Yes Jacek made a comment in the code about octals. > >> +A user first writes the color LED brightness file with the brightness level that >> +is necessary to achieve a blueish violet output from the RGB LED group. >> + >> +echo 138 > /sys/class/leds/rgb:grouped_leds/red_intensity >> +echo 43 > /sys/class/leds/rgb:grouped_leds/green_intensity >> +echo 226 > /sys/class/leds/rgb:grouped_leds/blue_intensity > No, you can't tell what kind of color this will result in. I tested this combination on two devices (LP5523 and LP50xx) and it was a blueish violet. > Will you be on ELCE/OSS in Lyon? Unfortunately no. Dan > > Best regards, > Pavel >
Dan, On 9/26/19 1:52 PM, Dan Murphy wrote: > Jacek > > On 9/25/19 4:12 PM, Jacek Anaszewski wrote: >> Dan, >> >> On 9/25/19 7:46 PM, Dan Murphy wrote: >>> Introduce a multicolor class that groups colored LEDs >>> within a LED node. >>> >>> The multi color class groups monochrome LEDs and allows controlling two >>> aspects of the final combined color: hue and lightness. The former is >>> controlled via <color>_intensity files and the latter is controlled >>> via brightness file. >>> >>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>> --- >>> drivers/leds/Kconfig | 10 ++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/led-class-multicolor.c | 220 +++++++++++++++++++++++++++ >>> include/linux/led-class-multicolor.h | 74 +++++++++ >>> 4 files changed, 305 insertions(+) >>> create mode 100644 drivers/leds/led-class-multicolor.c >>> create mode 100644 include/linux/led-class-multicolor.h >>> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 6e7703fd03d0..cfb1ebb6517f 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 2da39e896ce8..841038cfe35b 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..25371bd9a860 >>> --- /dev/null >>> +++ b/drivers/leds/led-class-multicolor.c >>> @@ -0,0 +1,220 @@ >>> +// 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" >>> + >>> +#define INTENSITY_NAME "_intensity" >>> +#define MAX_INTENSITY_NAME "_max_intensity" >>> + >>> +void led_mc_calc_brightness(struct led_classdev_mc *mcled_cdev, >>> + enum led_brightness brightness, >>> + int brightness_val[]) >>> +{ >>> + struct led_mc_color_entry *priv; >>> + int i = 0; >>> + >>> + list_for_each_entry(priv, &mcled_cdev->color_list, list) { >> I think we should have some way to let the caller know exact mapping >> of brightness_val to color_id. Possibly LED mc core should provide >> a helper to get color_id by color entry index. > > Why would we need that? To make LED mc API more straightforward and avoid making clients wondering in what order colors will be assigned to array elements. And finally forcing them to go and analyze LED mc core. >> But this remark is actually more relevant to the place of calling. >> I'll try to propose something there. >> >> And regarding brightness_val name - how about: >> >> s/brightness_val/brightness_component/ ? > > component does not make sense to me in this context. Actually > brightness_val > > does not make sense either since it is an adjusted intensity and > brightness is passed in in the second arg > > I think intensity_values make more sense. Every variable carries some value. so this is meaningless. Intensity OTOH can be confused with <color>_intensity, which is not going to be written to the hardware, but used as a factor for calculating final LED iout brightness. The aim of the equation is to multiple the ratio of color intensity to its max intensity by global brightness. The resulting value is written to the hardware to set brightness of a LED being one of the components producing the final final combined color. I had to come up with literal description of the equation to facilitate devising better variable name :-) Having the above, how about "color_component" ? >> >> >>> + brightness_val[i] = brightness * >>> + priv->intensity / priv->max_intensity; Now, looking closer at this equation it becomes obvious to me why Pavel opted for common max_brightness: in case of <color>_intensity == <color>_max_intensity we will get the result equal to max_brightness. And this is problematic: think of a case when calculated value will be greater than given <color>_max_intensity. It will have to be limited the that constraint and will fail to produce proper color combination. As a solution I see limiting max_brightness to the lowest supported brightness from the LEDs in the cluster. >>> + i++; >>> + } >>> +} >>> +EXPORT_SYMBOL_GPL(led_mc_calc_brightness); >>> + >>> +static ssize_t intensity_store(struct device *dev, >>> + struct device_attribute *intensity_attr, >>> + const char *buf, size_t size) >>> +{ >>> + struct led_mc_color_entry *priv = container_of(intensity_attr, >>> + struct led_mc_color_entry, >>> + intensity_attr); >>> + struct led_classdev *led_cdev = priv->mcled_cdev->led_cdev; >>> + unsigned long value; >>> + ssize_t ret; >>> + >>> + mutex_lock(&led_cdev->led_access); >>> + >>> + ret = kstrtoul(buf, 10, &value); >>> + if (ret) >>> + goto unlock; >>> + >>> + if (value > priv->max_intensity) { >>> + ret = -EINVAL; >>> + goto unlock; >>> + } >>> + >>> + priv->intensity = value; >>> + ret = size; >>> + >>> +unlock: >>> + mutex_unlock(&led_cdev->led_access); >>> + return ret; >>> +} >>> + >>> +static ssize_t intensity_show(struct device *dev, >>> + struct device_attribute *intensity_attr, >>> + char *buf) >>> +{ >>> + struct led_mc_color_entry *priv = container_of(intensity_attr, >>> + struct led_mc_color_entry, >>> + intensity_attr); >>> + >>> + return sprintf(buf, "%d\n", priv->intensity); >>> +} >>> + >>> +static ssize_t max_intensity_show(struct device *dev, >>> + struct device_attribute *max_intensity_attr, >>> + char *buf) >>> +{ >>> + struct led_mc_color_entry *priv = container_of(max_intensity_attr, >>> + struct led_mc_color_entry, >>> + max_intensity_attr); >>> + >>> + return sprintf(buf, "%d\n", priv->max_intensity); >>> +} >>> + >>> +static struct attribute *led_color_attrs[] = { >>> + NULL, >>> +}; >>> + >>> +static struct attribute_group led_color_group = { >>> + .name = "colors", >>> + .attrs = led_color_attrs, >>> +}; >>> + >>> +static int led_multicolor_init_color(struct led_classdev_mc >>> *mcled_cdev, >>> + int color_id, int color_index) >>> +{ >>> + struct led_classdev *led_cdev = mcled_cdev->led_cdev; >>> + struct led_mc_color_entry *mc_priv; >>> + char *intensity_file_name; >>> + char *max_intensity_file_name; >>> + size_t len; >>> + int ret; >>> + >>> + mc_priv = devm_kzalloc(led_cdev->dev, sizeof(*mc_priv), >>> GFP_KERNEL); >>> + if (!mc_priv) >>> + return -ENOMEM; >>> + >>> + mc_priv->led_color_id = color_id; >>> + mc_priv->mcled_cdev = mcled_cdev; >>> + >>> + sysfs_attr_init(&mc_priv->intensity_attr.attr); >>> + len = strlen(led_colors[color_id]) + strlen(INTENSITY_NAME) + 1; >>> + intensity_file_name = kzalloc(len, GFP_KERNEL); >>> + if (!intensity_file_name) >>> + return -ENOMEM; >>> + >>> + snprintf(intensity_file_name, len, "%s%s", >>> + led_colors[color_id], INTENSITY_NAME); >>> + mc_priv->intensity_attr.attr.name = intensity_file_name; >>> + mc_priv->intensity_attr.attr.mode = 644; >> Proper octal value should begin with 0. >> But please use combinations of dedicated S_I* definitions >> from include/uapi/linux/stat.h. > > Using the S_I* causes checkpatch warnings > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider > using octal permissions '0444'. > #139: FILE: drivers/leds/led-class-multicolor.c:139: > + mc_priv->max_intensity_attr.attr.mode = S_IRUGO; Ah, right. Anyway, you need to add leading 0. >> >>> + mc_priv->intensity_attr.store = intensity_store; >>> + mc_priv->intensity_attr.show = intensity_show; >>> + ret = sysfs_add_file_to_group(&led_cdev->dev->kobj, >>> + &mc_priv->intensity_attr.attr, >>> + led_color_group.name); >>> + if (ret) >>> + goto intensity_err_out; >>> + >>> + sysfs_attr_init(&mc_priv->max_intensity_attr.attr); >>> + len = strlen(led_colors[color_id]) + strlen(MAX_INTENSITY_NAME) >>> + 1; >>> + max_intensity_file_name = kzalloc(len, GFP_KERNEL); >>> + if (!max_intensity_file_name) { >>> + ret = -ENOMEM; >>> + goto intensity_err_out; >>> + } >>> + >>> + snprintf(max_intensity_file_name, len, "%s%s", >>> + led_colors[color_id], MAX_INTENSITY_NAME); >>> + mc_priv->max_intensity_attr.attr.name = max_intensity_file_name; >>> + mc_priv->max_intensity_attr.attr.mode = 444; >>> + mc_priv->max_intensity_attr.show = max_intensity_show; >>> + ret = sysfs_add_file_to_group(&led_cdev->dev->kobj, >>> + &mc_priv->max_intensity_attr.attr, >>> + led_color_group.name); >>> + if (ret) >>> + goto max_intensity_err_out; >>> + >>> + mc_priv->max_intensity = LED_FULL; >>> + list_add_tail(&mc_priv->list, &mcled_cdev->color_list); >>> + >>> +max_intensity_err_out: >>> + kfree(max_intensity_file_name); >>> +intensity_err_out: >>> + kfree(intensity_file_name); >>> + return ret; >>> +} >>> + >>> +static int led_multicolor_init_color_dir(struct led_classdev_mc >>> *mcled_cdev) >>> +{ >>> + struct led_classdev *led_cdev = mcled_cdev->led_cdev; >>> + int ret; >>> + int i, color_index = 0; >>> + >>> + ret = sysfs_create_group(&led_cdev->dev->kobj, &led_color_group); >>> + if (ret) >>> + return ret; >>> + >>> + for (i = 0; i < LED_COLOR_ID_MAX; i++) { >>> + if (test_bit(i, &mcled_cdev->available_colors)) { >>> + ret = led_multicolor_init_color(mcled_cdev, i, >>> + color_index); >> color_index is now unused AFAICS. > > Ack > >> >>> + if (ret) >>> + break; >>> + >>> + color_index++; >>> + } >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +int led_classdev_multicolor_register_ext(struct device *parent, >>> + struct led_classdev_mc *mcled_cdev, >>> + struct led_init_data *init_data) >>> +{ >>> + struct led_classdev *led_cdev; >>> + int ret; >>> + >>> + if (!mcled_cdev) >>> + return -EINVAL; >>> + >>> + led_cdev = mcled_cdev->led_cdev; >>> + INIT_LIST_HEAD(&mcled_cdev->color_list); >>> + >>> + /* Register led class device */ >>> + ret = led_classdev_register_ext(parent, led_cdev, init_data); >>> + if (ret) >>> + return ret; >>> + >>> + return led_multicolor_init_color_dir(mcled_cdev); >>> +} >>> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_register_ext); >> Why devm_* versions are missing now? > > I was using the led-class-flash.c as an example and that class does not > have the devm_* versions either. Why did you have them in v8 then? > Tried to make the 2 child classes look the same. We can fix the shortcomings while adding new code. > If they are missing from the led-class-flash code then that needs to be > fixed as well. Well, yes. >> >>> + >>> +void led_classdev_multicolor_unregister(struct led_classdev_mc >>> *mcled_cdev) >>> +{ >>> + struct led_mc_color_entry *priv, *next; >>> + >>> + if (!mcled_cdev) >>> + return; >>> + >>> + list_for_each_entry_safe(priv, next, &mcled_cdev->color_list, list) >>> + list_del(&priv->list); >>> + >>> + sysfs_remove_group(&mcled_cdev->led_cdev->dev->kobj, >>> &led_color_group); >>> + led_classdev_unregister(mcled_cdev->led_cdev); >>> +} >>> +EXPORT_SYMBOL_GPL(led_classdev_multicolor_unregister); >>> + >>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); >>> +MODULE_DESCRIPTION("Multi Color LED class interface"); >>> +MODULE_LICENSE("GPL v2"); >>> diff --git a/include/linux/led-class-multicolor.h >>> b/include/linux/led-class-multicolor.h >>> new file mode 100644 >>> index 000000000000..280ba5a614b4 >>> --- /dev/null >>> +++ b/include/linux/led-class-multicolor.h >>> @@ -0,0 +1,74 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* LED Multicolor class interface >>> + * Copyright (C) 2019 Texas Instruments Incorporated - >>> http://www.ti.com/ >>> + */ >>> + >>> +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED >>> +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED >>> + >>> +#include <linux/leds.h> >>> +#include <dt-bindings/leds/common.h> >>> + >>> +struct led_classdev_mc; >>> + >>> +struct led_mc_color_entry { >>> + struct led_classdev_mc *mcled_cdev; >>> + >>> + struct device_attribute max_intensity_attr; >>> + struct device_attribute intensity_attr; >>> + >>> + enum led_brightness max_intensity; >>> + enum led_brightness intensity; >>> + >>> + struct list_head list; >>> + >>> + int led_color_id; >>> +}; >>> + >>> +struct led_classdev_mc { >>> + /* led class device */ >>> + struct led_classdev *led_cdev; >>> + struct list_head color_list; >>> + >>> + unsigned long available_colors; >>> + int num_leds; >>> +}; >>> + >>> +static inline struct led_classdev_mc *lcdev_to_mccdev( >>> + struct led_classdev *lcdev) >>> +{ >>> + return container_of(lcdev, struct led_classdev_mc, led_cdev); >>> +} >>> + >>> +/** >>> + * led_classdev_multicolor_register_ext - register a new object of >>> led_classdev >>> + * class with support for multicolor LEDs >>> + * @parent: the multicolor LED to register >>> + * @mcled_cdev: the led_classdev_mc structure for this device >>> + * @init_data: the LED class Multi color device initialization data >>> + * >>> + * Returns: 0 on success or negative error value on failure >>> + */ >>> +int led_classdev_multicolor_register_ext(struct device *parent, >>> + struct led_classdev_mc *mcled_cdev, >>> + struct led_init_data *init_data); >>> + >>> +#define led_classdev_multicolor_register(parent, mcled_cdev) \ >>> + led_classdev_multicolor_register_ext(parent, mcled_cdev, NULL) >> Please turn it into inline. > > Again same statement as above on the led-class-flash. This is how this > is defined there. > > If that is not correct in the flash class then that needs to be fixed as > well. Did you see my patch fixing these things in leds.h [0]? I just missed that problem in led-class-flash.h. >>> + >>> +/** >>> + * 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 */ >>> +void led_mc_calc_brightness(struct led_classdev_mc *mcled_cdev, >>> + enum led_brightness brightness, >>> + int brightness_val[]); >>> + >>> +#endif /* __LINUX_MULTICOLOR_LEDS_H_INCLUDED */ >>> > [0] https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/commit/?h=for-next&id=7c322056e3564da1b5bdc3f3cb79229582955eb2 -- Best regards, Jacek Anaszewski