Message-ID: <f78ee3a4-596c-1b0c-3c91-15aad85ba9b2@gmail.com>
Date:   Wed, 31 Jul 2019 22:44:28 +0200
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Dan Murphy <dmurphy@...com>, pavel@....cz
Cc:     linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 6/9] leds: multicolor: Introduce a multicolor class
 definition

Dan,

On 7/31/19 9:06 PM, Dan Murphy wrote:
> Jacek
> 
> On 7/29/19 3:50 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 7/25/19 8:28 PM, Dan Murphy wrote:
>>> Introduce a multicolor class that groups colored LEDs
>>> within a LED node.
>>>
>>> The framework allows for dynamically setting individual LEDs
>>> or setting brightness levels of LEDs and updating them virtually
>>> simultaneously.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@...com>
>>> ---
>>>   drivers/leds/Kconfig                 |  10 +
>>>   drivers/leds/Makefile                |   1 +
>>>   drivers/leds/led-class-multicolor.c  | 402 +++++++++++++++++++++++++++
>>>   include/linux/led-class-multicolor.h |  96 +++++++
>>>   4 files changed, 509 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 f7a3dd7ecf3d..7f780d5b8490 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
>> s/Class/class/
>>
>> I'll need to fix it in LED flash class entry too.
>>
>>> +      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 41fb073a39c1..897b810257dd 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..123443e6d3eb
>>> --- /dev/null
>>> +++ b/drivers/leds/led-class-multicolor.c
>>> @@ -0,0 +1,402 @@
>>> +// 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"
>>> +
>>> +struct led_classdev_mc_priv {
>>> +    struct led_classdev_mc *mcled_cdev;
>>> +
>>> +    struct device_attribute max_intensity_attr;
>>> +    struct device_attribute intensity_attr;
>>> +    struct device_attribute color_index_attr;
>>> +
>>> +    enum led_brightness max_intensity;
>>> +    enum led_brightness intensity;
>>> +    enum led_brightness new_intensity;
>>> +
>>> +    struct list_head list;
>>> +
>>> +    int color_id;
>>> +    int color_index;
>> We need to differentiate both more meaningfully.
>> Maybe led_color_id and cluster_color_id?
> 
> Thats looks better.
> 

Looking at it now I had to wonder what I had on mind.
So maybe even better shot:

int color_id // so as original
int color_seq_pos // i.e. position in the color_mix sequence

[...]

>>> +static ssize_t color_mix_store(struct device *dev,
>>> +                struct device_attribute *color_mix_attr,
>>> +                const char *buf, size_t size)
>>> +{
>>> +    struct led_classdev_mc_data *data = container_of(color_mix_attr,
>>> +                            struct led_classdev_mc_data,
>>> +                              color_mix_attr);
>>> +    struct led_classdev_mc *mcled_cdev = data->mcled_cdev;
>>> +    const struct led_multicolor_ops *ops = mcled_cdev->ops;
>>> +    struct led_classdev_mc_priv *priv;
>>> +    unsigned int value[LED_COLOR_ID_MAX];
>>> +    int adj_brightness;
>>> +    int nrchars, offset = 0;
>>> +    unsigned int i;
>>> +    int ret;
>>> +
>>> +    for (i = 0; i < mcled_cdev->num_leds; i++) {
>>> +        ret = sscanf(buf + offset, "%i%n", &value[i], &nrchars);
>>> +        if (ret != 1)
>>> +            break;
>>> +
>>> +        offset += nrchars;
>>> +    }
>>> +
>>> +    if (i != mcled_cdev->num_leds) {
>> Shouldn't we return error if i != mcled_cdev->num_leds - 1 ?
>> How can we know which color the value will be for if there is less
>> values passed than the total number of colors in the cluster?
> 
> Ok so during my testing if I had the monochrome array as <R G B>
> 
> When I wrote only <R G> and no blue I was getting random values in the
> array for the
> 
> remaining indexes and the blue LED would randomly turn on/off at
> different levels.
> 
> So if the user passes in less then expected only ids with data will be
> written and the other colors will be turned off by the for loop below.

>From what I see it will lead to wrong mapping of given color to the
value array element in the following case:

echo "<green> <blue>" > color_mix

Then green intensity will be assigned to value[0] (expects red) and blue
to value[1] (expects green). Unless I don't get something.
Your ABI documentation doesn't mention any way to redefine the color_id
returned by <color>/color_id file. And that is good.

> 
>>
>>> +        for (; i < LED_COLOR_ID_MAX; i++)
>>> +            value[i] = 0;
>> What use case is it for?
> 
> See above but this should be
> 
> for (; i < mcled_cdev->num_leds; i++)
> 
> 
>>
>>> +    }
>>> +
>>> +    list_for_each_entry(priv, &data->color_list, list) {
>>> +        if (data->cluster_brightness) {
>>> +            adj_brightness =
>>> calculate_brightness(data->cluster_brightness,
>>> +                                  value[priv->color_index],
>>> +                                 priv->max_intensity);
>>> +            ret = ops->set_color_brightness(priv->mcled_cdev,
>>> +                            priv->color_id,
>>> +                            adj_brightness);
>>> +            if (ret < 0)
>>> +                goto done;
>>> +        }
>>> +
>>> +        priv->intensity = value[priv->color_index];
>>> +    }
>> Here we could use just brightness_set op as a single call. We should
>> always write all colors as a result of write to color_mix anyway.
> 
> I guess what is gained by just passing the array down to the device
> driver and having it
> 
> parse the array and do the peripheral call?

Those array values would not be directly written to the device,
but used for calculating the actual iout intensities. Driver
will just have to call calculate_brightness() (sticking to the naming
from this patch) and write the results calculated basing on brightness
and max_brightness.

> 
[...]
> 
>>> +
>>> +    priv->new_intensity = value;
>>> +
>>> +    if (data->cluster_brightness) {
>>> +        adj_value = calculate_brightness(data->cluster_brightness,
>>> +                    priv->new_intensity,
>>> +                    priv->max_intensity);
>>> +        ret = ops->set_color_brightness(priv->mcled_cdev,
>>> +                        priv->color_id, adj_value);
>>> +        if (ret < 0) {
>>> +            priv->new_intensity = priv->intensity;
>> This is unnecessary complication. Just write the calculated iout
>> intensity.
> 
> Not sure what complication you are referring to.

The whole need for new_intensity and cluster_brightness, and then
bringing back old intensity value on set_color_brightness() failure.

> 
>> We need to highlight it in the documentation that exact requested color
>> intensity values are written to the hardware only when
>> brightness == max_brightness.
> 
> But that is not a true statement.  Thats not really how it was designed.

But it probably should be. It would simplify the design.

So my idea is like I previously described the way I had first understood
this design:

The colors set under colors directory don't reflect the iout
intensities, but are only used for calculating them, basing on the
brightness and max_intensity values.

Effectively, after changing the colors/<color>/intensity the global
(legacy monochrome) brightness value will be still valid, since iout
color will be recalculated basing on it and the new color intensity.

-- 
Best regards,
Jacek Anaszewski

Powered by blists - more mailing lists