Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver

[Vesa JÃÃskelÃinen]

Hi Jacek,

On 07/01/2019 23.13, Jacek Anaszewski wrote:
> Hi Vesa,
>
> On 1/5/19 1:39 AM, Vesa JÃÃskelÃinen wrote:
> > Hi Jacek,
> >
> > On 04/01/2019 23.37, Jacek Anaszewski wrote:
> > > But, aside from that hypothetic issue, we need a solution for
> > > LEDn_BRIGHTNESS feature of lp5024, i.e. setting color intensity
> > > via a single register write. How would you propose to address that?
> >
> > You could model it to something like this in device tree:
> >
> > led-module @ <i2c-address> {
> > ÂÂÂÂÂcompatible = "lp5024";
> >
> > ÂÂÂÂÂ// There is in hardware setup to use either linear or
> > ÂÂÂÂÂ// logarithmic scaling:
> > ÂÂÂÂÂ//enable-logarithmic-brightness;
> >
> > ÂÂÂÂÂled0 {
> > ÂÂÂÂÂÂÂÂ // this will create led instance for LED0 in lp5024
> > ÂÂÂÂÂÂÂÂ label = "lp-led0";
> >
> > ÂÂÂÂÂÂÂÂ // This specifies LED number within lp5024
> > ÂÂÂÂÂÂÂÂ led-index = <0>;ÂÂ // set output-base as 0*3 == 0
> >
> > ÂÂÂÂÂÂÂÂ element-red {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to OUT0
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <0>;
> > ÂÂÂÂÂÂÂÂ };
> >
> > ÂÂÂÂÂÂÂÂ element-green {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to OUT1
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <1>;
> > ÂÂÂÂÂÂÂÂ };
> >
> > ÂÂÂÂÂÂÂÂ element-blue {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to OUT2
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <2>;
> > ÂÂÂÂÂÂÂÂ };
> >
> > ÂÂÂÂÂ};
> >
> > ÂÂÂÂÂled1 {
> > ÂÂÂÂÂÂÂÂ // this will create led instance for LED1 in lp5024
> > ÂÂÂÂÂÂÂÂ label = "lp-led1";
> >
> > ÂÂÂÂÂÂÂÂ // This specifies LED number within lp5024
> > ÂÂÂÂÂÂÂÂ led-index = <1>;ÂÂ // set output-base as 1*3 == 3
> >
> > ÂÂÂÂÂÂÂÂ element-red {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to OUT3
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <0>;
> > ÂÂÂÂÂÂÂÂ };
> >
> > ÂÂÂÂÂÂÂÂ element-green {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to OUT4
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <1>;
> > ÂÂÂÂÂÂÂÂ };
> >
> > ÂÂÂÂÂÂÂÂ element-blue {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to OUT5
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <2>;
> > ÂÂÂÂÂÂÂÂ };
> >
> > ÂÂÂÂÂ};
> >
> > ÂÂÂÂÂbank-led {
> > ÂÂÂÂÂÂÂÂ // this will create led instance for bank leds in lp5024
> > ÂÂÂÂÂÂÂÂ label = "lp-bank-led";
> >
> > ÂÂÂÂÂÂÂÂ // configured bank led configuration
> > ÂÂÂÂÂÂÂÂ led-index = <2 3 4 5 6 7>;
> > ÂÂÂÂÂÂÂÂ // As here is list of led-indices this entry is
> > ÂÂÂÂÂÂÂÂ // assumed to be bank configuration. Bank mode is enable
> > ÂÂÂÂÂÂÂÂ // for the indices.
> >
> > ÂÂÂÂÂÂÂÂ // set output-base as BANK A
> >
> > ÂÂÂÂÂÂÂÂ element-red {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to BANK A
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <0>;
> > ÂÂÂÂÂÂÂÂ };
> >
> > ÂÂÂÂÂÂÂÂ element-green {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to BANK B
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <1>;
> > ÂÂÂÂÂÂÂÂ };
> >
> > ÂÂÂÂÂÂÂÂ element-blue {
> > ÂÂÂÂÂÂÂÂÂÂÂÂ // refers to BANK C
> > ÂÂÂÂÂÂÂÂÂÂÂÂ output-offset = <2>;
> > ÂÂÂÂÂÂÂÂ };
> > ÂÂÂÂÂ};
> > };
> >
> > This would then create three led instances and each led instance has 
> > brightness setting and that goes straight to hardware.
> >
> > If one would want to override hardware control for brightness then I 
> > suppose you would define in led node something like:
> >
> > ÂÂÂÂÂbrightness-model = "hsl"
> >
> > This would then pick red, green and blue elements for hsl calculations 
> > and others color elements for linear. LED specific hardware brightness 
> > would then be either 0 or 0xFF depending if all of LED color elements 
> > are zero or not.
> >
> > Would that kind of model work?
>
> I'd prefer to have single RGB LED device. And your DT design
> is unnecessarily complex and a bit confusing.

As this chip series is kinda designed for N x RGB LED's my idea was that 
if from user space point of view we model it as N times of individual 
RGB LED instances that may not even have anything to do with together. 
Eg. could be used for different purposes and such.

And in device tree one would define logical connections for the leds so 
they would be mapped logically correct to user space.

If one would define it like:

led1 {
	// this will create led instance for LED1 in lp5024
	label = "lp-led1";
	
	// This specifies LED number within lp5024
	led-sources = <1>;
};
(note changed led-index to led-sources as that is what Pavel had and 
preferred)

We could assume that it is RGB led in this driver's case and create it 
automatically with elements "red", "green", and "blue". And this could 
then be mapped automatically to HSL color elements or what ever the 
model would be.

If you would model it differently in your hardware design then you would 
need to define more device tree nodes. Eg. if your order of LEDs would 
not be red, green, blue. Or if you would have non-RGB led(s) in there.

> Also, you provided scarce information about sysfs interface.
> It would be nice to see the sequence of commands.

In this case it could be:

# Note: Updated color to value array model.

$ ls /sys/class/leds
lp-led0	lp-led1	lp-bank-led

$ ls /sys/class/leds/lp-led0
brightness	color

$ ls /sys/class/leds/lp-led1
brightness	color

$ ls /sys/class/leds/lp-bank-led
brightness	color

# Idea of above is that as brightness is for triplet:
#   OUT(LED*3 + 0), OUT(LED*3 + 1), OUT(LED*3 + 2),
# Then if we model it like RGB LED then brightness would automatically
# map to correct OUTputs and be grouped from user space point of view
# logically in correct place.

# set first led to red
echo "255 0 0" > /sys/class/leds/lp-led0/color

# set second led to green
echo "0 255 0" > /sys/class/leds/lp-led1/color

# set bank led to blue
echo "0 0 255" > /sys/class/leds/lp-bank-led/color

# Set hardware brightness control to middle
echo "128" > /sys/class/leds/lp-bank-led/brightness

# If we would have software controlled virtual brightness enabled for
# particular led classdev then there would be some math in either user
# or in kernel space.

Thanks,
Vesa JÃÃskelÃinen