Re: [PATCH v2 1/3] leds: core: Introduce generic pattern interface

[Jacek Anaszewski]

On 07/18/2017 01:39 AM, Bjorn Andersson wrote:
> On Mon 17 Jul 14:08 PDT 2017, Jacek Anaszewski wrote:
> 
>> On 07/16/2017 11:14 PM, Bjorn Andersson wrote:
>>> On Sun 16 Jul 11:49 PDT 2017, Jacek Anaszewski wrote:
>>>> On 07/06/2017 05:18 AM, Pavel Machek wrote:
> [..]
>>>> I've been working on addition of RGB LED support to the LED core for
>>>> some time now, in the way as we agreed upon at [0], but it turns out to
>>>> be less trivial if we want to do it in an elegant way.
>>>>
>>>
>>> Generally 3 predefined LPG blocks are routed to the TRILED current sink.
>>>
>>> Exposing the TRILED block as a RGB LED instance would make sense in its
>>> own, but it doesn't give us a coherent solution for the LPG.
>>>
>>> The current board I'm working on (DragonBoard820c) has 4 LEDs, 3 of them
>>> are connected to the TRILED and the fourth is on a "GPIO" in current
>>> sink mode.
>>>
>>> By having each LPG represented as a LED device gives us a unified view
>>> of the hardware even though there are two different types of current
>>> sinks.
>>>
>>>
>>> Further more, per the 96boards specification we're expected to have
>>> different triggers on the different "colors" of the TRILED.
>>
>> What is the function of TRILED block then? My first impression was
>> that it allows for setting brightness on all three LED synchronously?
>>
> 
> It's nothing more than one hardware block providing 3 current sinks.

What's the benefit of grouping three sinks? Is there some other gain
besides possibility of turning on/off the whole block at once?


>>>
>>> So I do not agree with imposing this kind of decisions on the board
>>> design just to support this higher level feature.
>>>
>>>> Less elegant way would be duplicating led-core functions and changing
>>>> single enum led_brightness argument with the three ones (or a struct
>>>> containing three brightness components)
>>>>
>>>> I chose to go the elegant way and tried to introduce led_classdev_base
>>>> type that would be customizable with the set of ops, that would allow
>>>> for making the number of brightness components to be set at once
>>>> customizable. This of course entails significant amount of changes in
>>>> the LED core and some changes in LED Trigger core.
>>>>
>>>
>>> I think that the RGB interface has to be a "frontend" of any
>>> configurable LED instances and not tied to a particular hardware
>>> controller.
>>
>> That doesn't assure brightness setting synchronization which is
>> especially vital in case of triggers like timer.
>>
> 
> If you look at any available Android based device you can see what
> happens when you set red, then green and then blue brightness
> independently - from user space even. The LED might be
> red/green/blue/purple whatever, but I would argue that it's not
> noticeable due to the short duration between the updates.

> The case where synchronization matters is if you have pattern
> transitions as you're extending the time of the transition and you can
> spot the color variation in the transitions.

It would require more thorough analysis across various devices
programmed via different buses. Probably in most cases applying
the userspace frontend mentioned by you would work just fine.

And when it comes to higher frequencies, we have had already
attempts of adding hr timer support for "fast LEDs" like the
ones driven through GPIOs, and those could benefit from the
kernel level synchronization.

-- 
Best regards,
Jacek Anaszewski