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

[Jacek Anaszewski]

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.

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