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

[Jacek Anaszewski]

Hi Heiner,

Thanks for the feedback.

On 04/05/2016 10:43 PM, Heiner Kallweit wrote:
> Am 05.04.2016 um 21:45 schrieb Jacek Anaszewski:
> > On 04/05/2016 11:01 AM, Pavel Machek wrote:
> > > Hi!
> > >
> > > > > > > > It would have the same downsides as in case of having r, g and b in
> > > > > > > > separate attributes, i.e. - problems with setting LED colour in
> > > > > > > > a consistent way. This way LED blinking in whatever colour couldn't
> > > > > > > > be supported reliably. It was one of your primary rationale standing
> > > > > > > > behind this design, if I remember correctly. Second - what about
> > > > > > > > triggers? We've had a long discussion about it and this design turned
> > > > > > > > out to be most fitting.
> > > > > > >
> > > > > > > Are on/off triggers really that useful for a LED that can produce 16
> > > > > > > million colors?
> > > > > > >
> > > > > > > I believe we should support patterns for RGB LEDs. Something like
> > > > > > > [ (time, r, g, b), ... ] . Ok, what about this one?
> > > > > > >
> > > > > > > Lets say we have
> > > > > > >
> > > > > > > /sys/class/pattern/lp5533::0
> > > > > > > /sys/class/pattern/software::0
> > > > > > >
> > > > > > > /sys/class/led/n900::red ; default trigger "lp5533::0:0"
> > > > > > > /sys/class/led/n900::green ; default trigger "lp5533::0:1"
> > > > > > > /sys/class/led/n900::blue ; default trigger "lp5533::0:2"
> > > > > > >
> > > > > > > Normally, pattern would correspond to one RGB LED. We could have
> > > > > > > attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
> > > > > > > this pattern.
> > > >
> > > > Could you give an example on how to set a color for RGB LED using
> > > > this interface? Would it be compatible with LED triggers?
> > > > Where the "pattern" class would be implemented?
> > >
> > > Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
> > > set the color for the led. 'echo "trigger-name" > trigger' would set
> > > the trigger, probably just toggling between LED off and set color for
> > > the old triggers.
> > >
> > > Where to implement the patterns is different question, but for example
> > > drivers/leds/pattern?
> >
> > I'd rather leave the pattern issue for now, since it seems to be
> > different from the problem Heiner was trying to solve with his LED RGB
> > extension. Moreover, hardware patterns are device specific and it could
> > be hard to propose a generic interface.
> > Drivers can always expose their custom sysfs attributes for configuring
> > the patterns.
> >
> > Regardless of the above, some of your considerations brought me an idea
> > on how to add generic and backwards compatible support for setting RGB
> > color at one go.
> >
> > Currently LED class drivers of RGB LED controllers expose three LED
> > class devices - one per R, G and B color component. I propose that
> > such drivers set LED_DEV_CAP_RGB flag for each LED class device they
> > register. LED core, seeing the flag, would create a generic "color"
> > sysfs attribute for each of the three LED class devices.
> >
> > The "color" attribute would contain "R G B" values. Setting the "color"
> > attribute of any of the three LED class devices would affect brightness
> > properties (i.e. constituent colors) of the remaining two ones.
> > It would result in disabling any active triggers and writing all the
> > three color settings to the RGB LED controller at one go.
> >
> > We would probably need additional op in the LED core : color_set.
> >
> > Having the color set to nonzero value would signify the the three LED
> > class devices are in sync and that setting a trigger on any of them
> > applies to the remaining two ones. It would have to be considered
> > whether existing triggers could be made compatible with synchronized
> > RGB LED class devices.
> >
> > I'm curious what do you think about the idea.
> >
> > Pavel, Heiner, others?
>
> Exposing "coupled LED devices" as separate LED devices most likely is ok
> when accessed from user space as the name of the led_classdev's indicates
> that they belong together.
> But how about a trigger wanting to set a RGB LED to a specific color?

RGB triggers would use a new color_set op. It means that currently
implemented triggers would be unable to set arbitrary color, but
they could be used only in a monochrome context.

> (That's not available yet but one possible use case for RGB LED's)
> A trigger is bound to a led_classdev currently. In addition we'd need
> to introduce some kind of super_led_classdev having links to the respective
> R/G/B led_classdev's (+ trigger functions dealing with this super_led_classdev).
>
> These changes / extensions are not needed if a RGB LED is exposed as one
> led_classdev, just with flag LED_DEV_CAP_RGB set.
> OK, we'd still have to change the sysfs interface as obviously setting
> hue/sat/brightness via one "brightness" attribute is not acceptable.
> However this constraint might not affect the kernel-internal trigger API
> (usage of parameter brightness in led_trigger_event).

We would still have to abuse brightness parameter semantics.

> I see Pavel's point that there might be different types of multi-color LED's.
> At least we have:
> - multi-color LED's where each single LED is visible even if all are switched on
> - multi-color LED's like RGB LED's where you usually just see a uniform color

I think that if we are talking about RGB LEDs it is always the second
case.

> Last but not least regarding the patterns:
> Something like proposed by Pavel is e.g. (partially) supported by the blink(1)
> firmware. That would be an example of such a "hardware-accelerated" pattern.
>
> As I see it the current blinking support then would be one special case of a pattern.
> As a consequence once having pattern support we might be able to switch users of blinking
> to pattern and remove the blinking support.

Let's split patterns related discussion into a separate thread.
It would be best if it began with a patch.

--
Best regards,
Jacek Anaszewski