Re: [PATCH 04/24] dt-bindings: leds: Add function and color properties

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi Vesa,

On 11/13/2018 08:10 AM, Vesa Jääskeläinen wrote:
> Hi Jacek,
> 
> On 12/11/2018 18.02, Jacek Anaszewski wrote:
>>>> Support for RGB LEDs, or other variations of LED synchronization
>>>> have been approached several times, but without satisfying result
>>>> so far.
>>>>
>>>> Generally the problem boils down to the issue of how triggers
>>>> should handle multiple synchronized LED class devices in a backwards
>>>> compatible way with monochrome LEDs.
>>>>
>>>> At some point the HSV [0] approach was proposed, but there was a
>>>> problem
>>>> with getting uniform colors across devices. Especially white.
>>>> Certainly a calibration mechanism would be required.
>>>>
>>>> [0] https://lkml.org/lkml/2017/8/30/423
>>>
>>> We do not usually use PWM controlled leds so our calibration has been HW
>>> engineer tuning the resistors for the leds to get acceptable color for
>>> different colors variations.
>>>
>>> If one wants to have fixed colors for such device then one could use
>>> this similar method to define them or/and free form interface to enter
>>> RGB values there. Thou with PWM RGB leds you probably want to have more
>>> animation there which probably would require some additional support
>>> code.
>>>
>>> One way to do atomic PWM RGB color change with sysfs could be:
>>>
>>> echo "21 223 242" > color
>>>
>>> or
>>>
>>> echo "21 223 242" > rgb
>>>
>>> or:
>>>
>>> echo "r:21 g:232 b:242" > color (or something similar)
>>>
>>> and if there is know registered name then just write it to "color" which
>>> would pick registered color rgb values to led instances rgb value.
>>>
>>> Now for PWM RGB led one could use "brightness" and "rgb" value to
>>> calculate actual color with some color space formula (like hsv in [0]).
>>>
>>> Doing white with RGB LED is a bit hard so usually you want to get RGBW
>>> LED (or RGBAW LED) if "real" white is something that is needed. This
>>> could then be "rgbw" entry and "color" to pick from fixed set.
>>>
>>> These presets in device tree for "color" could be considered one way of
>>> doing calibration for particular hardware.
>>>
>>> So in device tree for RGB PWM led it could be like:
>>>
>>> color-orange {
>>>      rgb = <249 197 9>
>>> }
>>>
>>> color-warm-white {
>>>      rgb = <255 253 249>
>>> }
>>>
>>> How would that sound like?
>>
>> Thank you for the description of your approach.
>>
>> Predefined DT color definitions make an interesting option. Nonetheless,
>> your design assumes that for RGB LEDs max_brightness would have to be
>> always 1.
>>
>> While it would solve the issue, it wouldn't allow to benefit from
>> the whole potential of RGB LEDs - think of having adjustable "color
>> brightness" (like in HSV color model).
> 
> What I tried to describe above was that these could also work with HSV
> model also with larger brigtness values too.
> 
> Let's say we do following sequence to change from off state to user
> configured intensity value:
> 
> # Turn off leds
> echo 0 > brightness
> 
> # Select requested color
> echo "orange" > color
> 
> # Use configured LED intensity from user configuration
> echo 84 > brightness
> 
> Driver would now use rgb value <249 197 9> and brightness <84> to
> calculate (with HSV solution) real PWM values for the led component
> colors. With led controller this is probably easier to get linear
> feedback but for generic PWM controller you may want to utilize PWM
> curve feature like defined for backlight driver.
> 
> When thinking this I instantly see my self with thinking PS4's
> sliding/pulsing color animation for orange/blue/white. And controlling
> this animation steps manually from user space probably is not the best
> idea so like with blinking you would need more accurately timed action
> within kernel I suppose (or hardware with the support).
> 
> With that idea forward:
> 
> # Turning animation sequencer off causes instant changes for values
> echo "off" > animation-sequencer
> 
> # User 1 second for animation (no effect yet as sequencer is off)
> echo 1000 > animation-time-ms (or so)
> 
> # Turn off leds (instance change)
> echo 0 > brightness
> 
> # Select requested color (instant change)
> echo "orange" > color
> 
> # Change to linear animation sequencer
> echo "linear" > animation-sequencer
> 
> # Now everything from this point is subject to animation sequencer
> # Sequencer remember active state and next state (as seen in sysfs)
> 
> # Use configured LED intensity from user configuration (no effect yet)
> echo 84 > brightness
> 
> # Trigger transition sequence
> echo load > animation-sequencer
> 
> # This would cause 1 second animation from black to orange with user
> intensivity of 84
> 
> # Wait for transition to complete and some extra
> sleep 4
> 
> # Change color to blue with animation
> echo "blue" > color
> echo load > animation-sequencer
> 
> # This would cause driver to change from orange to blue in current
> intensivity level during 1 second period
> 
> # -- end --
> 
> Now that went a bit ahead of time but I believe it demonstrates the
> potential for this approach with combined with HSL functionality for
> future.

We have just contributed pattern trigger - it could be adjusted
to RGB LED purposes too.

See drivers/leds/trigger/ledtrig-pattern.c and
Documentation/ABI/testing/sysfs-class-led-trigger-pattern in
Linus' tree.

>> How abut allowing for providing an array of color intensity/brightness
>> levels per each color? In the simplest case there could be a single
>> level per color, but it should be possible to provide up to 255 levels.
> 
> I believe that brightness and color should be separate topics.

I didn't make myself clear enough - I like the approach with the
color sysfs file. By the "color brightness" I mean color arrays,
that would have to be defined per each color. Elements of such an
array would make the "color brightness/lightness" range.

> We have in our devices option in user interface to configure intesivity
> for backlight and in one device also intensivity for status led (this is
> the only PWM led we have :)). (I believe this might be currently
> implemented without actual led driver and directly use PWM interface)
> 
> Even in when reduced intensivity we want the color to be set to
> specified value even thou it would not be as bright as full intensivity
> would be.
> 
> What do you think?

I think that we've just agreed the preliminary requirements for
a new RGB LED class interface.

-- 
Best regards,
Jacek Anaszewski