Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Pavel,

On 04/15/2016 01:53 PM, Pavel Machek wrote:
> Hi!
>
>>>> How about implementing patterns as a specific typer of triggers?
>>>> Let's say we have ledtrig-rgb-pattern:
>>>
>>> Well, we'd need ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, ... , as we
>>> can have more than one rgb led. But yes.
>>
>> Triggers can have many listeners, i.e. led_trigger_event() sets
>> brightness on all LED class devices registered on given trigger.
>> We could have led_trigger_rgb_event() that would set brightness
>> on all groups-of-three LEDs registered on given rgb-trigger.
>
> I do not understand that.

Triggers are defined as kernel source of led events.

Currently we have two types of triggers as far as the source of
led event is concerned:
- triggers that are created per LED class device and therefore each
   LED class device has their own source of kernel event,
   initialized on trigger activation (e.g. ledtrig-timer,
   ledtrig-heartbeat, ledtrig-oneshot),
- triggers that propagate kernel events from one source to many
   listeners (e.g.ledtrig-ide-disk, ledtrig-cpu) - they internally
   use led_trigger_event(), which iterates through all LED class devices
   registered on a trigger and applies the same brightness.

In case of RGB trigger we'd like to have a common source of kernel
events for three LED class devices. After deeper analysis I'd modify
the approach a bit, in order to add a capability of generating kernel
led events from user space.

Let's say that we have LED RGB driver that exposes three LED class
devices: lp5523:red, lp5523:green, lp5523 blue.

$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::red/trigger
$echo "red" > /sys/class/leds/lp5523::red/rgb_color
$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::green/trigger
$echo "green" > /sys/class/leds/lp5523::green/rgb_color
$echo "rgb-pattern-lp5523" > /sys/class/leds/lp5523::blue/trigger
$echo "blue" > /sys/class/leds/lp5523::blue/rgb_color

led-rgb-pattern trigger would create a new trigger each time a non
existing rgp-pattern-* suffix is passed. In order to make it possible
for the user space to generate trigger events, a dedicated sysfs
interface would have to be exposed. How about creating a new LED RGB
class device that would expose "color" sysfs attribute with three space
separated R G B values? The device would appear in the sysfs after
rgb-pattern trigger is created.

Internally the LED RGB class device would use a new
led_trigger_rgb_event() to set the color:


void led_trigger_rgb_event(struct led_trigger *trig,
             enum led_brightness red,
             enum led_brightness green,
             enum led_brightness blue,
{
         struct led_classdev *led_cdev;

         if (!trig)
             return;

         read_lock(&trig->leddev_list_lock);
         list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
                if (led_cdev>flags & LED_RGB_COLOR_R)
                    led_set_brightness(led_cdev, red);
                else if (led_cdev>flags & LED_RGB_COLOR_G)
                        led_set_brightness(led_cdev, green);
                else if (led_cdev>flags & LED_RGB_COLOR_B)
                        led_set_brightness(led_cdev, blue);
         read_unlock(&trig->leddev_list_lock);
}



>
>> I agree that ledtrig-rgb-pattern-1, ledtrig-rgb-pattern-2, etc. would
>> be also needed to add a capability of setting different colors on
>> different LED devices.
>
> Ok.
>
>>> For patterns, I'd suggest array of (r g b time) values.
>>>
>>> Pattern engines can do stuff like "slowly turn LED from off to red, then switch color to
>>> white, then slowly turn it to yellow, then turn it off at once" with defined speeds
>>> for "slowly" and option of either linear on non-linear brightness ramping.
>>>
>>> The last option might be a bit too much, but I believe we should support the rest.
>>
>> Yes, that's an interesting idea. It also turns out that trigger based
>> patterns could be also used for defining generic patterns for a group
>> of monochrome LEDs.
>
> Yes, controlling monochrome LEDs synchronously is another task for
> patterns. Actually, N900 uses that to control 6 keyboard backlight
> LEDs synchronously... and yes, it would be somehow nice to preserve
> this functionality.


-- 
Best regards,
Jacek Anaszewski