Re: [PATCH v11 04/16] leds: multicolor: Introduce a multicolor class definition

[Jacek Anaszewski]

Dan,

On 10/10/19 2:27 AM, Dan Murphy wrote:
> Jacek
> 
> On 10/9/19 4:47 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 10/8/19 10:47 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@xxxxxx>
>>> ---
>>> Â .../ABI/testing/sysfs-class-led-multicolorÂÂÂ |Â 35 +++
>>> Â Documentation/leds/index.rstÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
>>>  Documentation/leds/leds-class-multicolor.rst | 96 +++++++
>>> Â drivers/leds/KconfigÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 10 +
>>> Â drivers/leds/MakefileÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 1 +
>>> Â drivers/leds/led-class-multicolor.cÂÂÂÂÂÂÂÂÂÂ | 271 ++++++++++++++++++
>>> Â include/linux/led-class-multicolor.hÂÂÂÂÂÂÂÂÂ | 143 +++++++++
>>> Â 7 files changed, 557 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..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@xxxxxx>
>>> +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
>> This equation alone incites questions on how it is supposed to work.
>>
>> It would be better to present the whole algorithm for calculating
>> combined color here.
> 
> I am not sure I follow. Isn't that explained in the class document?

I just think that this document should contain all necessary info
for the reader to understand the effect of writing brightness.
Looking once more it seems that slight modification of the last
sentence above equation could suffice:

s/is calculated/is calculated for each grouped LED/

>>> +
>>> +ÂÂÂÂÂÂÂ 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@xxxxxx>
>>> +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@xxxxxx>
>>> +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..7a695a29377e
>>> --- /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-r--r-- 1 root root 4096 Jul 7 03:10 blue_intensity
>>> +-r--r--r-- 1 root root 4096 Jul 7 03:10
>>> blue_max_intensity
>>> +-rw-r--r-- 1 root root 4096 Jul 7 03:10
>>> green_intensity
>>> +-r--r--r-- 1 root root 4096 Jul 7 03:10
>>> green_max_intensity
>>> +-rw-r--r-- 1 root root 4096 Jul 7 03:10 red_intensity
>>> +-r--r--r-- 1 root root 4096 Jul 7 03:10
>>> red_max_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
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 4b68520ac251..a1ede89afc9e 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..89f4bc9e057c
>>> --- /dev/null
>>> +++ b/drivers/leds/led-class-multicolor.c
>>> @@ -0,0 +1,271 @@
>>> +// 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"
>>> +
>>> +int led_mc_calc_brightness(struct led_classdev_mc *mcled_cdev,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum led_brightness brightness,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct led_mc_color_conversion color_component[])
>> Now the function name doesn't match with the output array name.
>>
>> How about:
>>
>> - led_mc_brightness_to_color_components
>> - led_mc_calc_color_components
>>
>> Other suggestions?
> 
> led_mc_calc_color_components is fine the first one is to long

Agreed.

[...]
>>> +static inline int devm_led_classdev_multicolor_register(struct
>>> device *parent,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct led_classdev_mc *mcled_cdev)
>>> +{
>>> +ÂÂÂ return -EINVAL;
>>> +}
>> Do you have use case for which these no-ops would be useful?
>> We don't have no-ops for any of current LED API beside triggers,
>> which are indeed useful.
> 
> Absolutely this was the solution for the kbuild test. This MC framework
> is optional for drivers where as the LED class and flash are not.
> 
> The LP55xx does not have a dependency on the CONFIG_MULTICOLOR_CLASS
> flag and it does not need a hard dependency. Without that flag defined
> in the defconfig

Ack.

-- 
Best regards,
Jacek Anaszewski