Re: [PATCH v18 4/4] leds: multicolor: Introduce a multicolor class definition

[Jacek Anaszewski]

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 <dmurphy@xxxxxx>
>>>>> +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