Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Vesa, On 1/9/19 8:11 AM, Vesa Jääskeläinen wrote: Hi Pavel, On 09/01/2019 0.59, Pavel Machek wrote: Hi! Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. So they have feature where they have independent controls for each channel, then one common control per three channels. Other chips have common control for all the LEDs, for example. We don't support that currently; lets focus on the RGB thing first. I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. It is true that _chip_ manufacturer can not know what LEDs will be connected. But _system_ manufacturer can and should know that, and should tell be able to tell us in the dts. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend on the LED circuit design. Depending on LED circuit design and actual LEDs connected is okay.. we just need to get information from _system_ designer (not chip designer), and pass it to a place where color is computed. a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential relation. Human eyes percieve logarithm of lumens. RGB color model uses even more complex function. One general question: do you have any solutions in store? I played with LEDs on N900 over the weekend, yes. And getting reasonable colors seems to be possible, when a) and b) are solved... a) seems to be more important than b). Now... this does not tell us how we should design kernel<->user interface, but it should tell us that main goals - 1) and 2) are possible. I was thinking about this calibration and color correctness thing and I am thinking a bit that it should be partly in kernel and partly in user space. For displays and printers there are defined icc-profiles that define how colors are mapped to particular device in cases when you want to have accurate color representation. In theory to get accurate LED output one could model LEDs with icc profile and then pick your color and convert it with icc profile to actual raw hardware values. Then this raw hardware value could be written from user space to kernel. icc-profiles idea sounds interesting. We would have to adjust hsv->rgb algorithm to take the profiles into account. I wonder how that would work in practice. In kernel we could provide raw hardware value support and in case of PWM IC controlled LEDs we could provide curve mapping to linearize the behavior of values entered to enable use in cases where close enough is good enough. Eg. if you want to have "white" then you have your color space picker like sRGB(255,255,255). Then you would define icc profile it might translate to hardware raw values 253%, 230%, 225%. Then you would write this to kernel with: # set red, green and blue color elements echo "253 230 225" > color In this case however behavior of brightness node is challening in accuracy domain. Britghtness value 255 would of course provide 1:1 mapping in this case. To go right to correct color and brightness at one go we could have optional brightness at the end of color value array: # set red, green and blue color element and brightness
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Vesa,
On 1/9/19 7:46 AM, Vesa Jääskeläinen wrote:
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 @ {
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.
Actually the only differences between your DT design and that
initially proposed by Dan are element-* nodes describing
IOUT -> color mapping.
I am not sure if LEDn_BRIGHTNESS feature would work as intended
when colors are not assigned to IOUTs in the order shown on the
Figure 14. in the data sheet. I'm afraid that we would be changing
hue as well (sticking to the HSV nomenclature). Let's not bother with
covering arrangements not being in line with the data sheet
recommendations.
Of course the element-color nodes will be necessary for leds-pwm,
like in your github example.
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
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 1/10/19 3:02 PM, Jacek Anaszewski wrote: > On 1/10/19 8:58 PM, Dan Murphy wrote: >> On 1/10/19 1:23 PM, Jacek Anaszewski wrote: >>> Dan, >>> >>> On 1/10/19 1:46 PM, Dan Murphy wrote: Jacek On 1/8/19 3:25 PM, Dan Murphy wrote: > Jacek > > On 1/8/19 3:18 PM, Jacek Anaszewski wrote: >> Hi Dan, >> >> On 1/7/19 10:14 PM, Dan Murphy wrote: >>> Jacek >>> >>> On 1/7/19 2:59 PM, Jacek Anaszewski wrote: Dan, On 1/7/19 8:36 PM, Dan Murphy wrote: > Jacek > > On 1/7/19 1:13 PM, Jacek Anaszewski wrote: >> On 1/6/19 4:52 PM, Jacek Anaszewski wrote: >>> Hi Pavel, >>> >>> On 1/5/19 11:12 PM, Pavel Machek wrote: Hi! >> Grab yourself an RGB LED and play with it; you'll see what the >> problems are. It is hard to explain colors over email... > > Video [0] gives some overview of lp5024 capabilities. > > I don't see any problems in exposing separate red,green,blue > files and brightness for the devices with hardware support for > that. Well, that's what we do today, as three separate LEDs, right? >>> >>> No. It doesn't allow for setting color intensity by having >>> the color fixed beforehand. Below is relevant excerpt from >>> the lp5024 documentation. This is not something that can be >>> mapped to RGB color space, but rather to HSV/HSL, with the >>> reservation that the hardware implementation uses PWM >>> for setting color intensity. >>> >>> >>> 8.3.1.2 Independent Intensity Control Per RGB LED Module >>> >>> When color is fixed, the independent intensity-control is used to >>> achieve accurate and flexible dimming control for every RGB LED >>> module. >>> >>> 8.3.1.2.1 Intensity-Control Register Configuration >>> >>> Every three consecutive output channels are assigned to their >>> respective >>> intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, >>> OUT1, >>> and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to >>> connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx >>> device allows 256-step intensity control for each RGB LED module, >>> which >>> helps achieve a smooth dimming effect. >>> >>> I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display >>> >>> The difference is that monitor display driver is pre-calibrated >>> for given display by the manufacturer. With the LED controllers the >>> manufacturer has no control over what LEDs will be connected to the >>> iouts. Therefore it should be not surprising that colors produced >>> by custom LEDs are not as user would expect when comparing to >>> the RGB color displayed on the monitor display. >>> >>> TI provides "Various LED Ring Lighting Patterns Reference Design" >>> [0] >>> that show how to produce various patterns with use of the reference >>> board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs >>> [1]. >>> >>> Document [0] mentions also specific "Design considerations" in the >>> chapter 2.2: >>> >>> >>> Several considerations are taken into account for this particular >>> design: >>> • LED map (ring) for meeting the requirement of popular >>> human-machine interaction style. >>> • LED size, numbers and the diffuse design for meeting lighting >>> pattern uniformity. >>> • Analog dimming in the difference ambient light lux without losing >>> dimming resolution in lighting pattern. >>> >>> These considerations apply to most human-machine interaction end >>> equipment with day and night vision >>> designs in some way, but the designer must decide the particular >>> considerations to take into account for a >>> specific design. >>> >>>
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 1/10/19 8:58 PM, Dan Murphy wrote: On 1/10/19 1:23 PM, Jacek Anaszewski wrote: Dan, On 1/10/19 1:46 PM, Dan Murphy wrote: Jacek On 1/8/19 3:25 PM, Dan Murphy wrote: Jacek On 1/8/19 3:18 PM, Jacek Anaszewski wrote: Hi Dan, On 1/7/19 10:14 PM, Dan Murphy wrote: Jacek On 1/7/19 2:59 PM, Jacek Anaszewski wrote: Dan, On 1/7/19 8:36 PM, Dan Murphy wrote: Jacek On 1/7/19 1:13 PM, Jacek Anaszewski wrote: On 1/6/19 4:52 PM, Jacek Anaszewski wrote: Hi Pavel, On 1/5/19 11:12 PM, Pavel Machek wrote: Hi! Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. 8.3.1.2 Independent Intensity Control Per RGB LED Module When color is fixed, the independent intensity-control is used to achieve accurate and flexible dimming control for every RGB LED module. 8.3.1.2.1 Intensity-Control Register Configuration Every three consecutive output channels are assigned to their respective intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx device allows 256-step intensity control for each RGB LED module, which helps achieve a smooth dimming effect. I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. TI provides "Various LED Ring Lighting Patterns Reference Design" [0] that show how to produce various patterns with use of the reference board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. Document [0] mentions also specific "Design considerations" in the chapter 2.2: Several considerations are taken into account for this particular design: • LED map (ring) for meeting the requirement of popular human-machine interaction style. • LED size, numbers and the diffuse design for meeting lighting pattern uniformity. • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern. These considerations apply to most human-machine interaction end equipment with day and night vision designs in some way, but the designer must decide the particular considerations to take into account for a specific design. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend Typo here: s/any fixed/and fixed/ on the LED circuit design. 2a) LEDs probably slightly change color as they age. That's out of scope, unless the variation is much greater than on monitors. 2b) Manufacturing differences cause small color variation. Again, that's out of scope, unless the variation is much greater than on monitors. Nice to have features: 3) Full range of available colors/intensities should be available to userspace 4) Interface should work well with existing triggers 5) It would be nice if userland knew how many lumens are produced at each wavelength for each setting (again, minus aging and manufacturing variations). 6) Complexity of math in kernel should be low, and preferably it should be integer or fixed point Problems: a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 1/10/19 1:23 PM, Jacek Anaszewski wrote: > Dan, > > On 1/10/19 1:46 PM, Dan Murphy wrote: >> Jacek >> >> On 1/8/19 3:25 PM, Dan Murphy wrote: >>> Jacek >>> >>> On 1/8/19 3:18 PM, Jacek Anaszewski wrote: Hi Dan, On 1/7/19 10:14 PM, Dan Murphy wrote: > Jacek > > On 1/7/19 2:59 PM, Jacek Anaszewski wrote: >> Dan, >> >> On 1/7/19 8:36 PM, Dan Murphy wrote: >>> Jacek >>> >>> On 1/7/19 1:13 PM, Jacek Anaszewski wrote: On 1/6/19 4:52 PM, Jacek Anaszewski wrote: > Hi Pavel, > > On 1/5/19 11:12 PM, Pavel Machek wrote: >> Hi! >> Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... >>> >>> Video [0] gives some overview of lp5024 capabilities. >>> >>> I don't see any problems in exposing separate red,green,blue >>> files and brightness for the devices with hardware support for >>> that. >> >> Well, that's what we do today, as three separate LEDs, right? > > No. It doesn't allow for setting color intensity by having > the color fixed beforehand. Below is relevant excerpt from > the lp5024 documentation. This is not something that can be > mapped to RGB color space, but rather to HSV/HSL, with the > reservation that the hardware implementation uses PWM > for setting color intensity. > > > 8.3.1.2 Independent Intensity Control Per RGB LED Module > > When color is fixed, the independent intensity-control is used to > achieve accurate and flexible dimming control for every RGB LED > module. > > 8.3.1.2.1 Intensity-Control Register Configuration > > Every three consecutive output channels are assigned to their > respective > intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, > and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to > connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx > device allows 256-step intensity control for each RGB LED module, > which > helps achieve a smooth dimming effect. > > >> I don't have problem with that, either; other drivers already do >> that. He's free to use existing same interface. >> >> But that is insufficient, as it does not allow simple stuff, such as >> turning led "white". >> >> So... perhaps we should agree on requirements, first, and then we can >> discuss solutions? >> >> Requirements for RGB LED interface: >> >> 1) Userspace should be able to set the white color >> >> 2) Userspace should be able to arbitrary color from well known list >> and it should approximately match what would CRT, LCD or OLED >> monitor display > > The difference is that monitor display driver is pre-calibrated > for given display by the manufacturer. With the LED controllers the > manufacturer has no control over what LEDs will be connected to the > iouts. Therefore it should be not surprising that colors produced > by custom LEDs are not as user would expect when comparing to > the RGB color displayed on the monitor display. > > TI provides "Various LED Ring Lighting Patterns Reference Design" [0] > that show how to produce various patterns with use of the reference > board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs > [1]. > > Document [0] mentions also specific "Design considerations" in the > chapter 2.2: > > > Several considerations are taken into account for this particular > design: > • LED map (ring) for meeting the requirement of popular human-machine > interaction style. > • LED size, numbers and the diffuse design for meeting lighting > pattern uniformity. > • Analog dimming in the difference ambient light lux without losing > dimming resolution in lighting pattern. > > These considerations apply to most human-machine interaction end > equipment with day and night vision > designs in some way, but the designer must decide the particular > considerations to take into account for a > specific design. > > > This renders your requirement 2) infeasible with use of custom LEDs > any fixed algorithm, since the final effect will always heavily depend Typo here: s/any fixed/and fixed/ > on the LED circuit design. > >> >> 2a) LEDs probably slightly change color as they age. That's
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Dan, On 1/10/19 1:46 PM, Dan Murphy wrote: Jacek On 1/8/19 3:25 PM, Dan Murphy wrote: Jacek On 1/8/19 3:18 PM, Jacek Anaszewski wrote: Hi Dan, On 1/7/19 10:14 PM, Dan Murphy wrote: Jacek On 1/7/19 2:59 PM, Jacek Anaszewski wrote: Dan, On 1/7/19 8:36 PM, Dan Murphy wrote: Jacek On 1/7/19 1:13 PM, Jacek Anaszewski wrote: On 1/6/19 4:52 PM, Jacek Anaszewski wrote: Hi Pavel, On 1/5/19 11:12 PM, Pavel Machek wrote: Hi! Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. 8.3.1.2 Independent Intensity Control Per RGB LED Module When color is fixed, the independent intensity-control is used to achieve accurate and flexible dimming control for every RGB LED module. 8.3.1.2.1 Intensity-Control Register Configuration Every three consecutive output channels are assigned to their respective intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx device allows 256-step intensity control for each RGB LED module, which helps achieve a smooth dimming effect. I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. TI provides "Various LED Ring Lighting Patterns Reference Design" [0] that show how to produce various patterns with use of the reference board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. Document [0] mentions also specific "Design considerations" in the chapter 2.2: Several considerations are taken into account for this particular design: • LED map (ring) for meeting the requirement of popular human-machine interaction style. • LED size, numbers and the diffuse design for meeting lighting pattern uniformity. • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern. These considerations apply to most human-machine interaction end equipment with day and night vision designs in some way, but the designer must decide the particular considerations to take into account for a specific design. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend Typo here: s/any fixed/and fixed/ on the LED circuit design. 2a) LEDs probably slightly change color as they age. That's out of scope, unless the variation is much greater than on monitors. 2b) Manufacturing differences cause small color variation. Again, that's out of scope, unless the variation is much greater than on monitors. Nice to have features: 3) Full range of available colors/intensities should be available to userspace 4) Interface should work well with existing triggers 5) It would be nice if userland knew how many lumens are produced at each wavelength for each setting (again, minus aging and manufacturing variations). 6) Complexity of math in kernel should be low, and preferably it should be integer or fixed point Problems: a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential relation. Human eyes percieve logarithm of lumens. RGB color model uses even more complex
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Jacek On 1/8/19 3:25 PM, Dan Murphy wrote: > Jacek > > On 1/8/19 3:18 PM, Jacek Anaszewski wrote: >> Hi Dan, >> >> On 1/7/19 10:14 PM, Dan Murphy wrote: >>> Jacek >>> >>> On 1/7/19 2:59 PM, Jacek Anaszewski wrote: Dan, On 1/7/19 8:36 PM, Dan Murphy wrote: > Jacek > > On 1/7/19 1:13 PM, Jacek Anaszewski wrote: >> On 1/6/19 4:52 PM, Jacek Anaszewski wrote: >>> Hi Pavel, >>> >>> On 1/5/19 11:12 PM, Pavel Machek wrote: Hi! >> Grab yourself an RGB LED and play with it; you'll see what the >> problems are. It is hard to explain colors over email... > > Video [0] gives some overview of lp5024 capabilities. > > I don't see any problems in exposing separate red,green,blue > files and brightness for the devices with hardware support for > that. Well, that's what we do today, as three separate LEDs, right? >>> >>> No. It doesn't allow for setting color intensity by having >>> the color fixed beforehand. Below is relevant excerpt from >>> the lp5024 documentation. This is not something that can be >>> mapped to RGB color space, but rather to HSV/HSL, with the >>> reservation that the hardware implementation uses PWM >>> for setting color intensity. >>> >>> >>> 8.3.1.2 Independent Intensity Control Per RGB LED Module >>> >>> When color is fixed, the independent intensity-control is used to >>> achieve accurate and flexible dimming control for every RGB LED module. >>> >>> 8.3.1.2.1 Intensity-Control Register Configuration >>> >>> Every three consecutive output channels are assigned to their respective >>> intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, >>> and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to >>> connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx >>> device allows 256-step intensity control for each RGB LED module, which >>> helps achieve a smooth dimming effect. >>> >>> I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display >>> >>> The difference is that monitor display driver is pre-calibrated >>> for given display by the manufacturer. With the LED controllers the >>> manufacturer has no control over what LEDs will be connected to the >>> iouts. Therefore it should be not surprising that colors produced >>> by custom LEDs are not as user would expect when comparing to >>> the RGB color displayed on the monitor display. >>> >>> TI provides "Various LED Ring Lighting Patterns Reference Design" [0] >>> that show how to produce various patterns with use of the reference >>> board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. >>> >>> Document [0] mentions also specific "Design considerations" in the >>> chapter 2.2: >>> >>> >>> Several considerations are taken into account for this particular >>> design: >>> • LED map (ring) for meeting the requirement of popular human-machine >>> interaction style. >>> • LED size, numbers and the diffuse design for meeting lighting pattern >>> uniformity. >>> • Analog dimming in the difference ambient light lux without losing >>> dimming resolution in lighting pattern. >>> >>> These considerations apply to most human-machine interaction end >>> equipment with day and night vision >>> designs in some way, but the designer must decide the particular >>> considerations to take into account for a >>> specific design. >>> >>> >>> This renders your requirement 2) infeasible with use of custom LEDs >>> any fixed algorithm, since the final effect will always heavily depend >> >> Typo here: s/any fixed/and fixed/ >> >>> on the LED circuit design. >>> 2a) LEDs probably slightly change color as they age. That's out of scope, unless the variation is much greater than on monitors. 2b) Manufacturing differences cause small color variation. Again, that's out of scope, unless the variation is much greater than on monitors. Nice to have features: 3)
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Pavel, On 09/01/2019 0.59, Pavel Machek wrote: Hi! Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. So they have feature where they have independent controls for each channel, then one common control per three channels. Other chips have common control for all the LEDs, for example. We don't support that currently; lets focus on the RGB thing first. I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. It is true that _chip_ manufacturer can not know what LEDs will be connected. But _system_ manufacturer can and should know that, and should tell be able to tell us in the dts. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend on the LED circuit design. Depending on LED circuit design and actual LEDs connected is okay.. we just need to get information from _system_ designer (not chip designer), and pass it to a place where color is computed. a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential relation. Human eyes percieve logarithm of lumens. RGB color model uses even more complex function. One general question: do you have any solutions in store? I played with LEDs on N900 over the weekend, yes. And getting reasonable colors seems to be possible, when a) and b) are solved... a) seems to be more important than b). Now... this does not tell us how we should design kernel<->user interface, but it should tell us that main goals - 1) and 2) are possible. I was thinking about this calibration and color correctness thing and I am thinking a bit that it should be partly in kernel and partly in user space. For displays and printers there are defined icc-profiles that define how colors are mapped to particular device in cases when you want to have accurate color representation. In theory to get accurate LED output one could model LEDs with icc profile and then pick your color and convert it with icc profile to actual raw hardware values. Then this raw hardware value could be written from user space to kernel. In kernel we could provide raw hardware value support and in case of PWM IC controlled LEDs we could provide curve mapping to linearize the behavior of values entered to enable use in cases where close enough is good enough. Eg. if you want to have "white" then you have your color space picker like sRGB(255,255,255). Then you would define icc profile it might translate to hardware raw values 253%, 230%, 225%. Then you would write this to kernel with: # set red, green and blue color elements echo "253 230 225" > color In this case however behavior of brightness node is challening in accuracy domain. Britghtness value 255 would of course provide 1:1 mapping in this case. To go right to correct color and brightness at one go we could have optional brightness at the end of color value array: # set red, green and blue color element and brightness setting: echo "253 230 225 255" > color If you want to have fancier behavior for brightness in your system then we probably need to have configurable brightness model. - hardware, like in lp5024 case would map to
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
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 @ {
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
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Dan,
On 07/01/2019 21.34, Dan Murphy wrote:
Vesa
On 1/4/19 6:39 PM, 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 @ {
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
Can we not use led-sources like I have done already?
It was just for illustration of the idea. Names can be agreed. I have
nothing against led-sources name. I was just looking at datasheet to try
to undestand what it did and then tried to figure out if it could be
mapped the idea I have been playing with.
I really like to keep the DT nodes simple and re-use nodes that exist if
possible.
I'll reply to Jacek's email about more clarifications of the idea.
Thanks,
Vesa Jääskeläinen
My code already maps and groups the outputs into the associated banks
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > >>>Grab yourself an RGB LED and play with it; you'll see what the > >>>problems are. It is hard to explain colors over email... > >> > >>Video [0] gives some overview of lp5024 capabilities. > >> > >>I don't see any problems in exposing separate red,green,blue > >>files and brightness for the devices with hardware support for > >>that. > > > >Well, that's what we do today, as three separate LEDs, right? > > No. It doesn't allow for setting color intensity by having > the color fixed beforehand. Below is relevant excerpt from > the lp5024 documentation. This is not something that can be > mapped to RGB color space, but rather to HSV/HSL, with the > reservation that the hardware implementation uses PWM > for setting color intensity. So they have feature where they have independent controls for each channel, then one common control per three channels. Other chips have common control for all the LEDs, for example. We don't support that currently; lets focus on the RGB thing first. > >I don't have problem with that, either; other drivers already do > >that. He's free to use existing same interface. > > > >But that is insufficient, as it does not allow simple stuff, such as > >turning led "white". > > > >So... perhaps we should agree on requirements, first, and then we can > >discuss solutions? > > > >Requirements for RGB LED interface: > > > >1) Userspace should be able to set the white color > > > >2) Userspace should be able to arbitrary color from well known list > >and it should approximately match what would CRT, LCD or OLED monitor display > > The difference is that monitor display driver is pre-calibrated > for given display by the manufacturer. With the LED controllers the > manufacturer has no control over what LEDs will be connected to the > iouts. Therefore it should be not surprising that colors produced > by custom LEDs are not as user would expect when comparing to > the RGB color displayed on the monitor display. It is true that _chip_ manufacturer can not know what LEDs will be connected. But _system_ manufacturer can and should know that, and should tell be able to tell us in the dts. > This renders your requirement 2) infeasible with use of custom LEDs > any fixed algorithm, since the final effect will always heavily depend > on the LED circuit design. Depending on LED circuit design and actual LEDs connected is okay.. we just need to get information from _system_ designer (not chip designer), and pass it to a place where color is computed. > >a) RGB LEDs are usually not balanced. Setting 100% PWM on > >red/green/blue channels will result in nothing close to white > >light. In fact, to get white light on N900, blue and green channel's > >PWM needs to be set pretty low, as in 5%. > > > >b) LED class does not define any relation between "brightness" in > >sysfs and ammount of light in lumens. Some drivers use close to linear > >relation, some use exponential relation. Human eyes percieve logarithm > >of lumens. RGB color model uses even more complex function. > > One general question: do you have any solutions in store? I played with LEDs on N900 over the weekend, yes. And getting reasonable colors seems to be possible, when a) and b) are solved... a) seems to be more important than b). Now... this does not tell us how we should design kernel<->user interface, but it should tell us that main goals - 1) and 2) are possible. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Jacek On 1/8/19 3:18 PM, Jacek Anaszewski wrote: > Hi Dan, > > On 1/7/19 10:14 PM, Dan Murphy wrote: >> Jacek >> >> On 1/7/19 2:59 PM, Jacek Anaszewski wrote: >>> Dan, >>> >>> On 1/7/19 8:36 PM, Dan Murphy wrote: Jacek On 1/7/19 1:13 PM, Jacek Anaszewski wrote: > On 1/6/19 4:52 PM, Jacek Anaszewski wrote: >> Hi Pavel, >> >> On 1/5/19 11:12 PM, Pavel Machek wrote: >>> Hi! >>> > Grab yourself an RGB LED and play with it; you'll see what the > problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. >>> >>> Well, that's what we do today, as three separate LEDs, right? >> >> No. It doesn't allow for setting color intensity by having >> the color fixed beforehand. Below is relevant excerpt from >> the lp5024 documentation. This is not something that can be >> mapped to RGB color space, but rather to HSV/HSL, with the >> reservation that the hardware implementation uses PWM >> for setting color intensity. >> >> >> 8.3.1.2 Independent Intensity Control Per RGB LED Module >> >> When color is fixed, the independent intensity-control is used to >> achieve accurate and flexible dimming control for every RGB LED module. >> >> 8.3.1.2.1 Intensity-Control Register Configuration >> >> Every three consecutive output channels are assigned to their respective >> intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, >> and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to >> connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx >> device allows 256-step intensity control for each RGB LED module, which >> helps achieve a smooth dimming effect. >> >> >>> I don't have problem with that, either; other drivers already do >>> that. He's free to use existing same interface. >>> >>> But that is insufficient, as it does not allow simple stuff, such as >>> turning led "white". >>> >>> So... perhaps we should agree on requirements, first, and then we can >>> discuss solutions? >>> >>> Requirements for RGB LED interface: >>> >>> 1) Userspace should be able to set the white color >>> >>> 2) Userspace should be able to arbitrary color from well known list >>> and it should approximately match what would CRT, LCD or OLED monitor >>> display >> >> The difference is that monitor display driver is pre-calibrated >> for given display by the manufacturer. With the LED controllers the >> manufacturer has no control over what LEDs will be connected to the >> iouts. Therefore it should be not surprising that colors produced >> by custom LEDs are not as user would expect when comparing to >> the RGB color displayed on the monitor display. >> >> TI provides "Various LED Ring Lighting Patterns Reference Design" [0] >> that show how to produce various patterns with use of the reference >> board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. >> >> Document [0] mentions also specific "Design considerations" in the >> chapter 2.2: >> >> >> Several considerations are taken into account for this particular design: >> • LED map (ring) for meeting the requirement of popular human-machine >> interaction style. >> • LED size, numbers and the diffuse design for meeting lighting pattern >> uniformity. >> • Analog dimming in the difference ambient light lux without losing >> dimming resolution in lighting pattern. >> >> These considerations apply to most human-machine interaction end >> equipment with day and night vision >> designs in some way, but the designer must decide the particular >> considerations to take into account for a >> specific design. >> >> >> This renders your requirement 2) infeasible with use of custom LEDs >> any fixed algorithm, since the final effect will always heavily depend > > Typo here: s/any fixed/and fixed/ > >> on the LED circuit design. >> >>> >>> 2a) LEDs probably slightly change color as they age. That's out >>> of >>> scope, unless the variation is much greater than on monitors. >>> >>> 2b) Manufacturing differences cause small color variation. Again, >>> that's out of scope, unless the variation is much greater than on >>> monitors. >>> >>> Nice to have features: >>> >>> 3) Full range of available colors/intensities should be available to >>> userspace >>> >>> 4) Interface should work well with existing triggers >>> >>> 5) It would be nice if
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Jacek
Thanks for the review. v2 will contain the LP5030/36 as well.
On 1/8/19 3:10 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 12/19/18 5:26 PM, Dan Murphy wrote:
>> Introduce the LP5024 and LP5018 RGB LED driver.
>> The difference in these 2 parts are only in the number of
>> LED outputs where the LP5024 can control 24 LEDs the LP5018
>> can only control 18.
>>
>> The device has the ability to group LED output into control banks
>> so that multiple LED banks can be controlled with the same mixing and
>> brightness. Inversely the LEDs can also be controlled independently.
>>
>> Signed-off-by: Dan Murphy
>> ---
>> drivers/leds/Kconfig | 7 +
>> drivers/leds/Makefile | 1 +
>> drivers/leds/leds-lp5024.c | 610 +
>> 3 files changed, 618 insertions(+)
>> create mode 100644 drivers/leds/leds-lp5024.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index a72f97fca57b..d306bedb00b7 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -326,6 +326,13 @@ config LEDS_LP3952
>> To compile this driver as a module, choose M here: the
>> module will be called leds-lp3952.
>> +config LEDS_LP5024
>> + tristate "LED Support for TI LP5024/18 LED driver chip"
>> + depends on LEDS_CLASS && REGMAP_I2C
>> + help
>> + If you say yes here you get support for the Texas Instruments
>> + LP5024 and LP5018 LED driver.
>> +
>> config LEDS_LP55XX_COMMON
>> tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
>> depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 4c1b0054f379..60b4e4ddd3ee 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o
>> obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o
>> obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o
>> obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o
>> +obj-$(CONFIG_LEDS_LP5024) += leds-lp5024.o
>> obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o
>> obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o
>> obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o
>> diff --git a/drivers/leds/leds-lp5024.c b/drivers/leds/leds-lp5024.c
>> new file mode 100644
>> index ..90e8dca15609
>> --- /dev/null
>> +++ b/drivers/leds/leds-lp5024.c
>> @@ -0,0 +1,610 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* TI LP50XX LED chip family driver
>> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#define LP5024_DEV_CFG0 0x00
>> +#define LP5024_DEV_CFG1 0x01
>> +#define LP5024_LED_CFG0 0x02
>> +#define LP5024_BNK_BRT 0x03
>> +#define LP5024_BNKA_CLR 0x04
>> +#define LP5024_BNKB_CLR 0x05
>> +#define LP5024_BNKC_CLR 0x06
>> +#define LP5024_LED0_BRT 0x07
>> +#define LP5024_LED1_BRT 0x08
>> +#define LP5024_LED2_BRT 0x09
>> +#define LP5024_LED3_BRT 0x0a
>> +#define LP5024_LED4_BRT 0x0b
>> +#define LP5024_LED5_BRT 0x0c
>> +#define LP5024_LED6_BRT 0x0d
>> +#define LP5024_LED7_BRT 0x0e
>> +
>> +#define LP5024_OUT0_CLR 0x0f
>> +#define LP5024_OUT1_CLR 0x10
>> +#define LP5024_OUT2_CLR 0x11
>> +#define LP5024_OUT3_CLR 0x12
>> +#define LP5024_OUT4_CLR 0x13
>> +#define LP5024_OUT5_CLR 0x14
>> +#define LP5024_OUT6_CLR 0x15
>> +#define LP5024_OUT7_CLR 0x16
>> +#define LP5024_OUT8_CLR 0x17
>> +#define LP5024_OUT9_CLR 0x18
>> +#define LP5024_OUT10_CLR 0x19
>> +#define LP5024_OUT11_CLR 0x1a
>> +#define LP5024_OUT12_CLR 0x1b
>> +#define LP5024_OUT13_CLR 0x1c
>> +#define LP5024_OUT14_CLR 0x1d
>> +#define LP5024_OUT15_CLR 0x1e
>> +#define LP5024_OUT16_CLR 0x1f
>> +#define LP5024_OUT17_CLR 0x20
>> +#define LP5024_OUT18_CLR 0x21
>> +#define LP5024_OUT19_CLR 0x22
>> +#define LP5024_OUT20_CLR 0x23
>> +#define LP5024_OUT21_CLR 0x24
>> +#define LP5024_OUT22_CLR 0x25
>> +#define LP5024_OUT23_CLR 0x26
>> +
>> +#define LP5024_RESET 0x27
>> +#define LP5024_SW_RESET 0xff
>> +
>> +#define LP5024_CHIP_EN BIT(6)
>> +
>> +#define LP5024_CONTROL_A 0
>> +#define LP5024_CONTROL_B 1
>> +#define LP5024_CONTROL_C 2
>> +#define LP5024_MAX_CONTROL_BANKS 3
>> +
>> +#define LP5018_MAX_LED_STRINGS 6
>> +#define LP5024_MAX_LED_STRINGS 8
>> +
>> +enum lp5024_model {
>> + LP5018,
>> + LP5024,
>> +};
>> +
>> +struct lp5024_led {
>> + u32 led_strings[LP5024_MAX_LED_STRINGS];
>> + char label[LED_MAX_NAME_SIZE];
>> + struct led_classdev led_dev;
>> + struct lp5024 *priv;
>> + int
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Dan, On 1/7/19 10:14 PM, Dan Murphy wrote: Jacek On 1/7/19 2:59 PM, Jacek Anaszewski wrote: Dan, On 1/7/19 8:36 PM, Dan Murphy wrote: Jacek On 1/7/19 1:13 PM, Jacek Anaszewski wrote: On 1/6/19 4:52 PM, Jacek Anaszewski wrote: Hi Pavel, On 1/5/19 11:12 PM, Pavel Machek wrote: Hi! Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. 8.3.1.2 Independent Intensity Control Per RGB LED Module When color is fixed, the independent intensity-control is used to achieve accurate and flexible dimming control for every RGB LED module. 8.3.1.2.1 Intensity-Control Register Configuration Every three consecutive output channels are assigned to their respective intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx device allows 256-step intensity control for each RGB LED module, which helps achieve a smooth dimming effect. I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. TI provides "Various LED Ring Lighting Patterns Reference Design" [0] that show how to produce various patterns with use of the reference board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. Document [0] mentions also specific "Design considerations" in the chapter 2.2: Several considerations are taken into account for this particular design: • LED map (ring) for meeting the requirement of popular human-machine interaction style. • LED size, numbers and the diffuse design for meeting lighting pattern uniformity. • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern. These considerations apply to most human-machine interaction end equipment with day and night vision designs in some way, but the designer must decide the particular considerations to take into account for a specific design. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend Typo here: s/any fixed/and fixed/ on the LED circuit design. 2a) LEDs probably slightly change color as they age. That's out of scope, unless the variation is much greater than on monitors. 2b) Manufacturing differences cause small color variation. Again, that's out of scope, unless the variation is much greater than on monitors. Nice to have features: 3) Full range of available colors/intensities should be available to userspace 4) Interface should work well with existing triggers 5) It would be nice if userland knew how many lumens are produced at each wavelength for each setting (again, minus aging and manufacturing variations). 6) Complexity of math in kernel should be low, and preferably it should be integer or fixed point Problems: a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential relation. Human eyes percieve logarithm of lumens. RGB color model uses even more complex function. c) Except for white LEDs, LEDs are basically sources of single wavelength of light, while CRTs and LCDs produce broader spectrums. d)
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Dan,
On 12/19/18 5:26 PM, Dan Murphy wrote:
Introduce the LP5024 and LP5018 RGB LED driver.
The difference in these 2 parts are only in the number of
LED outputs where the LP5024 can control 24 LEDs the LP5018
can only control 18.
The device has the ability to group LED output into control banks
so that multiple LED banks can be controlled with the same mixing and
brightness. Inversely the LEDs can also be controlled independently.
Signed-off-by: Dan Murphy
---
drivers/leds/Kconfig | 7 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-lp5024.c | 610 +
3 files changed, 618 insertions(+)
create mode 100644 drivers/leds/leds-lp5024.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a72f97fca57b..d306bedb00b7 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -326,6 +326,13 @@ config LEDS_LP3952
To compile this driver as a module, choose M here: the
module will be called leds-lp3952.
+config LEDS_LP5024
+ tristate "LED Support for TI LP5024/18 LED driver chip"
+ depends on LEDS_CLASS && REGMAP_I2C
+ help
+ If you say yes here you get support for the Texas Instruments
+ LP5024 and LP5018 LED driver.
+
config LEDS_LP55XX_COMMON
tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501"
depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b0054f379..60b4e4ddd3ee 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o
obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o
obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o
obj-$(CONFIG_LEDS_LP3952) += leds-lp3952.o
+obj-$(CONFIG_LEDS_LP5024) += leds-lp5024.o
obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o
obj-$(CONFIG_LEDS_LP5521) += leds-lp5521.o
obj-$(CONFIG_LEDS_LP5523) += leds-lp5523.o
diff --git a/drivers/leds/leds-lp5024.c b/drivers/leds/leds-lp5024.c
new file mode 100644
index ..90e8dca15609
--- /dev/null
+++ b/drivers/leds/leds-lp5024.c
@@ -0,0 +1,610 @@
+// SPDX-License-Identifier: GPL-2.0
+/* TI LP50XX LED chip family driver
+ * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define LP5024_DEV_CFG00x00
+#define LP5024_DEV_CFG10x01
+#define LP5024_LED_CFG00x02
+#define LP5024_BNK_BRT 0x03
+#define LP5024_BNKA_CLR0x04
+#define LP5024_BNKB_CLR0x05
+#define LP5024_BNKC_CLR0x06
+#define LP5024_LED0_BRT0x07
+#define LP5024_LED1_BRT0x08
+#define LP5024_LED2_BRT0x09
+#define LP5024_LED3_BRT0x0a
+#define LP5024_LED4_BRT0x0b
+#define LP5024_LED5_BRT0x0c
+#define LP5024_LED6_BRT0x0d
+#define LP5024_LED7_BRT0x0e
+
+#define LP5024_OUT0_CLR0x0f
+#define LP5024_OUT1_CLR0x10
+#define LP5024_OUT2_CLR0x11
+#define LP5024_OUT3_CLR0x12
+#define LP5024_OUT4_CLR0x13
+#define LP5024_OUT5_CLR0x14
+#define LP5024_OUT6_CLR0x15
+#define LP5024_OUT7_CLR0x16
+#define LP5024_OUT8_CLR0x17
+#define LP5024_OUT9_CLR0x18
+#define LP5024_OUT10_CLR 0x19
+#define LP5024_OUT11_CLR 0x1a
+#define LP5024_OUT12_CLR 0x1b
+#define LP5024_OUT13_CLR 0x1c
+#define LP5024_OUT14_CLR 0x1d
+#define LP5024_OUT15_CLR 0x1e
+#define LP5024_OUT16_CLR 0x1f
+#define LP5024_OUT17_CLR 0x20
+#define LP5024_OUT18_CLR 0x21
+#define LP5024_OUT19_CLR 0x22
+#define LP5024_OUT20_CLR 0x23
+#define LP5024_OUT21_CLR 0x24
+#define LP5024_OUT22_CLR 0x25
+#define LP5024_OUT23_CLR 0x26
+
+#define LP5024_RESET 0x27
+#define LP5024_SW_RESET0xff
+
+#define LP5024_CHIP_EN BIT(6)
+
+#define LP5024_CONTROL_A 0
+#define LP5024_CONTROL_B 1
+#define LP5024_CONTROL_C 2
+#define LP5024_MAX_CONTROL_BANKS 3
+
+#define LP5018_MAX_LED_STRINGS 6
+#define LP5024_MAX_LED_STRINGS 8
+
+enum lp5024_model {
+ LP5018,
+ LP5024,
+};
+
+struct lp5024_led {
+ u32 led_strings[LP5024_MAX_LED_STRINGS];
+ char label[LED_MAX_NAME_SIZE];
+ struct led_classdev led_dev;
+ struct lp5024 *priv;
+ int led_number;
+ u8 ctrl_bank_enabled;
+};
+
+/**
+ * struct lp5024 -
+ * @enable_gpio: Hardware enable gpio
+ * @regulator: LED supply regulator pointer
+ * @client:
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Jacek
On 1/7/19 3:13 PM, 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 @ {
>> 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.
+1 to that comment
>
> Also, you provided scarce information about sysfs interface.
> It would be nice to see the sequence of commands.
>
--
--
Dan Murphy
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Jacek On 1/7/19 2:59 PM, Jacek Anaszewski wrote: > Dan, > > On 1/7/19 8:36 PM, Dan Murphy wrote: >> Jacek >> >> On 1/7/19 1:13 PM, Jacek Anaszewski wrote: >>> On 1/6/19 4:52 PM, Jacek Anaszewski wrote: Hi Pavel, On 1/5/19 11:12 PM, Pavel Machek wrote: > Hi! > >>> Grab yourself an RGB LED and play with it; you'll see what the >>> problems are. It is hard to explain colors over email... >> >> Video [0] gives some overview of lp5024 capabilities. >> >> I don't see any problems in exposing separate red,green,blue >> files and brightness for the devices with hardware support for >> that. > > Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. 8.3.1.2 Independent Intensity Control Per RGB LED Module When color is fixed, the independent intensity-control is used to achieve accurate and flexible dimming control for every RGB LED module. 8.3.1.2.1 Intensity-Control Register Configuration Every three consecutive output channels are assigned to their respective intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx device allows 256-step intensity control for each RGB LED module, which helps achieve a smooth dimming effect. > I don't have problem with that, either; other drivers already do > that. He's free to use existing same interface. > > But that is insufficient, as it does not allow simple stuff, such as > turning led "white". > > So... perhaps we should agree on requirements, first, and then we can > discuss solutions? > > Requirements for RGB LED interface: > > 1) Userspace should be able to set the white color > > 2) Userspace should be able to arbitrary color from well known list > and it should approximately match what would CRT, LCD or OLED monitor > display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. TI provides "Various LED Ring Lighting Patterns Reference Design" [0] that show how to produce various patterns with use of the reference board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. Document [0] mentions also specific "Design considerations" in the chapter 2.2: Several considerations are taken into account for this particular design: • LED map (ring) for meeting the requirement of popular human-machine interaction style. • LED size, numbers and the diffuse design for meeting lighting pattern uniformity. • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern. These considerations apply to most human-machine interaction end equipment with day and night vision designs in some way, but the designer must decide the particular considerations to take into account for a specific design. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend >>> >>> Typo here: s/any fixed/and fixed/ >>> on the LED circuit design. > > 2a) LEDs probably slightly change color as they age. That's out of > scope, unless the variation is much greater than on monitors. > > 2b) Manufacturing differences cause small color variation. Again, > that's out of scope, unless the variation is much greater than on > monitors. > > Nice to have features: > > 3) Full range of available colors/intensities should be available to > userspace > > 4) Interface should work well with existing triggers > > 5) It would be nice if userland knew how many lumens are produced at > each wavelength for each setting (again, minus aging and manufacturing > variations). > > 6) Complexity of math in kernel should be low, and preferably it > should be integer or fixed point > > Problems: > > a) RGB LEDs are usually not balanced. Setting 100% PWM on >
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
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 @ {
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.
Also, you provided scarce information about sysfs interface.
It would be nice to see the sequence of commands.
--
Best regards,
Jacek Anaszewski
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Dan, On 1/7/19 8:36 PM, Dan Murphy wrote: Jacek On 1/7/19 1:13 PM, Jacek Anaszewski wrote: On 1/6/19 4:52 PM, Jacek Anaszewski wrote: Hi Pavel, On 1/5/19 11:12 PM, Pavel Machek wrote: Hi! Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. 8.3.1.2 Independent Intensity Control Per RGB LED Module When color is fixed, the independent intensity-control is used to achieve accurate and flexible dimming control for every RGB LED module. 8.3.1.2.1 Intensity-Control Register Configuration Every three consecutive output channels are assigned to their respective intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx device allows 256-step intensity control for each RGB LED module, which helps achieve a smooth dimming effect. I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. TI provides "Various LED Ring Lighting Patterns Reference Design" [0] that show how to produce various patterns with use of the reference board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. Document [0] mentions also specific "Design considerations" in the chapter 2.2: Several considerations are taken into account for this particular design: • LED map (ring) for meeting the requirement of popular human-machine interaction style. • LED size, numbers and the diffuse design for meeting lighting pattern uniformity. • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern. These considerations apply to most human-machine interaction end equipment with day and night vision designs in some way, but the designer must decide the particular considerations to take into account for a specific design. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend Typo here: s/any fixed/and fixed/ on the LED circuit design. 2a) LEDs probably slightly change color as they age. That's out of scope, unless the variation is much greater than on monitors. 2b) Manufacturing differences cause small color variation. Again, that's out of scope, unless the variation is much greater than on monitors. Nice to have features: 3) Full range of available colors/intensities should be available to userspace 4) Interface should work well with existing triggers 5) It would be nice if userland knew how many lumens are produced at each wavelength for each setting (again, minus aging and manufacturing variations). 6) Complexity of math in kernel should be low, and preferably it should be integer or fixed point Problems: a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential relation. Human eyes percieve logarithm of lumens. RGB color model uses even more complex function. c) Except for white LEDs, LEDs are basically sources of single wavelength of light, while CRTs and LCDs produce broader spectrums. d) RG, RGBW and probably other LED combinations exist. e) Not all "red" LEDs will produce same wavelength.
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Jacek On 1/7/19 1:13 PM, Jacek Anaszewski wrote: > On 1/6/19 4:52 PM, Jacek Anaszewski wrote: >> Hi Pavel, >> >> On 1/5/19 11:12 PM, Pavel Machek wrote: >>> Hi! >>> > Grab yourself an RGB LED and play with it; you'll see what the > problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. >>> >>> Well, that's what we do today, as three separate LEDs, right? >> >> No. It doesn't allow for setting color intensity by having >> the color fixed beforehand. Below is relevant excerpt from >> the lp5024 documentation. This is not something that can be >> mapped to RGB color space, but rather to HSV/HSL, with the >> reservation that the hardware implementation uses PWM >> for setting color intensity. >> >> >> 8.3.1.2 Independent Intensity Control Per RGB LED Module >> >> When color is fixed, the independent intensity-control is used to >> achieve accurate and flexible dimming control for every RGB LED module. >> >> 8.3.1.2.1 Intensity-Control Register Configuration >> >> Every three consecutive output channels are assigned to their respective >> intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, >> and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to >> connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx >> device allows 256-step intensity control for each RGB LED module, which >> helps achieve a smooth dimming effect. >> >> >>> I don't have problem with that, either; other drivers already do >>> that. He's free to use existing same interface. >>> >>> But that is insufficient, as it does not allow simple stuff, such as >>> turning led "white". >>> >>> So... perhaps we should agree on requirements, first, and then we can >>> discuss solutions? >>> >>> Requirements for RGB LED interface: >>> >>> 1) Userspace should be able to set the white color >>> >>> 2) Userspace should be able to arbitrary color from well known list >>> and it should approximately match what would CRT, LCD or OLED monitor >>> display >> >> The difference is that monitor display driver is pre-calibrated >> for given display by the manufacturer. With the LED controllers the >> manufacturer has no control over what LEDs will be connected to the >> iouts. Therefore it should be not surprising that colors produced >> by custom LEDs are not as user would expect when comparing to >> the RGB color displayed on the monitor display. >> >> TI provides "Various LED Ring Lighting Patterns Reference Design" [0] >> that show how to produce various patterns with use of the reference >> board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. >> >> Document [0] mentions also specific "Design considerations" in the >> chapter 2.2: >> >> >> Several considerations are taken into account for this particular design: >> • LED map (ring) for meeting the requirement of popular human-machine >> interaction style. >> • LED size, numbers and the diffuse design for meeting lighting pattern >> uniformity. >> • Analog dimming in the difference ambient light lux without losing dimming >> resolution in lighting pattern. >> >> These considerations apply to most human-machine interaction end equipment >> with day and night vision >> designs in some way, but the designer must decide the particular >> considerations to take into account for a >> specific design. >> >> >> This renders your requirement 2) infeasible with use of custom LEDs >> any fixed algorithm, since the final effect will always heavily depend > > Typo here: s/any fixed/and fixed/ > >> on the LED circuit design. >> >>> >>> 2a) LEDs probably slightly change color as they age. That's out of >>> scope, unless the variation is much greater than on monitors. >>> >>> 2b) Manufacturing differences cause small color variation. Again, >>> that's out of scope, unless the variation is much greater than on >>> monitors. >>> >>> Nice to have features: >>> >>> 3) Full range of available colors/intensities should be available to >>> userspace >>> >>> 4) Interface should work well with existing triggers >>> >>> 5) It would be nice if userland knew how many lumens are produced at >>> each wavelength for each setting (again, minus aging and manufacturing >>> variations). >>> >>> 6) Complexity of math in kernel should be low, and preferably it >>> should be integer or fixed point >>> >>> Problems: >>> >>> a) RGB LEDs are usually not balanced. Setting 100% PWM on >>> red/green/blue channels will result in nothing close to white >>> light. In fact, to get white light on N900, blue and green channel's >>> PWM needs to be set pretty low, as in 5%. >>> >>> b) LED class does not define any relation between "brightness" in >>> sysfs and ammount of light in lumens. Some drivers use close to linear >>> relation, some use
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Vesa
On 1/4/19 6:39 PM, 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 @ {
> 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
>
Can we not use led-sources like I have done already?
I really like to keep the DT nodes simple and re-use nodes that exist if
possible.
My code already maps and groups the outputs into the associated banks
Dan
--
--
Dan Murphy
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 1/6/19 4:52 PM, Jacek Anaszewski wrote: Hi Pavel, On 1/5/19 11:12 PM, Pavel Machek wrote: Hi! Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. 8.3.1.2 Independent Intensity Control Per RGB LED Module When color is fixed, the independent intensity-control is used to achieve accurate and flexible dimming control for every RGB LED module. 8.3.1.2.1 Intensity-Control Register Configuration Every three consecutive output channels are assigned to their respective intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx device allows 256-step intensity control for each RGB LED module, which helps achieve a smooth dimming effect. I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. TI provides "Various LED Ring Lighting Patterns Reference Design" [0] that show how to produce various patterns with use of the reference board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. Document [0] mentions also specific "Design considerations" in the chapter 2.2: Several considerations are taken into account for this particular design: • LED map (ring) for meeting the requirement of popular human-machine interaction style. • LED size, numbers and the diffuse design for meeting lighting pattern uniformity. • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern. These considerations apply to most human-machine interaction end equipment with day and night vision designs in some way, but the designer must decide the particular considerations to take into account for a specific design. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend Typo here: s/any fixed/and fixed/ on the LED circuit design. 2a) LEDs probably slightly change color as they age. That's out of scope, unless the variation is much greater than on monitors. 2b) Manufacturing differences cause small color variation. Again, that's out of scope, unless the variation is much greater than on monitors. Nice to have features: 3) Full range of available colors/intensities should be available to userspace 4) Interface should work well with existing triggers 5) It would be nice if userland knew how many lumens are produced at each wavelength for each setting (again, minus aging and manufacturing variations). 6) Complexity of math in kernel should be low, and preferably it should be integer or fixed point Problems: a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential relation. Human eyes percieve logarithm of lumens. RGB color model uses even more complex function. c) Except for white LEDs, LEDs are basically sources of single wavelength of light, while CRTs and LCDs produce broader spectrums. d) RG, RGBW and probably other LED combinations exist. e) Not all "red" LEDs will produce same wavelength. Similar differences will exist for other colors. f) We have existing RGB LEDs represented as
Re: Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Pavel, On 1/5/19 11:12 PM, Pavel Machek wrote: Hi! Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. Well, that's what we do today, as three separate LEDs, right? No. It doesn't allow for setting color intensity by having the color fixed beforehand. Below is relevant excerpt from the lp5024 documentation. This is not something that can be mapped to RGB color space, but rather to HSV/HSL, with the reservation that the hardware implementation uses PWM for setting color intensity. 8.3.1.2 Independent Intensity Control Per RGB LED Module When color is fixed, the independent intensity-control is used to achieve accurate and flexible dimming control for every RGB LED module. 8.3.1.2.1 Intensity-Control Register Configuration Every three consecutive output channels are assigned to their respective intensity-control register (LEDx_BRIGHTNESS). For example, OUT0, OUT1, and OUT2 are assigned to LED0_BRIGHTNESS, so it is recommended to connect the RGB LEDs in the sequence as shown in Table 1. The LP50xx device allows 256-step intensity control for each RGB LED module, which helps achieve a smooth dimming effect. I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display The difference is that monitor display driver is pre-calibrated for given display by the manufacturer. With the LED controllers the manufacturer has no control over what LEDs will be connected to the iouts. Therefore it should be not surprising that colors produced by custom LEDs are not as user would expect when comparing to the RGB color displayed on the monitor display. TI provides "Various LED Ring Lighting Patterns Reference Design" [0] that show how to produce various patterns with use of the reference board with LED ring built using sixteen 19-337/R6GHBHC-A01/2T LEDs [1]. Document [0] mentions also specific "Design considerations" in the chapter 2.2: Several considerations are taken into account for this particular design: • LED map (ring) for meeting the requirement of popular human-machine interaction style. • LED size, numbers and the diffuse design for meeting lighting pattern uniformity. • Analog dimming in the difference ambient light lux without losing dimming resolution in lighting pattern. These considerations apply to most human-machine interaction end equipment with day and night vision designs in some way, but the designer must decide the particular considerations to take into account for a specific design. This renders your requirement 2) infeasible with use of custom LEDs any fixed algorithm, since the final effect will always heavily depend on the LED circuit design. 2a) LEDs probably slightly change color as they age. That's out of scope, unless the variation is much greater than on monitors. 2b) Manufacturing differences cause small color variation. Again, that's out of scope, unless the variation is much greater than on monitors. Nice to have features: 3) Full range of available colors/intensities should be available to userspace 4) Interface should work well with existing triggers 5) It would be nice if userland knew how many lumens are produced at each wavelength for each setting (again, minus aging and manufacturing variations). 6) Complexity of math in kernel should be low, and preferably it should be integer or fixed point Problems: a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential relation. Human eyes percieve logarithm of lumens. RGB color model uses even more complex function. c) Except for white LEDs, LEDs are basically sources of single wavelength of light, while CRTs and LCDs produce broader spectrums. d) RG, RGBW and probably other LED combinations exist. e) Not all "red" LEDs will produce same wavelength. Similar differences will exist for other colors. f) We have existing RGB LEDs represented as three separate monochromatic LEDs in sysfs. One general question: do you have any
Generic RGB LED support was Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > >Grab yourself an RGB LED and play with it; you'll see what the > >problems are. It is hard to explain colors over email... > > Video [0] gives some overview of lp5024 capabilities. > > I don't see any problems in exposing separate red,green,blue > files and brightness for the devices with hardware support for > that. Well, that's what we do today, as three separate LEDs, right? I don't have problem with that, either; other drivers already do that. He's free to use existing same interface. But that is insufficient, as it does not allow simple stuff, such as turning led "white". So... perhaps we should agree on requirements, first, and then we can discuss solutions? Requirements for RGB LED interface: 1) Userspace should be able to set the white color 2) Userspace should be able to arbitrary color from well known list and it should approximately match what would CRT, LCD or OLED monitor display 2a) LEDs probably slightly change color as they age. That's out of scope, unless the variation is much greater than on monitors. 2b) Manufacturing differences cause small color variation. Again, that's out of scope, unless the variation is much greater than on monitors. Nice to have features: 3) Full range of available colors/intensities should be available to userspace 4) Interface should work well with existing triggers 5) It would be nice if userland knew how many lumens are produced at each wavelength for each setting (again, minus aging and manufacturing variations). 6) Complexity of math in kernel should be low, and preferably it should be integer or fixed point Problems: a) RGB LEDs are usually not balanced. Setting 100% PWM on red/green/blue channels will result in nothing close to white light. In fact, to get white light on N900, blue and green channel's PWM needs to be set pretty low, as in 5%. b) LED class does not define any relation between "brightness" in sysfs and ammount of light in lumens. Some drivers use close to linear relation, some use exponential relation. Human eyes percieve logarithm of lumens. RGB color model uses even more complex function. c) Except for white LEDs, LEDs are basically sources of single wavelength of light, while CRTs and LCDs produce broader spectrums. d) RG, RGBW and probably other LED combinations exist. e) Not all "red" LEDs will produce same wavelength. Similar differences will exist for other colors. f) We have existing RGB LEDs represented as three separate monochromatic LEDs in sysfs. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 1/5/19 1:31 PM, Pavel Machek wrote: We will need to solve RGB leds somehow, hopefully this is solved with it. When? With this attitude we will procrastinate it forever. It's been almost 3 years since first HSV patches. I proposed rough design of LED RGB class interface in [0]. If you find it totally flawed, then please spot the problems. If not, let's improve it and implement. That is fatally flawed, and inferior to the HSV patches, sorry... Please spot the problems. Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Video [0] gives some overview of lp5024 capabilities. I don't see any problems in exposing separate red,green,blue files and brightness for the devices with hardware support for that. I want to push things forward and I'm going to ask Dan to give this design a try. [0] https://www.youtube.com/watch?v=qdt-alh8i6E -- Best regards, Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
> >We will need to solve RGB leds somehow, hopefully this is solved with > >it. > > When? With this attitude we will procrastinate it forever. > It's been almost 3 years since first HSV patches. > > I proposed rough design of LED RGB class interface in [0]. > If you find it totally flawed, then please spot the problems. > If not, let's improve it and implement. That is fatally flawed, and inferior to the HSV patches, sorry... Grab yourself an RGB LED and play with it; you'll see what the problems are. It is hard to explain colors over email... Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Pavel, On 1/4/19 11:07 PM, Pavel Machek wrote: Hi! 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? So they have hardware feature that allows control of 3 LEDs at the same time, right? Actually turris routers (IIRC) have similar feature, but shared for all their LEDs. I'd suggest simply ignoring that feature for now :-). Why not use brightness file for that? We will need to solve RGB leds somehow, hopefully this is solved with it. When? With this attitude we will procrastinate it forever. It's been almost 3 years since first HSV patches. I proposed rough design of LED RGB class interface in [0]. If you find it totally flawed, then please spot the problems. If not, let's improve it and implement. [0] https://lkml.org/lkml/2019/1/3/550 -- Best regards, Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
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 @ {
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?
Thanks,
Vesa Jääskeläinen
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > 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? So they have hardware feature that allows control of 3 LEDs at the same time, right? Actually turris routers (IIRC) have similar feature, but shared for all their LEDs. I'd suggest simply ignoring that feature for now :-). We will need to solve RGB leds somehow, hopefully this is solved with it. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 1/4/19 9:12 PM, Pavel Machek wrote: Hi! I suppose if one would just make it an array of values (separated by space) and then one file with string array of color element names and on file with maximum value array it could be within those words. The it would be something like: $ echo "23 54 32" > color Go ahead, but first convince Pavel, and then Greg :-) I'd personally would not have much against, especially that the list of values will not grow for that one like in case of old patch set [0] where Pavel and Greg thwarted my similar attempt. Oh, you can get this past Pavel (and probably Greg) -- if you have a good reason. Performance is _not_ a good reason. You don't need to change LED color 5 times a second. We would need to do some profiling here to check if the problem exists. 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? If you can demonstrate a reasonable design, where user selects color from well-known RGB pallete and either kernel or library+kernel acts to set that color on the LED, we can talk. When user asks for white, it has to be white. Exact color temperature does not matter. When user asks for pink, it has to be pink. Again, exact color does not matter; different monitors display slightly different colors, too -- Best regards, Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > >And... LEDs are linear-enough as it is. That is not a problem. But RGB > >does _not_ expect linear response. That's why colors are _way_ off currently. > > Example what I was given was some LEDs are off for let's say 20% of PWM > linearity and then there is non-linear curve for PWM value vs. intensity > (this was in context of white LCD backlight). Well, we'll do non-linear curve calibration if we have to. > As such I have nothing against adding support for HSL or other color space > based brightness calculations. It might just need to be configurable what > kind of mode is being used based on hardware solution in place. HSL seem to > need a bit of fixed point math. Got floating point version working already > but that does not work with kernel space so one needs to adapt it to fixed > point. First, we need _some_ kind of solution. Then we can decide kernel vs. library. We can do floats in kernel too, if really required. > One problem here is to figure out is if user configures some color -- is it > configured with 100% brightness eg. Should one calculate RGB->HSL and then > scale L with brightness and calculate RGB back for setting color -- or does > it mean replacing L with brightness value? I don't understand the question here. > Additional problem is then if you have let's say yellow LED color element > there. How would one scale that then? Linear is of course easy. If you need > to configure this to some color space then how does one define the color in > device tree so that such color space conversion is possible? One possible > solution is to calibrate the curve (like what you do with LCD calibration) > and then just assume brightness as linear brightness value in that > case. Calibrated curves would be nice... For leds, you should be able to list the wavelength, right? > Or if you have multi-color LED with red and green but no other color > elements. How does brightness scaling work with this one? I'd ignore that for now. > >You say it is "rather slow" to change all 3 colors. How long does it > >take, and how long do you need it to take? > > Some times in embedded linux systems you can see "stalls" in operation of an > application flow eg. time is spent in different places. I believe "slowest" > CPU with embedded linux we are using is Atmel's AT91 series (ARM9) and > executing code in there can at times be a bit time consuming. > > I have seen delays like 18ms in TI's AM335x CPU series (Cortex-A8) and not > even too high load situations (eg. breaking some serial protocols because of > breaks in transmission in-between transfer without being a problem in > application code as such). It is completely different story when there is a > bit of load in system or some special situation in the kernel/OS. > > Running high priority thread for configuring LEDs to avoid problems sounds > like a wrong solution. > > Co-worker of mine tried multi-color LED brightness sliding from user space > with current PWM led driver and it was visible from eye that it was not > timed smoothly. Ok, 18msec is _way_ higher than sysfs overhead (like 400x?). If you have slow i2c or something, then you'll get fun stuff, no matter what the kernel<->user interface. > For PWM controlled multi-color LEDs we don't yet have a solution. We need > configurable color kernel based blinking. > If we could figure out acceptable solution for color transition problem then > I suppose all parties would be happy? If you can get LEDs to display the colors in similar way monitors do, that will be major step forward. Then we can design the rest, or maybe ressurect the HSV patch. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > >I suppose if one would just make it an array of values (separated by > >space) and then one file with string array of color element names and on > >file with maximum value array it could be within those words. > > > >The it would be something like: > > > >$ echo "23 54 32" > color > > Go ahead, but first convince Pavel, and then Greg :-) > > I'd personally would not have much against, especially that the > list of values will not grow for that one like in case of old patch set > [0] where Pavel and Greg thwarted my similar attempt. Oh, you can get this past Pavel (and probably Greg) -- if you have a good reason. Performance is _not_ a good reason. You don't need to change LED color 5 times a second. If you can demonstrate a reasonable design, where user selects color from well-known RGB pallete and either kernel or library+kernel acts to set that color on the LED, we can talk. When user asks for white, it has to be white. Exact color temperature does not matter. When user asks for pink, it has to be pink. Again, exact color does not matter; different monitors display slightly different colors, too. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Pavel, On 04/01/2019 1.34, Pavel Machek wrote: Hi! Regarding led_scale_color_elements() - I checked it in GIMP and the results are not satisfactory when increasing brightness. Even if we managed to fix it, the result would not be guaranteed to be the same across all devices. No and they will never be the same. I was told by our hardware expert that it is rather impossible to get linearly behaving LED control without special curve fitting trimmed for particular hardware and LED component in use. And if you go and change LED component/vendor it would need to be "calibrated" again if such accuracy would be required. Also LEDs age and that has also effect on this. Well, it is not possible to "perfectly" calibrate LCD monitors, either. Yet, color tables make sense for them. And we should aim for the same thing. And yes, it may mean re-doing calibration when vendor changes. And it will mean some math and some understanding of colors. And... LEDs are linear-enough as it is. That is not a problem. But RGB does _not_ expect linear response. That's why colors are _way_ off currently. Example what I was given was some LEDs are off for let's say 20% of PWM linearity and then there is non-linear curve for PWM value vs. intensity (this was in context of white LCD backlight). One case where we currently are interested in linear intensity is LCD background color. For that we have ramp defined in device tree. There would probably be simple fix to get that curve fitting to work better but let's keep that as another topic for now. I was thinking that if we get "calibration" curve support in PWM LED brightness we could then just use that as one solution within kernel and let LCD PWM brightness support use same functionality from LED framework. (eg. LCD backlight would bind to PWM controlled mono color LED node) Other case is that we need to dim LEDs in low light situations. As such I have nothing against adding support for HSL or other color space based brightness calculations. It might just need to be configurable what kind of mode is being used based on hardware solution in place. HSL seem to need a bit of fixed point math. Got floating point version working already but that does not work with kernel space so one needs to adapt it to fixed point. One problem here is to figure out is if user configures some color -- is it configured with 100% brightness eg. Should one calculate RGB->HSL and then scale L with brightness and calculate RGB back for setting color -- or does it mean replacing L with brightness value? Additional problem is then if you have let's say yellow LED color element there. How would one scale that then? Linear is of course easy. If you need to configure this to some color space then how does one define the color in device tree so that such color space conversion is possible? One possible solution is to calibrate the curve (like what you do with LCD calibration) and then just assume brightness as linear brightness value in that case. Or if you have multi-color LED with red and green but no other color elements. How does brightness scaling work with this one? I have another proposal, being a mix of what has been discussed so far: RGB LED class will expose following files: a) available by default: - red, green, blue Writing any of these file will result in writing corresponding device register. Problem with this is that we are basically back at square one and one cannot do "atomic" color change with this. In order to set or activate new values one would need "load values" file or such that when writing to it would activate new values. However it becomes quite clumsy interface at that point as you need to handle multiple writes to multiple files and makes those operations rather slow. If you don't like the interface, create an shared library. It may be neccessary, anyway, for the color operations. You say it is "rather slow" to change all 3 colors. How long does it take, and how long do you need it to take? Some times in embedded linux systems you can see "stalls" in operation of an application flow eg. time is spent in different places. I believe "slowest" CPU with embedded linux we are using is Atmel's AT91 series (ARM9) and executing code in there can at times be a bit time consuming. I have seen delays like 18ms in TI's AM335x CPU series (Cortex-A8) and not even too high load situations (eg. breaking some serial protocols because of breaks in transmission in-between transfer without being a problem in application code as such). It is completely different story when there is a bit of load in system or some special situation in the kernel/OS. Running high priority thread for configuring LEDs to avoid problems sounds like a wrong solution. Co-worker of mine tried multi-color LED brightness sliding from user space with current PWM led driver and it was visible from eye that it was not timed
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Vesa, On 1/4/19 12:19 AM, Vesa Jääskeläinen wrote: Hi Jacek, Comments below. On 04/01/2019 0.05, Jacek Anaszewski wrote: Hi Vesa, Thank you for sharing your ideas. Please find my comment below. On 1/1/19 2:45 PM, Vesa Jääskeläinen wrote: Hi All, On 20/12/2018 14.40, Vesa Jääskeläinen wrote: Idea was to define preset colors in device tree as an example when you are dealing with multi-color LEDs without PWM. In that case you only have GPIOs to control and then have a problem what does those GPIO's mean. With preset definitions one can use color names to act as a shortcut to configure GPIO's to proper state for that particular color. For more flexible setups where you have PWM or such control you have larger space of available colors. In this case you need to somehow define also meaning of those controls. Also we may not have LED with only red, green and blue elements. There might in example be amber, ultraviolet, white elements. This is where device tree is concerned. It helps us craft the logical definition for LED so that we can control it from user space in common way. Now the next problem then is how does user space work then. For multi-color LEDs it it important to change the color atomically so that no wrong colors are being shown as user space got interrupted when controlling it. Also we have brightness setting that would be useful for PWM controlled LEDs. Setting color is easy when you use preset names then you only need to deal with brightness value (eg. RGB -> HSV * brightness -> RGB). Of course here additional problem is other color elements are they then scaled according to brightness value?. Setting color as "raw" values is then next problem. In order to do it atomically it needs to be one atomic activation and could be eg. one write to "color" sysfs entry with combination of all color elements and perhaps additionally also brightness value. Next question is then what is the format for such entry then? What are the value ranges? In here we can utilize device tree definition to help define what kind of LED we do have and what kind of capabilities it does have. Additional problem risen also in discussion was non-linearity of some control mechanisms vs. perceived color. So there might be a need for curve mapping similarly what is with backlight control and that would be defined either in device tree and possibly in user space if there is a need for that. I suppose golden curve definition in device tree should be good enough. Then there was additional discussion about possible animation support but I would leave that for future design as that would then be utilizing the same framework. I suppose color space handling and that kind of stuff should be in some led core functionality and then raw control should be part of physical led driver. I was planning to play with it during holiday season but lets see how it goes. Feel free to also experiment with the idea. I was playing with this and got some results with PWM LED driver. I would like to get feedback now even thou it is not yet ready for patch sending. They still need more work but the idea can be seen here: https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led This branch is now based on Linux kernel 4.20 release. Consider that branch as volatile as I will forcibly update it when there are updates. From there specifically in commits (while they last): drivers: leds: Add core support for multi color element LEDs https://github.com/vesajaaskelainen/linux/commit/55d553906d0a158591435bb6323a318462079d59 WIP: drivers: leds: leds-pwm: Add multi color element LED support. https://github.com/vesajaaskelainen/linux/commit/efccef08cbf3b2e1e49b95b69ff81cd380519fe3 What is there now: - led-core supports color elements - led-class supports users space configuration - both led-core and led-class are driver agnostic so they should be treated as generic code. - leds-pwm: my testing code with PWM led. - no HSV support for brightness as there could be multiple color elements out from traditional red-green-blue space or odd combinations of colors and they are a bit hard to map to HSV formula (and it needs fixed point math). - no color presets that could be optionally be selected - when I configure led trigger to heartbeat it actually blinks with color specified -- thou trigger gets zeroed out with one sets new color or brightness as that was previous functionality with brightness. - some documentation added - code should pass checkpatch What I was planning to do next: - cleanup PWM LED driver so that it works with and without LED_MULTI_COLOR_LED being defined. - improve documentation - try out how my other device behaves which have dual color element LED controlled with GPIO's and see how it would integrate to gpio-led driver. I would like to get feedback on: - Device tree idea - Internal logic - Should the trigger be really reseted when one
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > >Regarding led_scale_color_elements() - I checked it in GIMP and > >the results are not satisfactory when increasing brightness. > >Even if we managed to fix it, the result would not be guaranteed > >to be the same across all devices. > > No and they will never be the same. I was told by our hardware expert that > it is rather impossible to get linearly behaving LED control without special > curve fitting trimmed for particular hardware and LED component in use. And > if you go and change LED component/vendor it would need to be "calibrated" > again if such accuracy would be required. Also LEDs age and that has also > effect on this. Well, it is not possible to "perfectly" calibrate LCD monitors, either. Yet, color tables make sense for them. And we should aim for the same thing. And yes, it may mean re-doing calibration when vendor changes. And it will mean some math and some understanding of colors. And... LEDs are linear-enough as it is. That is not a problem. But RGB does _not_ expect linear response. That's why colors are _way_ off currently. > >I have another proposal, being a mix of what has been discussed so far: > > > > RGB LED class will expose following files: > > a) available by default: > > - red, green, blue > > Writing any of these file will result in writing corresponding > > device register. > > Problem with this is that we are basically back at square one and one cannot > do "atomic" color change with this. > > In order to set or activate new values one would need "load values" file or > such that when writing to it would activate new values. However it becomes > quite clumsy interface at that point as you need to handle multiple writes > to multiple files and makes those operations rather slow. If you don't like the interface, create an shared library. It may be neccessary, anyway, for the color operations. You say it is "rather slow" to change all 3 colors. How long does it take, and how long do you need it to take? > Then we have color presets left that could kinda solve the issue on setting > the color to fixed values atomically. Lets not design crazy interface "because sysfs writing is too slow". Hint: it is not. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Jacek, Comments below. On 04/01/2019 0.05, Jacek Anaszewski wrote: Hi Vesa, Thank you for sharing your ideas. Please find my comment below. On 1/1/19 2:45 PM, Vesa Jääskeläinen wrote: Hi All, On 20/12/2018 14.40, Vesa Jääskeläinen wrote: Idea was to define preset colors in device tree as an example when you are dealing with multi-color LEDs without PWM. In that case you only have GPIOs to control and then have a problem what does those GPIO's mean. With preset definitions one can use color names to act as a shortcut to configure GPIO's to proper state for that particular color. For more flexible setups where you have PWM or such control you have larger space of available colors. In this case you need to somehow define also meaning of those controls. Also we may not have LED with only red, green and blue elements. There might in example be amber, ultraviolet, white elements. This is where device tree is concerned. It helps us craft the logical definition for LED so that we can control it from user space in common way. Now the next problem then is how does user space work then. For multi-color LEDs it it important to change the color atomically so that no wrong colors are being shown as user space got interrupted when controlling it. Also we have brightness setting that would be useful for PWM controlled LEDs. Setting color is easy when you use preset names then you only need to deal with brightness value (eg. RGB -> HSV * brightness -> RGB). Of course here additional problem is other color elements are they then scaled according to brightness value?. Setting color as "raw" values is then next problem. In order to do it atomically it needs to be one atomic activation and could be eg. one write to "color" sysfs entry with combination of all color elements and perhaps additionally also brightness value. Next question is then what is the format for such entry then? What are the value ranges? In here we can utilize device tree definition to help define what kind of LED we do have and what kind of capabilities it does have. Additional problem risen also in discussion was non-linearity of some control mechanisms vs. perceived color. So there might be a need for curve mapping similarly what is with backlight control and that would be defined either in device tree and possibly in user space if there is a need for that. I suppose golden curve definition in device tree should be good enough. Then there was additional discussion about possible animation support but I would leave that for future design as that would then be utilizing the same framework. I suppose color space handling and that kind of stuff should be in some led core functionality and then raw control should be part of physical led driver. I was planning to play with it during holiday season but lets see how it goes. Feel free to also experiment with the idea. I was playing with this and got some results with PWM LED driver. I would like to get feedback now even thou it is not yet ready for patch sending. They still need more work but the idea can be seen here: https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led This branch is now based on Linux kernel 4.20 release. Consider that branch as volatile as I will forcibly update it when there are updates. From there specifically in commits (while they last): drivers: leds: Add core support for multi color element LEDs https://github.com/vesajaaskelainen/linux/commit/55d553906d0a158591435bb6323a318462079d59 WIP: drivers: leds: leds-pwm: Add multi color element LED support. https://github.com/vesajaaskelainen/linux/commit/efccef08cbf3b2e1e49b95b69ff81cd380519fe3 What is there now: - led-core supports color elements - led-class supports users space configuration - both led-core and led-class are driver agnostic so they should be treated as generic code. - leds-pwm: my testing code with PWM led. - no HSV support for brightness as there could be multiple color elements out from traditional red-green-blue space or odd combinations of colors and they are a bit hard to map to HSV formula (and it needs fixed point math). - no color presets that could be optionally be selected - when I configure led trigger to heartbeat it actually blinks with color specified -- thou trigger gets zeroed out with one sets new color or brightness as that was previous functionality with brightness. - some documentation added - code should pass checkpatch What I was planning to do next: - cleanup PWM LED driver so that it works with and without LED_MULTI_COLOR_LED being defined. - improve documentation - try out how my other device behaves which have dual color element LED controlled with GPIO's and see how it would integrate to gpio-led driver. I would like to get feedback on: - Device tree idea - Internal logic - Should the trigger be really reseted when one changes value of brightness? I would think it should
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Vesa, Thank you for sharing your ideas. Please find my comment below. On 1/1/19 2:45 PM, Vesa Jääskeläinen wrote: Hi All, On 20/12/2018 14.40, Vesa Jääskeläinen wrote: Idea was to define preset colors in device tree as an example when you are dealing with multi-color LEDs without PWM. In that case you only have GPIOs to control and then have a problem what does those GPIO's mean. With preset definitions one can use color names to act as a shortcut to configure GPIO's to proper state for that particular color. For more flexible setups where you have PWM or such control you have larger space of available colors. In this case you need to somehow define also meaning of those controls. Also we may not have LED with only red, green and blue elements. There might in example be amber, ultraviolet, white elements. This is where device tree is concerned. It helps us craft the logical definition for LED so that we can control it from user space in common way. Now the next problem then is how does user space work then. For multi-color LEDs it it important to change the color atomically so that no wrong colors are being shown as user space got interrupted when controlling it. Also we have brightness setting that would be useful for PWM controlled LEDs. Setting color is easy when you use preset names then you only need to deal with brightness value (eg. RGB -> HSV * brightness -> RGB). Of course here additional problem is other color elements are they then scaled according to brightness value?. Setting color as "raw" values is then next problem. In order to do it atomically it needs to be one atomic activation and could be eg. one write to "color" sysfs entry with combination of all color elements and perhaps additionally also brightness value. Next question is then what is the format for such entry then? What are the value ranges? In here we can utilize device tree definition to help define what kind of LED we do have and what kind of capabilities it does have. Additional problem risen also in discussion was non-linearity of some control mechanisms vs. perceived color. So there might be a need for curve mapping similarly what is with backlight control and that would be defined either in device tree and possibly in user space if there is a need for that. I suppose golden curve definition in device tree should be good enough. Then there was additional discussion about possible animation support but I would leave that for future design as that would then be utilizing the same framework. I suppose color space handling and that kind of stuff should be in some led core functionality and then raw control should be part of physical led driver. I was planning to play with it during holiday season but lets see how it goes. Feel free to also experiment with the idea. I was playing with this and got some results with PWM LED driver. I would like to get feedback now even thou it is not yet ready for patch sending. They still need more work but the idea can be seen here: https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led This branch is now based on Linux kernel 4.20 release. Consider that branch as volatile as I will forcibly update it when there are updates. From there specifically in commits (while they last): drivers: leds: Add core support for multi color element LEDs https://github.com/vesajaaskelainen/linux/commit/55d553906d0a158591435bb6323a318462079d59 WIP: drivers: leds: leds-pwm: Add multi color element LED support. https://github.com/vesajaaskelainen/linux/commit/efccef08cbf3b2e1e49b95b69ff81cd380519fe3 What is there now: - led-core supports color elements - led-class supports users space configuration - both led-core and led-class are driver agnostic so they should be treated as generic code. - leds-pwm: my testing code with PWM led. - no HSV support for brightness as there could be multiple color elements out from traditional red-green-blue space or odd combinations of colors and they are a bit hard to map to HSV formula (and it needs fixed point math). - no color presets that could be optionally be selected - when I configure led trigger to heartbeat it actually blinks with color specified -- thou trigger gets zeroed out with one sets new color or brightness as that was previous functionality with brightness. - some documentation added - code should pass checkpatch What I was planning to do next: - cleanup PWM LED driver so that it works with and without LED_MULTI_COLOR_LED being defined. - improve documentation - try out how my other device behaves which have dual color element LED controlled with GPIO's and see how it would integrate to gpio-led driver. I would like to get feedback on: - Device tree idea - Internal logic - Should the trigger be really reseted when one changes value of brightness? I would think it should function like setting brightness entry from sysfs would set current brightness
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 1/1/19 7:11 PM, Dan Murphy wrote: Jacek Thanks for the reply! All Happy New Year! Happy New Year! On 1/1/19 8:42 AM, Jacek Anaszewski wrote: On 12/31/18 8:15 PM, Dan Murphy wrote: On 12/31/18 9:47 AM, Jacek Anaszewski wrote: On 12/31/18 4:43 PM, Jacek Anaszewski wrote: On 12/30/18 6:35 PM, Pavel Machek wrote: On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: On 12/29/18 8:07 PM, Pavel Machek wrote: Hi! With the "color" sysfs file it will make more sense to allow for user defined color palettes. I think defining these values in the device tree or acpi severely limits the devices capabilities. Especially in development phases. If the knobs were exposed then the user space can create new experiences. The color definition should be an absolute color defined in the dt and either the framework or user space needs to mix these appropriately. IMO user space should set the policy of the user experience and the dt/acpi needs to set the capabilities. I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. Maybe the framework could take these groups and combine/group them into a single node with the groups colors. There is still HSV approach [0] in store. One problem with proposed implementation is fixed algorithm of RGB <-> HSV color space conversion. Maybe allowing for some board specific adjustments in DT would add more flexibility. [0] https://lkml.org/lkml/2017/8/31/255 Yes we could do HSV. Problem is that that we do not really have RGB available. We do have integers for red, green and blue, but they do not correspond to RGB colorspace. OK, so conversion from HSV to RGB would only increase the aberration. So, let's stick to RGB - we've got to have some stable ground and this is something that the hardware at least pretends to be compliant with. I'm not saying that we should stick to RGB. I'm just saying that problem is complex. And no, hardware does not even pretend to be compliant with RGB color model ( https://en.wikipedia.org/wiki/RGB_color_model ). In particular, in RGB there is non-linear brightness curve. Quotation from the wiki page you referred to: "RGB is a device-dependent color model: different devices detect or reproduce a given RGB value differently, since the color elements (such as phosphors or dyes) and their response to the individual R, G, and B levels vary from manufacturer to manufacturer, or even in the same device over time. Thus an RGB value does not define the same color across devices without some kind of color management" This claim alone leaves much room for the manufacturers to pretend that their devices are compliant with RGB model. And the documentation of the hardware the discussed driver is for also refers to RGB model in many places - e.g. see Table 1, page 15 in the document [0], where mapping of output triplets to an RGB module is shown. One thing that I missed is that the discussed hardware provides LEDn_BRIGHTNESS registers for each RGB LED module, that can be configured to set color intensity in linear or logarithmic fashion. Actually this stands in contradiction with RGB model, since change of "color intensity" means change of all RGB components. We could use brightness file as for monochrome LEDs for that, Here I mean brightness file in addition to the previously proposed red, green and blue files. but we'd need to come up with consistent interface semantics for all devices, also those which don't have corresponding functionality. Probably this is the place where we could apply some RGB<->HSV conversion, as color intensity feels something more of HSV's saturation and value. It would be good to hear from Dan how that looks in reality in case of lp5024 device. Sorry for the non-response I have had a passing in my family and have not been at my computer for some time. Sorry to hear that. In fact there was no delay, since I wrote that yesterday. I am not seeing how HSV will fit into this device. Not sure what the V is in HSV. I meant RGB<->HSV conversion as a fallback for RGB LED controllers that don't have the functionality similar to LEDn_BRIGHTNESS. For lp5024 we'd use just what hardware offers. You can get the flavor of the relationship between RGB and HSV using e.g. GIMP color editor. After that you could share a feedback if changing LEDn_BRIGHTNESS feels like changing saturation and value of HSV. Remember that we're still talking about generic approach to the problem. I am still not a fan of defining colors in the kernel. I think the user space needs to do this based on information it is given. When I look at Android the user space sets all the policies of the hardware the kernel just provides the vehicle to hardware. I think defining any set colors in the kernel for devices that have a full color spectrum palette is very restricting. The kernel should indicate the absolute colors available and not the colors that are allowed. So in this
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Jacek Thanks for the reply! All Happy New Year! On 1/1/19 8:42 AM, Jacek Anaszewski wrote: > On 12/31/18 8:15 PM, Dan Murphy wrote: >> On 12/31/18 9:47 AM, Jacek Anaszewski wrote: >>> On 12/31/18 4:43 PM, Jacek Anaszewski wrote: On 12/30/18 6:35 PM, Pavel Machek wrote: > On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: >> On 12/29/18 8:07 PM, Pavel Machek wrote: >>> Hi! >>> >> With the "color" sysfs file it will make more sense to allow for user >> defined color palettes. >> > > I think defining these values in the device tree or acpi severely > limits the devices > capabilities. Especially in development phases. If the knobs were > exposed then the user space > can create new experiences. The color definition should be an > absolute color defined in the dt and > either the framework or user space needs to mix these appropriately. > IMO user space should set the policy > of the user experience and the dt/acpi needs to set the capabilities. > > I do like Pavels idea on defining the more standard binding pattern > to "group" leds into a single group. > > Maybe the framework could take these groups and combine/group them > into a single node with the groups colors. There is still HSV approach [0] in store. One problem with proposed implementation is fixed algorithm of RGB <-> HSV color space conversion. Maybe allowing for some board specific adjustments in DT would add more flexibility. [0] https://lkml.org/lkml/2017/8/31/255 >>> >>> Yes we could do HSV. Problem is that that we do not really have RGB >>> available. We do have integers for red, green and blue, but they do >>> not correspond to RGB colorspace. >> >> OK, so conversion from HSV to RGB would only increase the aberration. >> So, let's stick to RGB - we've got to have some stable ground and this >> is something that the hardware at least pretends to be compliant >> with. > > I'm not saying that we should stick to RGB. I'm just saying that > problem is complex. > > And no, hardware does not even pretend to be compliant with RGB color > model ( https://en.wikipedia.org/wiki/RGB_color_model ). In > particular, in RGB there is non-linear brightness curve. Quotation from the wiki page you referred to: "RGB is a device-dependent color model: different devices detect or reproduce a given RGB value differently, since the color elements (such as phosphors or dyes) and their response to the individual R, G, and B levels vary from manufacturer to manufacturer, or even in the same device over time. Thus an RGB value does not define the same color across devices without some kind of color management" This claim alone leaves much room for the manufacturers to pretend that their devices are compliant with RGB model. And the documentation of the hardware the discussed driver is for also refers to RGB model in many places - e.g. see Table 1, page 15 in the document [0], where mapping of output triplets to an RGB module is shown. One thing that I missed is that the discussed hardware provides LEDn_BRIGHTNESS registers for each RGB LED module, that can be configured to set color intensity in linear or logarithmic fashion. Actually this stands in contradiction with RGB model, since change of "color intensity" means change of all RGB components. We could use brightness file as for monochrome LEDs for that, >>> >>> Here I mean brightness file in addition to the previously proposed >>> red, green and blue files. >>> but we'd need to come up with consistent interface semantics for all devices, also those which don't have corresponding functionality. Probably this is the place where we could apply some RGB<->HSV conversion, as color intensity feels something more of HSV's saturation and value. It would be good to hear from Dan how that looks in reality in case of lp5024 device. >> >> Sorry for the non-response I have had a passing in my family and have not >> been at my >> computer for some time. > > Sorry to hear that. In fact there was no delay, since I wrote that > yesterday. > >> I am not seeing how HSV will fit into this device. Not sure what the V is >> in HSV. > I meant RGB<->HSV conversion as a fallback for RGB LED > controllers that don't have the functionality similar to > LEDn_BRIGHTNESS. For lp5024 we'd use just what hardware offers. > > You can get the flavor of the relationship between RGB and HSV using > e.g. GIMP color editor. After that you could share a feedback if > changing LEDn_BRIGHTNESS feels like changing saturation and value of > HSV. > > Remember that
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/31/18 8:15 PM, Dan Murphy wrote: On 12/31/18 9:47 AM, Jacek Anaszewski wrote: On 12/31/18 4:43 PM, Jacek Anaszewski wrote: On 12/30/18 6:35 PM, Pavel Machek wrote: On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: On 12/29/18 8:07 PM, Pavel Machek wrote: Hi! With the "color" sysfs file it will make more sense to allow for user defined color palettes. I think defining these values in the device tree or acpi severely limits the devices capabilities. Especially in development phases. If the knobs were exposed then the user space can create new experiences. The color definition should be an absolute color defined in the dt and either the framework or user space needs to mix these appropriately. IMO user space should set the policy of the user experience and the dt/acpi needs to set the capabilities. I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. Maybe the framework could take these groups and combine/group them into a single node with the groups colors. There is still HSV approach [0] in store. One problem with proposed implementation is fixed algorithm of RGB <-> HSV color space conversion. Maybe allowing for some board specific adjustments in DT would add more flexibility. [0] https://lkml.org/lkml/2017/8/31/255 Yes we could do HSV. Problem is that that we do not really have RGB available. We do have integers for red, green and blue, but they do not correspond to RGB colorspace. OK, so conversion from HSV to RGB would only increase the aberration. So, let's stick to RGB - we've got to have some stable ground and this is something that the hardware at least pretends to be compliant with. I'm not saying that we should stick to RGB. I'm just saying that problem is complex. And no, hardware does not even pretend to be compliant with RGB color model ( https://en.wikipedia.org/wiki/RGB_color_model ). In particular, in RGB there is non-linear brightness curve. Quotation from the wiki page you referred to: "RGB is a device-dependent color model: different devices detect or reproduce a given RGB value differently, since the color elements (such as phosphors or dyes) and their response to the individual R, G, and B levels vary from manufacturer to manufacturer, or even in the same device over time. Thus an RGB value does not define the same color across devices without some kind of color management" This claim alone leaves much room for the manufacturers to pretend that their devices are compliant with RGB model. And the documentation of the hardware the discussed driver is for also refers to RGB model in many places - e.g. see Table 1, page 15 in the document [0], where mapping of output triplets to an RGB module is shown. One thing that I missed is that the discussed hardware provides LEDn_BRIGHTNESS registers for each RGB LED module, that can be configured to set color intensity in linear or logarithmic fashion. Actually this stands in contradiction with RGB model, since change of "color intensity" means change of all RGB components. We could use brightness file as for monochrome LEDs for that, Here I mean brightness file in addition to the previously proposed red, green and blue files. but we'd need to come up with consistent interface semantics for all devices, also those which don't have corresponding functionality. Probably this is the place where we could apply some RGB<->HSV conversion, as color intensity feels something more of HSV's saturation and value. It would be good to hear from Dan how that looks in reality in case of lp5024 device. Sorry for the non-response I have had a passing in my family and have not been at my computer for some time. Sorry to hear that. In fact there was no delay, since I wrote that yesterday. I am not seeing how HSV will fit into this device. Not sure what the V is in HSV. I meant RGB<->HSV conversion as a fallback for RGB LED controllers that don't have the functionality similar to LEDn_BRIGHTNESS. For lp5024 we'd use just what hardware offers. You can get the flavor of the relationship between RGB and HSV using e.g. GIMP color editor. After that you could share a feedback if changing LEDn_BRIGHTNESS feels like changing saturation and value of HSV. Remember that we're still talking about generic approach to the problem. I am still not a fan of defining colors in the kernel. I think the user space needs to do this based on information it is given. When I look at Android the user space sets all the policies of the hardware the kernel just provides the vehicle to hardware. I think defining any set colors in the kernel for devices that have a full color spectrum palette is very restricting. The kernel should indicate the absolute colors available and not the colors that are allowed. So in this case we indicate that a Red, green and blue LED are available or that the palette is variable. Or in the case of a white LED driver we just say white.
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Pavel, On 12/31/18 5:28 PM, Pavel Machek wrote: Hi! There is still HSV approach [0] in store. One problem with proposed implementation is fixed algorithm of RGB <-> HSV color space conversion. Maybe allowing for some board specific adjustments in DT would add more flexibility. [0] https://lkml.org/lkml/2017/8/31/255 Yes we could do HSV. Problem is that that we do not really have RGB available. We do have integers for red, green and blue, but they do not correspond to RGB colorspace. OK, so conversion from HSV to RGB would only increase the aberration. So, let's stick to RGB - we've got to have some stable ground and this is something that the hardware at least pretends to be compliant with. I'm not saying that we should stick to RGB. I'm just saying that problem is complex. And no, hardware does not even pretend to be compliant with RGB color model ( https://en.wikipedia.org/wiki/RGB_color_model ). In particular, in RGB there is non-linear brightness curve. Quotation from the wiki page you referred to: "RGB is a device-dependent color model: different devices detect or reproduce a given RGB value differently, since the color elements (such as phosphors or dyes) and their response to the individual R, G, and B levels vary from manufacturer to manufacturer, or even in the same device over time. Thus an RGB value does not define the same color across devices without some kind of color management" This claim alone leaves much room for the manufacturers to pretend that their devices are compliant with RGB model. Much room, but everyone agrees that R=G=B=255 should be some kind of white, and what other colors "approximately" mean. I have two monitors, and no, colors don't match. Do you have access to RGB led? Try to compare two monitors, and then RGB led with monitor. RGB led will be _way_ off. This chart makes sense: https://www.rapidtables.com/web/color/RGB_Color.html Try it on your LED device... I believe problem to start with is the "white" problem. Setting R=G=B=255 will _not_ result in anything close to white light on hardware I have. RGBW LED controllers solve this problem. For the devices without white/amber we cannot do more than the hardware allows for. But the hardware can do some kind of white. It is just that R=G=B=255 will result in green-ish-blue and something like R=255, G=50, B=10 is neccessary to get approximation of white. IMO good start would be to specify what kind of intensities are neccessary to approximate white. And then try converting from RGB to led intensities, and see if it somehow works. I don't have access to RGB LED, and unfortunately have lack of time currently to play with it even if I had one. In order to gain a full understanding of your idea of RGB to LED intensity conversion, we'd need to see the full algorithm. It feels like imposing some restrictions on user regarding the available scope of colors to set. -- Best regards, Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi All, On 20/12/2018 14.40, Vesa Jääskeläinen wrote: Idea was to define preset colors in device tree as an example when you are dealing with multi-color LEDs without PWM. In that case you only have GPIOs to control and then have a problem what does those GPIO's mean. With preset definitions one can use color names to act as a shortcut to configure GPIO's to proper state for that particular color. For more flexible setups where you have PWM or such control you have larger space of available colors. In this case you need to somehow define also meaning of those controls. Also we may not have LED with only red, green and blue elements. There might in example be amber, ultraviolet, white elements. This is where device tree is concerned. It helps us craft the logical definition for LED so that we can control it from user space in common way. Now the next problem then is how does user space work then. For multi-color LEDs it it important to change the color atomically so that no wrong colors are being shown as user space got interrupted when controlling it. Also we have brightness setting that would be useful for PWM controlled LEDs. Setting color is easy when you use preset names then you only need to deal with brightness value (eg. RGB -> HSV * brightness -> RGB). Of course here additional problem is other color elements are they then scaled according to brightness value?. Setting color as "raw" values is then next problem. In order to do it atomically it needs to be one atomic activation and could be eg. one write to "color" sysfs entry with combination of all color elements and perhaps additionally also brightness value. Next question is then what is the format for such entry then? What are the value ranges? In here we can utilize device tree definition to help define what kind of LED we do have and what kind of capabilities it does have. Additional problem risen also in discussion was non-linearity of some control mechanisms vs. perceived color. So there might be a need for curve mapping similarly what is with backlight control and that would be defined either in device tree and possibly in user space if there is a need for that. I suppose golden curve definition in device tree should be good enough. Then there was additional discussion about possible animation support but I would leave that for future design as that would then be utilizing the same framework. I suppose color space handling and that kind of stuff should be in some led core functionality and then raw control should be part of physical led driver. I was planning to play with it during holiday season but lets see how it goes. Feel free to also experiment with the idea. I was playing with this and got some results with PWM LED driver. I would like to get feedback now even thou it is not yet ready for patch sending. They still need more work but the idea can be seen here: https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led This branch is now based on Linux kernel 4.20 release. Consider that branch as volatile as I will forcibly update it when there are updates. From there specifically in commits (while they last): drivers: leds: Add core support for multi color element LEDs https://github.com/vesajaaskelainen/linux/commit/55d553906d0a158591435bb6323a318462079d59 WIP: drivers: leds: leds-pwm: Add multi color element LED support. https://github.com/vesajaaskelainen/linux/commit/efccef08cbf3b2e1e49b95b69ff81cd380519fe3 What is there now: - led-core supports color elements - led-class supports users space configuration - both led-core and led-class are driver agnostic so they should be treated as generic code. - leds-pwm: my testing code with PWM led. - no HSV support for brightness as there could be multiple color elements out from traditional red-green-blue space or odd combinations of colors and they are a bit hard to map to HSV formula (and it needs fixed point math). - no color presets that could be optionally be selected - when I configure led trigger to heartbeat it actually blinks with color specified -- thou trigger gets zeroed out with one sets new color or brightness as that was previous functionality with brightness. - some documentation added - code should pass checkpatch What I was planning to do next: - cleanup PWM LED driver so that it works with and without LED_MULTI_COLOR_LED being defined. - improve documentation - try out how my other device behaves which have dual color element LED controlled with GPIO's and see how it would integrate to gpio-led driver. I would like to get feedback on: - Device tree idea - Internal logic - Should the trigger be really reseted when one changes value of brightness? I would think it should function like setting brightness entry from sysfs would set current brightness for trigger when it is lit. Setting color should change color and brightness and it should be active from there one until
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/31/18 9:47 AM, Jacek Anaszewski wrote: > On 12/31/18 4:43 PM, Jacek Anaszewski wrote: >> On 12/30/18 6:35 PM, Pavel Machek wrote: >>> On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: On 12/29/18 8:07 PM, Pavel Machek wrote: > Hi! > With the "color" sysfs file it will make more sense to allow for user defined color palettes. >>> >>> I think defining these values in the device tree or acpi severely >>> limits the devices >>> capabilities. Especially in development phases. If the knobs were >>> exposed then the user space >>> can create new experiences. The color definition should be an absolute >>> color defined in the dt and >>> either the framework or user space needs to mix these appropriately. >>> IMO user space should set the policy >>> of the user experience and the dt/acpi needs to set the capabilities. >>> >>> I do like Pavels idea on defining the more standard binding pattern to >>> "group" leds into a single group. >>> >>> Maybe the framework could take these groups and combine/group them into >>> a single node with the groups colors. >> >> There is still HSV approach [0] in store. One problem with proposed >> implementation is fixed algorithm of RGB <-> HSV color space conversion. >> Maybe allowing for some board specific adjustments in DT would add >> more flexibility. >> >> [0] https://lkml.org/lkml/2017/8/31/255 > > Yes we could do HSV. Problem is that that we do not really have RGB > available. We do have integers for red, green and blue, but they do > not correspond to RGB colorspace. OK, so conversion from HSV to RGB would only increase the aberration. So, let's stick to RGB - we've got to have some stable ground and this is something that the hardware at least pretends to be compliant with. >>> >>> I'm not saying that we should stick to RGB. I'm just saying that >>> problem is complex. >>> >>> And no, hardware does not even pretend to be compliant with RGB color >>> model ( https://en.wikipedia.org/wiki/RGB_color_model ). In >>> particular, in RGB there is non-linear brightness curve. >> >> Quotation from the wiki page you referred to: >> >> "RGB is a device-dependent color model: different devices detect or >> reproduce a given RGB value differently, since the color elements (such >> as phosphors or dyes) and their response to the individual R, G, and B >> levels vary from manufacturer to manufacturer, or even in the same >> device over time. Thus an RGB value does not define the same color >> across devices without some kind of color management" >> >> This claim alone leaves much room for the manufacturers to pretend that >> their devices are compliant with RGB model. >> >> And the documentation of the hardware the discussed driver is for >> also refers to RGB model in many places - e.g. see Table 1, page 15 >> in the document [0], where mapping of output triplets to an RGB module >> is shown. >> >> One thing that I missed is that the discussed hardware provides >> LEDn_BRIGHTNESS registers for each RGB LED module, that can be >> configured to set color intensity in linear or logarithmic fashion. >> >> Actually this stands in contradiction with RGB model, since >> change of "color intensity" means change of all RGB components. >> >> We could use brightness file as for monochrome LEDs for that, > > Here I mean brightness file in addition to the previously proposed > red, green and blue files. > >> but we'd need to come up with consistent interface semantics >> for all devices, also those which don't have corresponding >> functionality. Probably this is the place where we could apply >> some RGB<->HSV conversion, as color intensity feels something >> more of HSV's saturation and value. >> >> It would be good to hear from Dan how that looks in reality >> in case of lp5024 device. Sorry for the non-response I have had a passing in my family and have not been at my computer for some time. I am not seeing how HSV will fit into this device. Not sure what the V is in HSV. I am still not a fan of defining colors in the kernel. I think the user space needs to do this based on information it is given. When I look at Android the user space sets all the policies of the hardware the kernel just provides the vehicle to hardware. I think defining any set colors in the kernel for devices that have a full color spectrum palette is very restricting. The kernel should indicate the absolute colors available and not the colors that are allowed. So in this case we indicate that a Red, green and blue LED are available or that the palette is variable. Or in the case of a white LED driver we just say white. In the case of this device there are RGB outputs that are grouped in clusters and controlled by the LEDn_BRIGHTNESS register. This is what the brightness file is mapped to. Within that cluster the
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > There is still HSV approach [0] in store. One problem with proposed > implementation is fixed algorithm of RGB <-> HSV color space conversion. > Maybe allowing for some board specific adjustments in DT would add > more flexibility. > > [0] https://lkml.org/lkml/2017/8/31/255 > >>> > >>>Yes we could do HSV. Problem is that that we do not really have RGB > >>>available. We do have integers for red, green and blue, but they do > >>>not correspond to RGB colorspace. > >> > >>OK, so conversion from HSV to RGB would only increase the aberration. > >>So, let's stick to RGB - we've got to have some stable ground and this > >>is something that the hardware at least pretends to be compliant > >>with. > > > >I'm not saying that we should stick to RGB. I'm just saying that > >problem is complex. > > > >And no, hardware does not even pretend to be compliant with RGB color > >model ( https://en.wikipedia.org/wiki/RGB_color_model ). In > >particular, in RGB there is non-linear brightness curve. > > Quotation from the wiki page you referred to: > > "RGB is a device-dependent color model: different devices detect or > reproduce a given RGB value differently, since the color elements (such > as phosphors or dyes) and their response to the individual R, G, and B > levels vary from manufacturer to manufacturer, or even in the same > device over time. Thus an RGB value does not define the same color > across devices without some kind of color management" > > This claim alone leaves much room for the manufacturers to pretend that > their devices are compliant with RGB model. Much room, but everyone agrees that R=G=B=255 should be some kind of white, and what other colors "approximately" mean. I have two monitors, and no, colors don't match. Do you have access to RGB led? Try to compare two monitors, and then RGB led with monitor. RGB led will be _way_ off. This chart makes sense: https://www.rapidtables.com/web/color/RGB_Color.html Try it on your LED device... > >I believe problem to start with is the "white" problem. Setting > >R=G=B=255 will _not_ result in anything close to white light on > >hardware I have. > > RGBW LED controllers solve this problem. For the devices without > white/amber we cannot do more than the hardware allows for. But the hardware can do some kind of white. It is just that R=G=B=255 will result in green-ish-blue and something like R=255, G=50, B=10 is neccessary to get approximation of white. IMO good start would be to specify what kind of intensities are neccessary to approximate white. And then try converting from RGB to led intensities, and see if it somehow works. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/31/18 4:43 PM, Jacek Anaszewski wrote: On 12/30/18 6:35 PM, Pavel Machek wrote: On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: On 12/29/18 8:07 PM, Pavel Machek wrote: Hi! With the "color" sysfs file it will make more sense to allow for user defined color palettes. I think defining these values in the device tree or acpi severely limits the devices capabilities. Especially in development phases. If the knobs were exposed then the user space can create new experiences. The color definition should be an absolute color defined in the dt and either the framework or user space needs to mix these appropriately. IMO user space should set the policy of the user experience and the dt/acpi needs to set the capabilities. I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. Maybe the framework could take these groups and combine/group them into a single node with the groups colors. There is still HSV approach [0] in store. One problem with proposed implementation is fixed algorithm of RGB <-> HSV color space conversion. Maybe allowing for some board specific adjustments in DT would add more flexibility. [0] https://lkml.org/lkml/2017/8/31/255 Yes we could do HSV. Problem is that that we do not really have RGB available. We do have integers for red, green and blue, but they do not correspond to RGB colorspace. OK, so conversion from HSV to RGB would only increase the aberration. So, let's stick to RGB - we've got to have some stable ground and this is something that the hardware at least pretends to be compliant with. I'm not saying that we should stick to RGB. I'm just saying that problem is complex. And no, hardware does not even pretend to be compliant with RGB color model ( https://en.wikipedia.org/wiki/RGB_color_model ). In particular, in RGB there is non-linear brightness curve. Quotation from the wiki page you referred to: "RGB is a device-dependent color model: different devices detect or reproduce a given RGB value differently, since the color elements (such as phosphors or dyes) and their response to the individual R, G, and B levels vary from manufacturer to manufacturer, or even in the same device over time. Thus an RGB value does not define the same color across devices without some kind of color management" This claim alone leaves much room for the manufacturers to pretend that their devices are compliant with RGB model. And the documentation of the hardware the discussed driver is for also refers to RGB model in many places - e.g. see Table 1, page 15 in the document [0], where mapping of output triplets to an RGB module is shown. One thing that I missed is that the discussed hardware provides LEDn_BRIGHTNESS registers for each RGB LED module, that can be configured to set color intensity in linear or logarithmic fashion. Actually this stands in contradiction with RGB model, since change of "color intensity" means change of all RGB components. We could use brightness file as for monochrome LEDs for that, Here I mean brightness file in addition to the previously proposed red, green and blue files. but we'd need to come up with consistent interface semantics for all devices, also those which don't have corresponding functionality. Probably this is the place where we could apply some RGB<->HSV conversion, as color intensity feels something more of HSV's saturation and value. It would be good to hear from Dan how that looks in reality in case of lp5024 device. Our problem is how to set the color atomically. With HSV approach we were to obviate the problem by mapping brightness file to the "V" component of that color space, and write all H,S and V values to the hardware only on write to brightness file. I'm not sure how realistic the "atomic color" problem is. Computers are way faster than human vision. With LEDn_BRIGHTNESS registers of lp5024 it seems that we need the ability for grouping LEDs in triplets and be able to set their intensity with a single write operation. I believe problem to start with is the "white" problem. Setting R=G=B=255 will _not_ result in anything close to white light on hardware I have. RGBW LED controllers solve this problem. For the devices without white/amber we cannot do more than the hardware allows for. [0] http://www.ti.com/lit/ds/symlink/lp5024.pdf -- Best regards, Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/30/18 6:35 PM, Pavel Machek wrote: On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: On 12/29/18 8:07 PM, Pavel Machek wrote: Hi! With the "color" sysfs file it will make more sense to allow for user defined color palettes. I think defining these values in the device tree or acpi severely limits the devices capabilities. Especially in development phases. If the knobs were exposed then the user space can create new experiences. The color definition should be an absolute color defined in the dt and either the framework or user space needs to mix these appropriately. IMO user space should set the policy of the user experience and the dt/acpi needs to set the capabilities. I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. Maybe the framework could take these groups and combine/group them into a single node with the groups colors. There is still HSV approach [0] in store. One problem with proposed implementation is fixed algorithm of RGB <-> HSV color space conversion. Maybe allowing for some board specific adjustments in DT would add more flexibility. [0] https://lkml.org/lkml/2017/8/31/255 Yes we could do HSV. Problem is that that we do not really have RGB available. We do have integers for red, green and blue, but they do not correspond to RGB colorspace. OK, so conversion from HSV to RGB would only increase the aberration. So, let's stick to RGB - we've got to have some stable ground and this is something that the hardware at least pretends to be compliant with. I'm not saying that we should stick to RGB. I'm just saying that problem is complex. And no, hardware does not even pretend to be compliant with RGB color model ( https://en.wikipedia.org/wiki/RGB_color_model ). In particular, in RGB there is non-linear brightness curve. Quotation from the wiki page you referred to: "RGB is a device-dependent color model: different devices detect or reproduce a given RGB value differently, since the color elements (such as phosphors or dyes) and their response to the individual R, G, and B levels vary from manufacturer to manufacturer, or even in the same device over time. Thus an RGB value does not define the same color across devices without some kind of color management" This claim alone leaves much room for the manufacturers to pretend that their devices are compliant with RGB model. And the documentation of the hardware the discussed driver is for also refers to RGB model in many places - e.g. see Table 1, page 15 in the document [0], where mapping of output triplets to an RGB module is shown. One thing that I missed is that the discussed hardware provides LEDn_BRIGHTNESS registers for each RGB LED module, that can be configured to set color intensity in linear or logarithmic fashion. Actually this stands in contradiction with RGB model, since change of "color intensity" means change of all RGB components. We could use brightness file as for monochrome LEDs for that, but we'd need to come up with consistent interface semantics for all devices, also those which don't have corresponding functionality. Probably this is the place where we could apply some RGB<->HSV conversion, as color intensity feels something more of HSV's saturation and value. It would be good to hear from Dan how that looks in reality in case of lp5024 device. Our problem is how to set the color atomically. With HSV approach we were to obviate the problem by mapping brightness file to the "V" component of that color space, and write all H,S and V values to the hardware only on write to brightness file. I'm not sure how realistic the "atomic color" problem is. Computers are way faster than human vision. With LEDn_BRIGHTNESS registers of lp5024 it seems that we need the ability for grouping LEDs in triplets and be able to set their intensity with a single write operation. I believe problem to start with is the "white" problem. Setting R=G=B=255 will _not_ result in anything close to white light on hardware I have. RGBW LED controllers solve this problem. For the devices without white/amber we cannot do more than the hardware allows for. [0] http://www.ti.com/lit/ds/symlink/lp5024.pdf -- Best regards, Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On Sun 2018-12-30 18:09:35, Jacek Anaszewski wrote: > On 12/29/18 8:07 PM, Pavel Machek wrote: > >Hi! > > > With the "color" sysfs file it will make more sense to allow for user > defined color palettes. > > >>> > >>>I think defining these values in the device tree or acpi severely limits > >>>the devices > >>>capabilities. Especially in development phases. If the knobs were > >>>exposed then the user space > >>>can create new experiences. The color definition should be an absolute > >>>color defined in the dt and > >>>either the framework or user space needs to mix these appropriately. IMO > >>>user space should set the policy > >>>of the user experience and the dt/acpi needs to set the capabilities. > >>> > >>>I do like Pavels idea on defining the more standard binding pattern to > >>>"group" leds into a single group. > >>> > >>>Maybe the framework could take these groups and combine/group them into a > >>>single node with the groups colors. > >> > >>There is still HSV approach [0] in store. One problem with proposed > >>implementation is fixed algorithm of RGB <-> HSV color space conversion. > >>Maybe allowing for some board specific adjustments in DT would add > >>more flexibility. > >> > >>[0] https://lkml.org/lkml/2017/8/31/255 > > > >Yes we could do HSV. Problem is that that we do not really have RGB > >available. We do have integers for red, green and blue, but they do > >not correspond to RGB colorspace. > > OK, so conversion from HSV to RGB would only increase the aberration. > So, let's stick to RGB - we've got to have some stable ground and this > is something that the hardware at least pretends to be compliant >with. I'm not saying that we should stick to RGB. I'm just saying that problem is complex. And no, hardware does not even pretend to be compliant with RGB color model ( https://en.wikipedia.org/wiki/RGB_color_model ). In particular, in RGB there is non-linear brightness curve. > Our problem is how to set the color atomically. With HSV approach we > were to obviate the problem by mapping brightness file to the "V" > component of that color space, and write all H,S and V values to the > hardware only on write to brightness file. I'm not sure how realistic the "atomic color" problem is. Computers are way faster than human vision. I believe problem to start with is the "white" problem. Setting R=G=B=255 will _not_ result in anything close to white light on hardware I have. Anyway, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/29/18 8:07 PM, Pavel Machek wrote: Hi! With the "color" sysfs file it will make more sense to allow for user defined color palettes. I think defining these values in the device tree or acpi severely limits the devices capabilities. Especially in development phases. If the knobs were exposed then the user space can create new experiences. The color definition should be an absolute color defined in the dt and either the framework or user space needs to mix these appropriately. IMO user space should set the policy of the user experience and the dt/acpi needs to set the capabilities. I do like Pavels idea on defining the more standard binding pattern to "group" leds into a single group. Maybe the framework could take these groups and combine/group them into a single node with the groups colors. There is still HSV approach [0] in store. One problem with proposed implementation is fixed algorithm of RGB <-> HSV color space conversion. Maybe allowing for some board specific adjustments in DT would add more flexibility. [0] https://lkml.org/lkml/2017/8/31/255 Yes we could do HSV. Problem is that that we do not really have RGB available. We do have integers for red, green and blue, but they do not correspond to RGB colorspace. OK, so conversion from HSV to RGB would only increase the aberration. So, let's stick to RGB - we've got to have some stable ground and this is something that the hardware at least pretends to be compliant with. Our problem is how to set the color atomically. With HSV approach we were to obviate the problem by mapping brightness file to the "V" component of that color space, and write all H,S and V values to the hardware only on write to brightness file. We could apply similar approach in case of RGB LED class device. We would have four files instead of a single brightness file: red, green, blue and main_color. First three would accept values within 0-255 range, and main_color would accept one of "red", "green" or "blue". LED RGB class driver would write all color components to the hardware only on write to the file corresponding to the main_color. Also LED Trigger core would be taught to map brightness value to the main_color for RGB LEDs. Of course a new internal kernel API for setting color would have to be provided for use by dedicated RGB triggers. -- Best regards, Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > >>With the "color" sysfs file it will make more sense to allow for user > >>defined color palettes. > >> > > > >I think defining these values in the device tree or acpi severely limits the > >devices > >capabilities. Especially in development phases. If the knobs were exposed > >then the user space > >can create new experiences. The color definition should be an absolute > >color defined in the dt and > >either the framework or user space needs to mix these appropriately. IMO > >user space should set the policy > >of the user experience and the dt/acpi needs to set the capabilities. > > > >I do like Pavels idea on defining the more standard binding pattern to > >"group" leds into a single group. > > > >Maybe the framework could take these groups and combine/group them into a > >single node with the groups colors. > > There is still HSV approach [0] in store. One problem with proposed > implementation is fixed algorithm of RGB <-> HSV color space conversion. > Maybe allowing for some board specific adjustments in DT would add > more flexibility. > > [0] https://lkml.org/lkml/2017/8/31/255 Yes we could do HSV. Problem is that that we do not really have RGB available. We do have integers for red, green and blue, but they do not correspond to RGB colorspace. Anyway, this should not be driver specific; all drivers should use one interface. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/21/18 2:05 PM, Dan Murphy wrote:
On 12/21/2018 01:32 AM, Jacek Anaszewski wrote:
On 12/20/18 9:31 PM, Jacek Anaszewski wrote:
On 12/19/18 10:50 PM, Dan Murphy wrote:
On 12/19/2018 03:36 PM, Jacek Anaszewski wrote:
Hi Dan and Pavel,
On 12/19/18 9:41 PM, Dan Murphy wrote:
Pavel
On 12/19/2018 02:10 PM, Pavel Machek wrote:
On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
Pavel
Thanks for the review.
On 12/19/2018 01:34 PM, Pavel Machek wrote:
Hi!
+static DEVICE_ATTR_WO(ctrl_bank_a_mix);
+static DEVICE_ATTR_WO(ctrl_bank_b_mix);
+static DEVICE_ATTR_WO(ctrl_bank_c_mix);
+
+static struct attribute *lp5024_ctrl_bank_attrs[] = {
+ _attr_ctrl_bank_a_mix.attr,
+ _attr_ctrl_bank_b_mix.attr,
+ _attr_ctrl_bank_c_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
...
+
+static DEVICE_ATTR_WO(led1_mix);
+static DEVICE_ATTR_WO(led2_mix);
+static DEVICE_ATTR_WO(led3_mix);
+
+static struct attribute *lp5024_led_independent_attrs[] = {
+ _attr_led1_mix.attr,
+ _attr_led2_mix.attr,
+ _attr_led3_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_led_independent);
Ok, so you are adding new sysfs files. Are they _really_ neccessary?
We'd like to have uniform interfaces for the leds.
Yes I am adding these for this driver.
They adjust the individual brightness of each LED connected (what the HW guys
called mixing).
The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all 3
LEDs brightness are
uniform.
1 LED, 1 brightness file... that's what we do. Why should this one be different?
Yes I understand this and 1 DT child node per LED out but...
This device has a single register to control 3 LEDs brightness as a group and
individual brightness
registers to control the LEDs independently to mix the LEDs to a specific color.
There needs to be a way to control both so that developers can mix and adjust
the individual RGB and
then control the brightness of the group during run time without touching the
"mixing" value.
I don't think a user needs nor would want to have 24 different LED nodes with
24 different brightness files.
Or with the LP5036 that would have 36 LED nodes.
Table 1 in the data sheet shows how the outputs map to the control banks to the
LED registers.
Some time ago we had discussion with Vesa Jääskeläinen about possible
approaches to RGB LEDs [0]. What seemed to be the most suitable
variation of the discussed out-of-tree approach was the "color" property
and array of color triplets defined in Device Tree per each color.
Why does Device tree define the color?
Rob indicated that Device tree is supposed to define the hardware.
This thread seems to be defining the operation.
Perceived colors produced by LEDs from different manufacturers may
differ and this alone should be deemed a sufficient argument for having
board specific color definitions.
Shouldn't the color be done via user space and not dt?
I think that we should keep the userspace interface as simple
as possible and backwards compatible with monochrome LEDs.
I also propose to avoid the introduction of a color sysfs
property in favor of creating separate LED class devices
for different "color ranges". The devices would drive the same
LED but using different preset color levels.
On the other hand, scattering the control over the hardware
among multiple LED class devices would complicate extension
of pattern trigger with the support for RGB LEDs.
I looks like we will need the "color" sysfs file anyway.
We don't have to expose all device knobs to the userspace,
but instead provide some predefined configurations. It would
improve user experience by keeping LED class devices simple
in use. It would be Device Tree designer's responsibility to
provide color definitions that make sense for given RGB LED
controller and RGB LED element configuration.
Registering color palette with devm_rgb_register() you proposed
is also an option, but with one LED class device per color palette
it would mean allowing for creation/destruction of LED class
devices by any user having access to given LED's sysfs interface,
which is really bad solution.
With the "color" sysfs file it will make more sense to allow for user
defined color palettes.
I think defining these values in the device tree or acpi severely limits the
devices
capabilities. Especially in development phases. If the knobs were exposed
then the user space
can create new experiences. The color definition should be an absolute color
defined in the dt and
either the framework or user space needs to mix these appropriately. IMO user
space should set the policy
of the user experience and the dt/acpi needs to set the capabilities.
I do like Pavels idea on defining the more standard binding pattern to "group"
leds into a single group.
Maybe the framework could take these groups and combine/group them into a
single node with the groups colors.
There is still HSV approach [0] in store. One problem with proposed
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/21/2018 01:32 AM, Jacek Anaszewski wrote:
> On 12/20/18 9:31 PM, Jacek Anaszewski wrote:
>> On 12/19/18 10:50 PM, Dan Murphy wrote:
>>> On 12/19/2018 03:36 PM, Jacek Anaszewski wrote:
Hi Dan and Pavel,
On 12/19/18 9:41 PM, Dan Murphy wrote:
> Pavel
>
> On 12/19/2018 02:10 PM, Pavel Machek wrote:
>> On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
>>> Pavel
>>>
>>> Thanks for the review.
>>>
>>> On 12/19/2018 01:34 PM, Pavel Machek wrote:
Hi!
> +static DEVICE_ATTR_WO(ctrl_bank_a_mix);
> +static DEVICE_ATTR_WO(ctrl_bank_b_mix);
> +static DEVICE_ATTR_WO(ctrl_bank_c_mix);
> +
> +static struct attribute *lp5024_ctrl_bank_attrs[] = {
> + _attr_ctrl_bank_a_mix.attr,
> + _attr_ctrl_bank_b_mix.attr,
> + _attr_ctrl_bank_c_mix.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
...
> +
> +static DEVICE_ATTR_WO(led1_mix);
> +static DEVICE_ATTR_WO(led2_mix);
> +static DEVICE_ATTR_WO(led3_mix);
> +
> +static struct attribute *lp5024_led_independent_attrs[] = {
> + _attr_led1_mix.attr,
> + _attr_led2_mix.attr,
> + _attr_led3_mix.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(lp5024_led_independent);
Ok, so you are adding new sysfs files. Are they _really_ neccessary?
We'd like to have uniform interfaces for the leds.
>>>
>>> Yes I am adding these for this driver.
>>> They adjust the individual brightness of each LED connected (what the
>>> HW guys called mixing).
>>>
>>> The standard brightness sysfs adjusts all 3 LEDs simultaneously so that
>>> all 3 LEDs brightness are
>>> uniform.
>>
>> 1 LED, 1 brightness file... that's what we do. Why should this one be
>> different?
>>
>
> Yes I understand this and 1 DT child node per LED out but...
>
> This device has a single register to control 3 LEDs brightness as a group
> and individual brightness
> registers to control the LEDs independently to mix the LEDs to a specific
> color.
>
> There needs to be a way to control both so that developers can mix and
> adjust the individual RGB and
> then control the brightness of the group during run time without touching
> the "mixing" value.
>
> I don't think a user needs nor would want to have 24 different LED nodes
> with 24 different brightness files.
> Or with the LP5036 that would have 36 LED nodes.
>
> Table 1 in the data sheet shows how the outputs map to the control banks
> to the LED registers.
Some time ago we had discussion with Vesa Jääskeläinen about possible
approaches to RGB LEDs [0]. What seemed to be the most suitable
variation of the discussed out-of-tree approach was the "color" property
and array of color triplets defined in Device Tree per each color.
>>>
>>> Why does Device tree define the color?
>>>
>>> Rob indicated that Device tree is supposed to define the hardware.
>>> This thread seems to be defining the operation.
>>
>> Perceived colors produced by LEDs from different manufacturers may
>> differ and this alone should be deemed a sufficient argument for having
>> board specific color definitions.
>>
>>> Shouldn't the color be done via user space and not dt?
>>
>> I think that we should keep the userspace interface as simple
>> as possible and backwards compatible with monochrome LEDs.
>>
>> I also propose to avoid the introduction of a color sysfs
>> property in favor of creating separate LED class devices
>> for different "color ranges". The devices would drive the same
>> LED but using different preset color levels.
>
> On the other hand, scattering the control over the hardware
> among multiple LED class devices would complicate extension
> of pattern trigger with the support for RGB LEDs.
>
> I looks like we will need the "color" sysfs file anyway.
>
>> We don't have to expose all device knobs to the userspace,
>> but instead provide some predefined configurations. It would
>> improve user experience by keeping LED class devices simple
>> in use. It would be Device Tree designer's responsibility to
>> provide color definitions that make sense for given RGB LED
>> controller and RGB LED element configuration.
>>
>> Registering color palette with devm_rgb_register() you proposed
>> is also an option, but with one LED class device per color palette
>> it would mean allowing for creation/destruction of LED class
>> devices by any user having access to given LED's sysfs interface,
>> which is really bad solution.
>
> With the "color" sysfs file it will make more sense to allow for user
> defined color palettes.
>
I think defining these values in the device tree or acpi severely limits the
devices
capabilities.
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/20/18 9:31 PM, Jacek Anaszewski wrote:
On 12/19/18 10:50 PM, Dan Murphy wrote:
On 12/19/2018 03:36 PM, Jacek Anaszewski wrote:
Hi Dan and Pavel,
On 12/19/18 9:41 PM, Dan Murphy wrote:
Pavel
On 12/19/2018 02:10 PM, Pavel Machek wrote:
On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
Pavel
Thanks for the review.
On 12/19/2018 01:34 PM, Pavel Machek wrote:
Hi!
+static DEVICE_ATTR_WO(ctrl_bank_a_mix);
+static DEVICE_ATTR_WO(ctrl_bank_b_mix);
+static DEVICE_ATTR_WO(ctrl_bank_c_mix);
+
+static struct attribute *lp5024_ctrl_bank_attrs[] = {
+ _attr_ctrl_bank_a_mix.attr,
+ _attr_ctrl_bank_b_mix.attr,
+ _attr_ctrl_bank_c_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
...
+
+static DEVICE_ATTR_WO(led1_mix);
+static DEVICE_ATTR_WO(led2_mix);
+static DEVICE_ATTR_WO(led3_mix);
+
+static struct attribute *lp5024_led_independent_attrs[] = {
+ _attr_led1_mix.attr,
+ _attr_led2_mix.attr,
+ _attr_led3_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_led_independent);
Ok, so you are adding new sysfs files. Are they _really_ neccessary?
We'd like to have uniform interfaces for the leds.
Yes I am adding these for this driver.
They adjust the individual brightness of each LED connected (what
the HW guys called mixing).
The standard brightness sysfs adjusts all 3 LEDs simultaneously so
that all 3 LEDs brightness are
uniform.
1 LED, 1 brightness file... that's what we do. Why should this one
be different?
Yes I understand this and 1 DT child node per LED out but...
This device has a single register to control 3 LEDs brightness as a
group and individual brightness
registers to control the LEDs independently to mix the LEDs to a
specific color.
There needs to be a way to control both so that developers can mix
and adjust the individual RGB and
then control the brightness of the group during run time without
touching the "mixing" value.
I don't think a user needs nor would want to have 24 different LED
nodes with 24 different brightness files.
Or with the LP5036 that would have 36 LED nodes.
Table 1 in the data sheet shows how the outputs map to the control
banks to the LED registers.
Some time ago we had discussion with Vesa Jääskeläinen about possible
approaches to RGB LEDs [0]. What seemed to be the most suitable
variation of the discussed out-of-tree approach was the "color" property
and array of color triplets defined in Device Tree per each color.
Why does Device tree define the color?
Rob indicated that Device tree is supposed to define the hardware.
This thread seems to be defining the operation.
Perceived colors produced by LEDs from different manufacturers may
differ and this alone should be deemed a sufficient argument for having
board specific color definitions.
Shouldn't the color be done via user space and not dt?
I think that we should keep the userspace interface as simple
as possible and backwards compatible with monochrome LEDs.
I also propose to avoid the introduction of a color sysfs
property in favor of creating separate LED class devices
for different "color ranges". The devices would drive the same
LED but using different preset color levels.
On the other hand, scattering the control over the hardware
among multiple LED class devices would complicate extension
of pattern trigger with the support for RGB LEDs.
I looks like we will need the "color" sysfs file anyway.
We don't have to expose all device knobs to the userspace,
but instead provide some predefined configurations. It would
improve user experience by keeping LED class devices simple
in use. It would be Device Tree designer's responsibility to
provide color definitions that make sense for given RGB LED
controller and RGB LED element configuration.
Registering color palette with devm_rgb_register() you proposed
is also an option, but with one LED class device per color palette
it would mean allowing for creation/destruction of LED class
devices by any user having access to given LED's sysfs interface,
which is really bad solution.
With the "color" sysfs file it will make more sense to allow for user
defined color palettes.
--
Best regards,
Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/19/18 10:50 PM, Dan Murphy wrote:
On 12/19/2018 03:36 PM, Jacek Anaszewski wrote:
Hi Dan and Pavel,
On 12/19/18 9:41 PM, Dan Murphy wrote:
Pavel
On 12/19/2018 02:10 PM, Pavel Machek wrote:
On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
Pavel
Thanks for the review.
On 12/19/2018 01:34 PM, Pavel Machek wrote:
Hi!
+static DEVICE_ATTR_WO(ctrl_bank_a_mix);
+static DEVICE_ATTR_WO(ctrl_bank_b_mix);
+static DEVICE_ATTR_WO(ctrl_bank_c_mix);
+
+static struct attribute *lp5024_ctrl_bank_attrs[] = {
+ _attr_ctrl_bank_a_mix.attr,
+ _attr_ctrl_bank_b_mix.attr,
+ _attr_ctrl_bank_c_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
...
+
+static DEVICE_ATTR_WO(led1_mix);
+static DEVICE_ATTR_WO(led2_mix);
+static DEVICE_ATTR_WO(led3_mix);
+
+static struct attribute *lp5024_led_independent_attrs[] = {
+ _attr_led1_mix.attr,
+ _attr_led2_mix.attr,
+ _attr_led3_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_led_independent);
Ok, so you are adding new sysfs files. Are they _really_ neccessary?
We'd like to have uniform interfaces for the leds.
Yes I am adding these for this driver.
They adjust the individual brightness of each LED connected (what the HW guys
called mixing).
The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all 3
LEDs brightness are
uniform.
1 LED, 1 brightness file... that's what we do. Why should this one be different?
Yes I understand this and 1 DT child node per LED out but...
This device has a single register to control 3 LEDs brightness as a group and
individual brightness
registers to control the LEDs independently to mix the LEDs to a specific color.
There needs to be a way to control both so that developers can mix and adjust
the individual RGB and
then control the brightness of the group during run time without touching the
"mixing" value.
I don't think a user needs nor would want to have 24 different LED nodes with
24 different brightness files.
Or with the LP5036 that would have 36 LED nodes.
Table 1 in the data sheet shows how the outputs map to the control banks to the
LED registers.
Some time ago we had discussion with Vesa Jääskeläinen about possible
approaches to RGB LEDs [0]. What seemed to be the most suitable
variation of the discussed out-of-tree approach was the "color" property
and array of color triplets defined in Device Tree per each color.
Why does Device tree define the color?
Rob indicated that Device tree is supposed to define the hardware.
This thread seems to be defining the operation.
Perceived colors produced by LEDs from different manufacturers may
differ and this alone should be deemed a sufficient argument for having
board specific color definitions.
Shouldn't the color be done via user space and not dt?
I think that we should keep the userspace interface as simple
as possible and backwards compatible with monochrome LEDs.
I also propose to avoid the introduction of a color sysfs
property in favor of creating separate LED class devices
for different "color ranges". The devices would drive the same
LED but using different preset color levels.
We don't have to expose all device knobs to the userspace,
but instead provide some predefined configurations. It would
improve user experience by keeping LED class devices simple
in use. It would be Device Tree designer's responsibility to
provide color definitions that make sense for given RGB LED
controller and RGB LED element configuration.
Registering color palette with devm_rgb_register() you proposed
is also an option, but with one LED class device per color palette
it would mean allowing for creation/destruction of LED class
devices by any user having access to given LED's sysfs interface,
which is really bad solution.
--
Best regards,
Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/20/2018 03:06 AM, Pavel Machek wrote: > Hi! > >>> Anyway, if your 36 channels can be set independently, I believe you >>> just want them to export as 36 LEDs. >> >> I am not sure that is what the "customers" would want to have to set 36 >> different nodes or even declare those 36 nodes. >> That is 36 independent user space to kernel space calls that are very >> expensive just to set a LED color and brightness. >> > > No, 36 independend calls to kernel space are not that expensive. But they are time consuming. Also note that even if I set the OUTX color mix register the LEDX brightness register controls the output intensity. So if the brightness is 0x0 and the Mix register is 0xff then the LED is still off. I am sticking with the driver as is. I think this is a better representation of the part and its capability then presenting x number of sysfs nodes that will have no affect on the LED if set until the corresponding brightness register is not set properly. This will also keep the device tree entries down to a minimum. The LEDx brightness register actually controls the output and there are only 7 of these. Dan > > Pavel > -- -- Dan Murphy
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Vesa On 12/20/2018 06:40 AM, Vesa Jääskeläinen wrote: > Hi All, > > On 19/12/2018 23.50, Dan Murphy wrote: >> On 12/19/2018 03:36 PM, Jacek Anaszewski wrote: >>> Hi Dan and Pavel, >>> Some time ago we had discussion with Vesa Jääskeläinen about possible >>> approaches to RGB LEDs [0]. What seemed to be the most suitable >>> variation of the discussed out-of-tree approach was the "color" property >>> and array of color triplets defined in Device Tree per each color. >>> >> >> Why does Device tree define the color? >> >> Rob indicated that Device tree is supposed to define the hardware. >> This thread seems to be defining the operation. >> >> Shouldn't the color be done via user space and not dt? >> >> Especially if they want to change the color real time? >> >> Dan >> >>> Please refer to [0] for the details. >>> >>> [0] https://lkml.org/lkml/2018/11/9/938 >>> > > Idea was to define preset colors in device tree as an example when you are > dealing with multi-color LEDs without PWM. In that case you only have GPIOs > to control and then have a problem what does those GPIO's mean. > > With preset definitions one can use color names to act as a shortcut to > configure GPIO's to proper state for that particular color. > > For more flexible setups where you have PWM or such control you have larger > space of available colors. In this case you need to somehow define also > meaning of those controls. > > Also we may not have LED with only red, green and blue elements. There might > in example be amber, ultraviolet, white elements. > > This is where device tree is concerned. It helps us craft the logical > definition for LED so that we can control it from user space in common way. > > Now the next problem then is how does user space work then. > > For multi-color LEDs it it important to change the color atomically so that > no wrong colors are being shown as user space got interrupted when > controlling it. > > Also we have brightness setting that would be useful for PWM controlled LEDs. > > Setting color is easy when you use preset names then you only need to deal > with brightness value (eg. RGB -> HSV * brightness -> RGB). Of course here > additional problem is other color elements are they then scaled according to > brightness value?. > > Setting color as "raw" values is then next problem. In order to do it > atomically it needs to be one atomic activation and could be eg. one write to > "color" sysfs entry with combination of all color elements and perhaps > additionally also brightness value. Next question is then what is the format > for such entry then? What are the value ranges? In here we can utilize device > tree definition to help define what kind of LED we do have and what kind of > capabilities it does have. > > Additional problem risen also in discussion was non-linearity of some control > mechanisms vs. perceived color. So there might be a need for curve mapping > similarly what is with backlight control and that would be defined either in > device tree and possibly in user space if there is a need for that. I suppose > golden curve definition in device tree should be good enough. > > Then there was additional discussion about possible animation support but I > would leave that for future design as that would then be utilizing the same > framework. > > I suppose color space handling and that kind of stuff should be in some led > core functionality and then raw control should be part of physical led driver. > > I was planning to play with it during holiday season but lets see how it > goes. Feel free to also experiment with the idea. > Again I don't think device tree should be used to set color policy. This is to restricting it may be good for GPIO fixed current RGB LEDs but for variable RGB LEDs it would be very restricting. I believe this needs to be part of the LED framework and leave the device tree to define the HW and not define the product. Maybe a new devm_rgb_register call that exposes the color palette and can consolidate the 3 LEDs into a single sysfs node. Dan > Thanks, > Vesa Jääskeläinen -- -- Dan Murphy
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi All, On 19/12/2018 23.50, Dan Murphy wrote: On 12/19/2018 03:36 PM, Jacek Anaszewski wrote: Hi Dan and Pavel, Some time ago we had discussion with Vesa Jääskeläinen about possible approaches to RGB LEDs [0]. What seemed to be the most suitable variation of the discussed out-of-tree approach was the "color" property and array of color triplets defined in Device Tree per each color. Why does Device tree define the color? Rob indicated that Device tree is supposed to define the hardware. This thread seems to be defining the operation. Shouldn't the color be done via user space and not dt? Especially if they want to change the color real time? Dan Please refer to [0] for the details. [0] https://lkml.org/lkml/2018/11/9/938 Idea was to define preset colors in device tree as an example when you are dealing with multi-color LEDs without PWM. In that case you only have GPIOs to control and then have a problem what does those GPIO's mean. With preset definitions one can use color names to act as a shortcut to configure GPIO's to proper state for that particular color. For more flexible setups where you have PWM or such control you have larger space of available colors. In this case you need to somehow define also meaning of those controls. Also we may not have LED with only red, green and blue elements. There might in example be amber, ultraviolet, white elements. This is where device tree is concerned. It helps us craft the logical definition for LED so that we can control it from user space in common way. Now the next problem then is how does user space work then. For multi-color LEDs it it important to change the color atomically so that no wrong colors are being shown as user space got interrupted when controlling it. Also we have brightness setting that would be useful for PWM controlled LEDs. Setting color is easy when you use preset names then you only need to deal with brightness value (eg. RGB -> HSV * brightness -> RGB). Of course here additional problem is other color elements are they then scaled according to brightness value?. Setting color as "raw" values is then next problem. In order to do it atomically it needs to be one atomic activation and could be eg. one write to "color" sysfs entry with combination of all color elements and perhaps additionally also brightness value. Next question is then what is the format for such entry then? What are the value ranges? In here we can utilize device tree definition to help define what kind of LED we do have and what kind of capabilities it does have. Additional problem risen also in discussion was non-linearity of some control mechanisms vs. perceived color. So there might be a need for curve mapping similarly what is with backlight control and that would be defined either in device tree and possibly in user space if there is a need for that. I suppose golden curve definition in device tree should be good enough. Then there was additional discussion about possible animation support but I would leave that for future design as that would then be utilizing the same framework. I suppose color space handling and that kind of stuff should be in some led core functionality and then raw control should be part of physical led driver. I was planning to play with it during holiday season but lets see how it goes. Feel free to also experiment with the idea. Thanks, Vesa Jääskeläinen
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > > Anyway, if your 36 channels can be set independently, I believe you > > just want them to export as 36 LEDs. > > I am not sure that is what the "customers" would want to have to set 36 > different nodes or even declare those 36 nodes. > That is 36 independent user space to kernel space calls that are very > expensive just to set a LED color and brightness. > No, 36 independend calls to kernel space are not that expensive. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Pavel Thanks for trimming. On 12/19/2018 04:27 PM, Pavel Machek wrote: > Hi! > > [cc list trimmed] > > I don't think a user needs nor would want to have 24 different LED nodes > with 24 different brightness files. > Or with the LP5036 that would have 36 LED nodes. > > Table 1 in the data sheet shows how the outputs map to the control banks > to the LED registers. Some time ago we had discussion with Vesa Jääskeläinen about possible approaches to RGB LEDs [0]. What seemed to be the most suitable variation of the discussed out-of-tree approach was the "color" property and array of color triplets defined in Device Tree per each color. Please refer to [0] for the details. [0] https://lkml.org/lkml/2018/11/9/938 >>> >>> Yes, plus I also have the set of HSV patches somewhere... and they >>> work, but I found out that HSV->RGB conversion results in loss of precision. >>> >>> We may want to do something like that. >>> >>> But we need to do it once, in a driver core. We obviously don't want >>> each driver having different version of RGB support. >> >> True. But the RGB driver really should not be defining the color palette. >> >> Maybe a "color capability" reported to user space so that the user space can >> make the decision. >> >> It can report >> >> For GPIO or constant current situations >> static red >> static blue >> static green >> >> For adjustable configurations it can report: >> variable red >> variable blue >> variable green >> >> or even simpler "static" or "dynamic" as a return to report the RGB >> configuration. >> The LED driver would just need to set the variable. > > I'm not sure what static/variable is. My suggestion is really based in opinion. It is my opinion that the user space sets the policy of the RGB. The DT nor the driver should limit the capability of what the user would want to do. But instead should let the user know what the driver and HW can do. It seems that the suggestion made in the email thread [0] https://lkml.org/lkml/2018/11/9/938 limits that capability via the DT by defining a specific color. If I have a configurable current RGB combo then the palette is endless. But in a GPIO fixed current situation the palette is limited. So I don't think I agree with the DT side of it. There needs to be a way that the kernel can indicate to the user space "heres what I can do you need to let me know". As each kernel driver just needs to interact with the HW and not set policy. > > I have RGB leds here that can set any channel to 0-255, at > runtime. They seem to be quite common at phones. Yes I agree. Android specifically likes to set these values in the lighting HAL. But alternatively once the color is set the brightness can be controlled by PWM or an ALS. > > Anyway, if your 36 channels can be set independently, I believe you > just want them to export as 36 LEDs. I am not sure that is what the "customers" would want to have to set 36 different nodes or even declare those 36 nodes. That is 36 independent user space to kernel space calls that are very expensive just to set a LED color and brightness. Now that is very unrealistic that there would be 36 calls happening. But for this part it would cut the calls to set the brightness to the kernel in runtime after init to 1/3. Set the color and then only one call for the brightness. Dan > > Thanks, > Pavel > -- -- Dan Murphy
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! [cc list trimmed] > >>> I don't think a user needs nor would want to have 24 different LED nodes > >>> with 24 different brightness files. > >>> Or with the LP5036 that would have 36 LED nodes. > >>> > >>> Table 1 in the data sheet shows how the outputs map to the control banks > >>> to the LED registers. > >> > >> Some time ago we had discussion with Vesa Jääskeläinen about possible > >> approaches to RGB LEDs [0]. What seemed to be the most suitable > >> variation of the discussed out-of-tree approach was the "color" property > >> and array of color triplets defined in Device Tree per each color. > >> > >> Please refer to [0] for the details. > >> > >> [0] https://lkml.org/lkml/2018/11/9/938 > > > > Yes, plus I also have the set of HSV patches somewhere... and they > > work, but I found out that HSV->RGB conversion results in loss of precision. > > > > We may want to do something like that. > > > > But we need to do it once, in a driver core. We obviously don't want > > each driver having different version of RGB support. > > True. But the RGB driver really should not be defining the color palette. > > Maybe a "color capability" reported to user space so that the user space can > make the decision. > > It can report > > For GPIO or constant current situations > static red > static blue > static green > > For adjustable configurations it can report: > variable red > variable blue > variable green > > or even simpler "static" or "dynamic" as a return to report the RGB > configuration. > The LED driver would just need to set the variable. I'm not sure what static/variable is. I have RGB leds here that can set any channel to 0-255, at runtime. They seem to be quite common at phones. Anyway, if your 36 channels can be set independently, I believe you just want them to export as 36 LEDs. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/19/2018 04:03 PM, Pavel Machek wrote: > Hi! > >>> I don't think a user needs nor would want to have 24 different LED nodes >>> with 24 different brightness files. >>> Or with the LP5036 that would have 36 LED nodes. >>> >>> Table 1 in the data sheet shows how the outputs map to the control banks to >>> the LED registers. >> >> Some time ago we had discussion with Vesa Jääskeläinen about possible >> approaches to RGB LEDs [0]. What seemed to be the most suitable >> variation of the discussed out-of-tree approach was the "color" property >> and array of color triplets defined in Device Tree per each color. >> >> Please refer to [0] for the details. >> >> [0] https://lkml.org/lkml/2018/11/9/938 > > Yes, plus I also have the set of HSV patches somewhere... and they > work, but I found out that HSV->RGB conversion results in loss of precision. > > We may want to do something like that. > > But we need to do it once, in a driver core. We obviously don't want > each driver having different version of RGB support. True. But the RGB driver really should not be defining the color palette. Maybe a "color capability" reported to user space so that the user space can make the decision. It can report For GPIO or constant current situations static red static blue static green For adjustable configurations it can report: variable red variable blue variable green or even simpler "static" or "dynamic" as a return to report the RGB configuration. The LED driver would just need to set the variable. Dan > > Best regards, > Pavel > -- -- Dan Murphy
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi! > >I don't think a user needs nor would want to have 24 different LED nodes > >with 24 different brightness files. > >Or with the LP5036 that would have 36 LED nodes. > > > >Table 1 in the data sheet shows how the outputs map to the control banks to > >the LED registers. > > Some time ago we had discussion with Vesa Jääskeläinen about possible > approaches to RGB LEDs [0]. What seemed to be the most suitable > variation of the discussed out-of-tree approach was the "color" property > and array of color triplets defined in Device Tree per each color. > > Please refer to [0] for the details. > > [0] https://lkml.org/lkml/2018/11/9/938 Yes, plus I also have the set of HSV patches somewhere... and they work, but I found out that HSV->RGB conversion results in loss of precision. We may want to do something like that. But we need to do it once, in a driver core. We obviously don't want each driver having different version of RGB support. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/19/2018 03:36 PM, Jacek Anaszewski wrote:
> Hi Dan and Pavel,
>
> On 12/19/18 9:41 PM, Dan Murphy wrote:
>> Pavel
>>
>> On 12/19/2018 02:10 PM, Pavel Machek wrote:
>>> On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
Pavel
Thanks for the review.
On 12/19/2018 01:34 PM, Pavel Machek wrote:
> Hi!
>
>> +static DEVICE_ATTR_WO(ctrl_bank_a_mix);
>> +static DEVICE_ATTR_WO(ctrl_bank_b_mix);
>> +static DEVICE_ATTR_WO(ctrl_bank_c_mix);
>> +
>> +static struct attribute *lp5024_ctrl_bank_attrs[] = {
>> + _attr_ctrl_bank_a_mix.attr,
>> + _attr_ctrl_bank_b_mix.attr,
>> + _attr_ctrl_bank_c_mix.attr,
>> + NULL
>> +};
>> +ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
>
> ...
>> +
>> +static DEVICE_ATTR_WO(led1_mix);
>> +static DEVICE_ATTR_WO(led2_mix);
>> +static DEVICE_ATTR_WO(led3_mix);
>> +
>> +static struct attribute *lp5024_led_independent_attrs[] = {
>> + _attr_led1_mix.attr,
>> + _attr_led2_mix.attr,
>> + _attr_led3_mix.attr,
>> + NULL
>> +};
>> +ATTRIBUTE_GROUPS(lp5024_led_independent);
>
> Ok, so you are adding new sysfs files. Are they _really_ neccessary?
> We'd like to have uniform interfaces for the leds.
Yes I am adding these for this driver.
They adjust the individual brightness of each LED connected (what the HW
guys called mixing).
The standard brightness sysfs adjusts all 3 LEDs simultaneously so that
all 3 LEDs brightness are
uniform.
>>>
>>> 1 LED, 1 brightness file... that's what we do. Why should this one be
>>> different?
>>>
>>
>> Yes I understand this and 1 DT child node per LED out but...
>>
>> This device has a single register to control 3 LEDs brightness as a group
>> and individual brightness
>> registers to control the LEDs independently to mix the LEDs to a specific
>> color.
>>
>> There needs to be a way to control both so that developers can mix and
>> adjust the individual RGB and
>> then control the brightness of the group during run time without touching
>> the "mixing" value.
>>
>> I don't think a user needs nor would want to have 24 different LED nodes
>> with 24 different brightness files.
>> Or with the LP5036 that would have 36 LED nodes.
>>
>> Table 1 in the data sheet shows how the outputs map to the control banks to
>> the LED registers.
>
> Some time ago we had discussion with Vesa Jääskeläinen about possible
> approaches to RGB LEDs [0]. What seemed to be the most suitable
> variation of the discussed out-of-tree approach was the "color" property
> and array of color triplets defined in Device Tree per each color.
>
Why does Device tree define the color?
Rob indicated that Device tree is supposed to define the hardware.
This thread seems to be defining the operation.
Shouldn't the color be done via user space and not dt?
Especially if they want to change the color real time?
Dan
> Please refer to [0] for the details.
>
> [0] https://lkml.org/lkml/2018/11/9/938
>
--
--
Dan Murphy
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi Dan and Pavel,
On 12/19/18 9:41 PM, Dan Murphy wrote:
Pavel
On 12/19/2018 02:10 PM, Pavel Machek wrote:
On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
Pavel
Thanks for the review.
On 12/19/2018 01:34 PM, Pavel Machek wrote:
Hi!
+static DEVICE_ATTR_WO(ctrl_bank_a_mix);
+static DEVICE_ATTR_WO(ctrl_bank_b_mix);
+static DEVICE_ATTR_WO(ctrl_bank_c_mix);
+
+static struct attribute *lp5024_ctrl_bank_attrs[] = {
+ _attr_ctrl_bank_a_mix.attr,
+ _attr_ctrl_bank_b_mix.attr,
+ _attr_ctrl_bank_c_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
...
+
+static DEVICE_ATTR_WO(led1_mix);
+static DEVICE_ATTR_WO(led2_mix);
+static DEVICE_ATTR_WO(led3_mix);
+
+static struct attribute *lp5024_led_independent_attrs[] = {
+ _attr_led1_mix.attr,
+ _attr_led2_mix.attr,
+ _attr_led3_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_led_independent);
Ok, so you are adding new sysfs files. Are they _really_ neccessary?
We'd like to have uniform interfaces for the leds.
Yes I am adding these for this driver.
They adjust the individual brightness of each LED connected (what the HW guys
called mixing).
The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all 3
LEDs brightness are
uniform.
1 LED, 1 brightness file... that's what we do. Why should this one be different?
Yes I understand this and 1 DT child node per LED out but...
This device has a single register to control 3 LEDs brightness as a group and
individual brightness
registers to control the LEDs independently to mix the LEDs to a specific color.
There needs to be a way to control both so that developers can mix and adjust
the individual RGB and
then control the brightness of the group during run time without touching the
"mixing" value.
I don't think a user needs nor would want to have 24 different LED nodes with
24 different brightness files.
Or with the LP5036 that would have 36 LED nodes.
Table 1 in the data sheet shows how the outputs map to the control banks to the
LED registers.
Some time ago we had discussion with Vesa Jääskeläinen about possible
approaches to RGB LEDs [0]. What seemed to be the most suitable
variation of the discussed out-of-tree approach was the "color" property
and array of color triplets defined in Device Tree per each color.
Please refer to [0] for the details.
[0] https://lkml.org/lkml/2018/11/9/938
--
Best regards,
Jacek Anaszewski
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Pavel
On 12/19/2018 02:10 PM, Pavel Machek wrote:
> On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
>> Pavel
>>
>> Thanks for the review.
>>
>> On 12/19/2018 01:34 PM, Pavel Machek wrote:
>>> Hi!
>>>
+static DEVICE_ATTR_WO(ctrl_bank_a_mix);
+static DEVICE_ATTR_WO(ctrl_bank_b_mix);
+static DEVICE_ATTR_WO(ctrl_bank_c_mix);
+
+static struct attribute *lp5024_ctrl_bank_attrs[] = {
+ _attr_ctrl_bank_a_mix.attr,
+ _attr_ctrl_bank_b_mix.attr,
+ _attr_ctrl_bank_c_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
>>>
>>> ...
+
+static DEVICE_ATTR_WO(led1_mix);
+static DEVICE_ATTR_WO(led2_mix);
+static DEVICE_ATTR_WO(led3_mix);
+
+static struct attribute *lp5024_led_independent_attrs[] = {
+ _attr_led1_mix.attr,
+ _attr_led2_mix.attr,
+ _attr_led3_mix.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(lp5024_led_independent);
>>>
>>> Ok, so you are adding new sysfs files. Are they _really_ neccessary?
>>> We'd like to have uniform interfaces for the leds.
>>
>> Yes I am adding these for this driver.
>> They adjust the individual brightness of each LED connected (what the HW
>> guys called mixing).
>>
>> The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all
>> 3 LEDs brightness are
>> uniform.
>
> 1 LED, 1 brightness file... that's what we do. Why should this one be
> different?
>
Yes I understand this and 1 DT child node per LED out but...
This device has a single register to control 3 LEDs brightness as a group and
individual brightness
registers to control the LEDs independently to mix the LEDs to a specific color.
There needs to be a way to control both so that developers can mix and adjust
the individual RGB and
then control the brightness of the group during run time without touching the
"mixing" value.
I don't think a user needs nor would want to have 24 different LED nodes with
24 different brightness files.
Or with the LP5036 that would have 36 LED nodes.
Table 1 in the data sheet shows how the outputs map to the control banks to the
LED registers.
Dan
>> I did not think these belonged in the dt as they should be user space
>> configurable
>>
>>>
>>> If they are neccessary, we need documentation for them.
>>
>> Sure I have no problem documenting them but where do I do that?
>> I am assuming Documentation/leds/leds-lp5024.txt
>
> Documentation/abi/...
>
> Pavel
>
--
--
Dan Murphy
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On Wed 2018-12-19 13:41:18, Dan Murphy wrote:
> Pavel
>
> Thanks for the review.
>
> On 12/19/2018 01:34 PM, Pavel Machek wrote:
> > Hi!
> >
> >> +static DEVICE_ATTR_WO(ctrl_bank_a_mix);
> >> +static DEVICE_ATTR_WO(ctrl_bank_b_mix);
> >> +static DEVICE_ATTR_WO(ctrl_bank_c_mix);
> >> +
> >> +static struct attribute *lp5024_ctrl_bank_attrs[] = {
> >> + _attr_ctrl_bank_a_mix.attr,
> >> + _attr_ctrl_bank_b_mix.attr,
> >> + _attr_ctrl_bank_c_mix.attr,
> >> + NULL
> >> +};
> >> +ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
> >
> > ...
> >> +
> >> +static DEVICE_ATTR_WO(led1_mix);
> >> +static DEVICE_ATTR_WO(led2_mix);
> >> +static DEVICE_ATTR_WO(led3_mix);
> >> +
> >> +static struct attribute *lp5024_led_independent_attrs[] = {
> >> + _attr_led1_mix.attr,
> >> + _attr_led2_mix.attr,
> >> + _attr_led3_mix.attr,
> >> + NULL
> >> +};
> >> +ATTRIBUTE_GROUPS(lp5024_led_independent);
> >
> > Ok, so you are adding new sysfs files. Are they _really_ neccessary?
> > We'd like to have uniform interfaces for the leds.
>
> Yes I am adding these for this driver.
> They adjust the individual brightness of each LED connected (what the HW guys
> called mixing).
>
> The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all 3
> LEDs brightness are
> uniform.
1 LED, 1 brightness file... that's what we do. Why should this one be different?
> I did not think these belonged in the dt as they should be user space
> configurable
>
> >
> > If they are neccessary, we need documentation for them.
>
> Sure I have no problem documenting them but where do I do that?
> I am assuming Documentation/leds/leds-lp5024.txt
Documentation/abi/...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Pavel
Thanks for the review.
On 12/19/2018 01:34 PM, Pavel Machek wrote:
> Hi!
>
>> +static DEVICE_ATTR_WO(ctrl_bank_a_mix);
>> +static DEVICE_ATTR_WO(ctrl_bank_b_mix);
>> +static DEVICE_ATTR_WO(ctrl_bank_c_mix);
>> +
>> +static struct attribute *lp5024_ctrl_bank_attrs[] = {
>> +_attr_ctrl_bank_a_mix.attr,
>> +_attr_ctrl_bank_b_mix.attr,
>> +_attr_ctrl_bank_c_mix.attr,
>> +NULL
>> +};
>> +ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
>
> ...
>> +
>> +static DEVICE_ATTR_WO(led1_mix);
>> +static DEVICE_ATTR_WO(led2_mix);
>> +static DEVICE_ATTR_WO(led3_mix);
>> +
>> +static struct attribute *lp5024_led_independent_attrs[] = {
>> +_attr_led1_mix.attr,
>> +_attr_led2_mix.attr,
>> +_attr_led3_mix.attr,
>> +NULL
>> +};
>> +ATTRIBUTE_GROUPS(lp5024_led_independent);
>
> Ok, so you are adding new sysfs files. Are they _really_ neccessary?
> We'd like to have uniform interfaces for the leds.
Yes I am adding these for this driver.
They adjust the individual brightness of each LED connected (what the HW guys
called mixing).
The standard brightness sysfs adjusts all 3 LEDs simultaneously so that all 3
LEDs brightness are
uniform.
I did not think these belonged in the dt as they should be user space
configurable
>
> If they are neccessary, we need documentation for them.
Sure I have no problem documenting them but where do I do that?
I am assuming Documentation/leds/leds-lp5024.txt
This looks to be where Milo did this before.
Dan
>
> Thanks,
> Pavel
>
--
--
Dan Murphy
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
Hi!
> +static DEVICE_ATTR_WO(ctrl_bank_a_mix);
> +static DEVICE_ATTR_WO(ctrl_bank_b_mix);
> +static DEVICE_ATTR_WO(ctrl_bank_c_mix);
> +
> +static struct attribute *lp5024_ctrl_bank_attrs[] = {
> + _attr_ctrl_bank_a_mix.attr,
> + _attr_ctrl_bank_b_mix.attr,
> + _attr_ctrl_bank_c_mix.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(lp5024_ctrl_bank);
...
> +
> +static DEVICE_ATTR_WO(led1_mix);
> +static DEVICE_ATTR_WO(led2_mix);
> +static DEVICE_ATTR_WO(led3_mix);
> +
> +static struct attribute *lp5024_led_independent_attrs[] = {
> + _attr_led1_mix.attr,
> + _attr_led2_mix.attr,
> + _attr_led3_mix.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(lp5024_led_independent);
Ok, so you are adding new sysfs files. Are they _really_ neccessary?
We'd like to have uniform interfaces for the leds.
If they are neccessary, we need documentation for them.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH 2/2] leds: lp5024: Add the LP5024/18 RGB LED driver
On 12/19/2018 10:26 AM, Dan Murphy wrote: > Introduce the LP5024 and LP5018 RGB LED driver. > The difference in these 2 parts are only in the number of > LED outputs where the LP5024 can control 24 LEDs the LP5018 > can only control 18. > > The device has the ability to group LED output into control banks > so that multiple LED banks can be controlled with the same mixing and > brightness. Inversely the LEDs can also be controlled independently. > > Signed-off-by: Dan Murphy > --- > drivers/leds/Kconfig | 7 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-lp5024.c | 610 + > 3 files changed, 618 insertions(+) > create mode 100644 drivers/leds/leds-lp5024.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index a72f97fca57b..d306bedb00b7 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -326,6 +326,13 @@ config LEDS_LP3952 > To compile this driver as a module, choose M here: the > module will be called leds-lp3952. > > +config LEDS_LP5024 > + tristate "LED Support for TI LP5024/18 LED driver chip" > + depends on LEDS_CLASS && REGMAP_I2C > + help > + If you say yes here you get support for the Texas Instruments > + LP5024 and LP5018 LED driver. > + > config LEDS_LP55XX_COMMON > tristate "Common Driver for TI/National LP5521/5523/55231/5562/8501" > depends on LEDS_LP5521 || LEDS_LP5523 || LEDS_LP5562 || LEDS_LP8501 > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 4c1b0054f379..60b4e4ddd3ee 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -32,6 +32,7 @@ obj-$(CONFIG_LEDS_GPIO_REGISTER)+= leds-gpio-register.o > obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o > obj-$(CONFIG_LEDS_LP3944)+= leds-lp3944.o > obj-$(CONFIG_LEDS_LP3952)+= leds-lp3952.o > +obj-$(CONFIG_LEDS_LP5024)+= leds-lp5024.o > obj-$(CONFIG_LEDS_LP55XX_COMMON) += leds-lp55xx-common.o > obj-$(CONFIG_LEDS_LP5521)+= leds-lp5521.o > obj-$(CONFIG_LEDS_LP5523)+= leds-lp5523.o > diff --git a/drivers/leds/leds-lp5024.c b/drivers/leds/leds-lp5024.c > new file mode 100644 > index ..90e8dca15609 > --- /dev/null > +++ b/drivers/leds/leds-lp5024.c > @@ -0,0 +1,610 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* TI LP50XX LED chip family driver > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define LP5024_DEV_CFG0 0x00 > +#define LP5024_DEV_CFG1 0x01 > +#define LP5024_LED_CFG0 0x02 I have 2 more devices that are the same as this one the LP5036/5030. The major difference is the register map and some of the register offsets. As well as enabling the control bank for the extra LEDs. The delta starts here as the new devices add LED_CFG1 at 0x03 and then at LED7_BRT there are another 4 registers and then for the OUTX_CLR there are 12 more registers. So this pushes the RESET register way out there. So how should I handle this? I kicked around creating a new driver with the register and offset differences called the LP5036. Or is there some example on how to plug those devices in as well with a different reg map? I think a framework is over kill. Dan > +#define LP5024_BNK_BRT 0x03 > +#define LP5024_BNKA_CLR 0x04 > +#define LP5024_BNKB_CLR 0x05 > +#define LP5024_BNKC_CLR 0x06 > +#define LP5024_LED0_BRT 0x07 > +#define LP5024_LED1_BRT 0x08 > +#define LP5024_LED2_BRT 0x09 > +#define LP5024_LED3_BRT 0x0a > +#define LP5024_LED4_BRT 0x0b > +#define LP5024_LED5_BRT 0x0c > +#define LP5024_LED6_BRT 0x0d > +#define LP5024_LED7_BRT 0x0e > + > +#define LP5024_OUT0_CLR 0x0f > +#define LP5024_OUT1_CLR 0x10 > +#define LP5024_OUT2_CLR 0x11 > +#define LP5024_OUT3_CLR 0x12 > +#define LP5024_OUT4_CLR 0x13 > +#define LP5024_OUT5_CLR 0x14 > +#define LP5024_OUT6_CLR 0x15 > +#define LP5024_OUT7_CLR 0x16 > +#define LP5024_OUT8_CLR 0x17 > +#define LP5024_OUT9_CLR 0x18 > +#define LP5024_OUT10_CLR 0x19 > +#define LP5024_OUT11_CLR 0x1a > +#define LP5024_OUT12_CLR 0x1b > +#define LP5024_OUT13_CLR 0x1c > +#define LP5024_OUT14_CLR 0x1d > +#define LP5024_OUT15_CLR 0x1e > +#define LP5024_OUT16_CLR 0x1f > +#define LP5024_OUT17_CLR 0x20 > +#define LP5024_OUT18_CLR 0x21 > +#define LP5024_OUT19_CLR 0x22 > +#define LP5024_OUT20_CLR 0x23 > +#define LP5024_OUT21_CLR 0x24 > +#define LP5024_OUT22_CLR 0x25 > +#define LP5024_OUT23_CLR
