| Message ID | 20190114211723.11186-1-dmurphy@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers | expand |
Hi! > >+The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB > >+XX - Do not care ignored by the driver > >+RR - is the 8 bit Red LED value > >+GG - is the 8 bit Green LED value > >+BB - is the 8 bit Blue LED value > >+ > >+Example: > >+LED module output 4 of the LP5024 will be a yellow color: > >+echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color > >+ > >+LED module output 4 of the LP5024 will be dimmed 50%: > >+echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness > >+ > >+LED banked RGBs of the LP5036 will be a white color: > >+echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color > > This part with example cans remain in Documentation/leds if you > >like. Does it actually work like that on hardware? Is it supposed to support "normal" RGB colors as seen on monitors? Because 100% PWM on all channels does not result in white on hardware I have. > >+ } else { > >+ led_offset = (led->led_number * 3); > >+ red_reg = priv->mix_out0_reg + led_offset; > >+ green_reg = priv->mix_out0_reg + led_offset + 1; > >+ blue_reg = priv->mix_out0_reg + led_offset + 2; > >+ } > >+ > >+ red_val = (mix_value & 0xff0000) >> 16; > >+ green_val = (mix_value & 0xff00) >> 8; > >+ blue_val = (mix_value & 0xff); > > I've been rather thinking about space separated list of decimal > "red green blue" values, but maybe this way it will be less > controversial. Let's if there will be other opinions. We support maximum brightness > 255, so space separated is certainly better option than this. But... I believe we should have a reasonable design before we do something like this. There's no guarantee someone will not use lp50xx with just the white LEDs for example. How will this work? Plus existing hardware already uses three separate LEDs for RGB LED. Why not provide same interface? (Oh and don't try to say "sysfs is slow", without numbers). Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi! > On 1/15/19 4:22 PM, Pavel Machek wrote: > > Hi! > > > >>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB > >>> +XX - Do not care ignored by the driver > >>> +RR - is the 8 bit Red LED value > >>> +GG - is the 8 bit Green LED value > >>> +BB - is the 8 bit Blue LED value > >>> + > >>> +Example: > >>> +LED module output 4 of the LP5024 will be a yellow color: > >>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color > >>> + > >>> +LED module output 4 of the LP5024 will be dimmed 50%: > >>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness > >>> + > >>> +LED banked RGBs of the LP5036 will be a white color: > >>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color > >> > >> This part with example cans remain in Documentation/leds if you > >>> like. > > > > Does it actually work like that on hardware? > > What? If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, does it actually produce white? With all the different RGB modules manufacturers can use with lp5024P? If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does it actually produce yellow, with all the different RGB modules manufacturers can use with lp5024P? > > Is it supposed to support "normal" RGB colors as seen on monitors? > > Monitors are not an application for this part. You did not answer the question. When you talk about yellow, is it same yellow the rest of world talks about? > > Because 100% PWM on all channels does not result in white on hardware > > I have. > > I don't know I am usually blinded by the light and have no diffuser over > the LEDs to disperse the light so when I look I see all 3 colors. How can we have useful discussion about colors when you don't see the colors? Place a piece of paper over the LEDs.... > > But... > > > > I believe we should have a reasonable design before we do something > > like this. There's no guarantee someone will not use lp50xx with just > > the white LEDs for example. How will this work? Plus existing hardware > > already uses three separate LEDs for RGB LED. Why not provide same > > interface? > > Which existing hardware? Are they using this part? Nokia N900. They are not using this part, but any interface we invent should work there, too. > <rant> > Why are we delaying getting the RGB framework or HSV in? > I would rather design against something you want instead of having > everyone complain about every implementation I post. > </rant> Because you insist on creating new kernel interfaces, when existing interfaces work, and are doing that badly. Because your patches are of lower quality than is acceptable for linux kernel. Because you don't seem to be reading the emails. I sent list of requirements for RGB led support. This does not meet them. > It is not a normal RGB driver. The device collates the individual RGB > clusters into a single brightness register and you can modify the intensity of the individual > LEDs via other registers. If brightness is 0 then the cluster is OFF regardless of the color > set in the individual registers. I understand that. So just set cluster brightness to 255 and you have normal RGB driver you can control with existing interfaces. You don't have to use every feature your hardware has. You did not answer the "what if someone uses this with all white LEDs" question. You know what? First, submit driver with similar functionality to existing RGB drivers, using same interface existing drivers are using. When that is accepted, we can talk about extending kernel<->user interfaces. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hello On 1/16/19 4:04 PM, Pavel Machek wrote: > Hi! > > ..snip. Let me read it and reply when I have a time. > >>> You know what? First, submit driver with similar functionality to >>> existing RGB drivers, using same interface existing drivers are >>> using. When that is accepted, we can talk about extending >>> kernel<->user interfaces. >>> >> >> I could do that but then there is no way for users to have any other color but "white" with this driver. >> That defeats the purpose of the device itself. > > No, that is not what I meant. > > We do have RGB drivers in tree, they just present three separate LEDs > -- red, green and blue. I ask you to do the same for initial > submission. > For clarification you are asking me to register a LED class per output pin? If that is not you mean can you point me to an example because looking through the code I see the lp5562, bd2802 and lp3944 of which none are equivalent to the LP50xx devices. Each one of those devices have a dedicated register per LED output which makes perfect sense to do what you are asking. If it is what you are asking then this was already explained and it was agreed in the email chain to provide a Class with a "brightness" file that maps to the Master brightness register and the "color" file that would map to the respective color control register. These registrations would be per LED Module output and Banked output and not per LED output. RED LED Color Control Register------| |------| RED LED OUTPUT Green LED Color Control Register----|--- LED Master Brightness register--|------| Green LED OUTPUT Blue LED Color Control Register-----| |------| Blue LED OUTPUT As explained before the Master brightness register has absolute control over the output current to the LED outputs regardless of the color control setting. > You'll still be able to set brightness independently on the > red/green/blue LEDs... > But which one would control the overall brightness of the cluster? Dan >> I am not sure if you are aware of this or care but I found this recent blog on this effort: >> https://www.phoronix.com/scan.php?page=news_item&px=Linux-RGB-LED-Interface >> See some of the comments. > > I went through the comments quickly, but see nothing really > interesting. > > Best regards, > > Pavel > -- ------------------ Dan Murphy
Hello On 1/17/19 6:02 PM, Pavel Machek wrote: > Hi! > > >>> I am willing to work with you on the HSV and adapting the LP50xx part to this framework. >>> Or any RGB framework for that matter. I still don't agree with the kernel needing to declare colors >>> maybe color capabilities but not specific colors. >> >> Dan, if you have a bandwidth for LED RGB class implementation >> then please go ahead. It would be good to compare colors produced >> by software HSV->RGB algorithm to what can be achieved with >> LEDn_BRIGHTNESS feature. > > Don't get me wrong, I'd like to see LED RGB class implementation. But > it will delay merge of this driver. > > If we want to do that, we should first discuss the requirements, and > then come up with interface.. and only then we can talk about the > driver code. > > That's why I believe preferable way would be to merge the driver using > the existing interface. > > Of course, first designing RGB LED class and then merging the > driver.. is okay with me. But lets not rush the class because there's > driver waiting for it. > We have been able to provide users a preliminary driver they can use to test their hardware. It is not the ideal driver but it helps them develop their product. The important thing here is that the driver is available to all from the Linux mainline and not from a product repo. >> The requirements for LED RGB class as I would see it: >> >> sysfs interface: >> >> brightness-model: space separated list of available options: >> - rgb (default): >> - creates color file with "red green blue" decimal values >> - creates brightness file >> a) for devices with hardware support for adjusting color >> intensity it maps to corresponding register >> b) for the rest writing any value greater than 0 will result >> in setting all color registers to max >> - hsv: >> - creates color file with "h s v" values - it shall >> use software HSV->RGB algorithm for setting color registers >> >> - any other custom color ranges defined in DT, but it can be covered >> later >> - other options? > > First, I think we want to decide if RGB LED should be presented as > 3 LEDs or as 1 LED... and what to do with existing RGB leds being > presented as 3 LEDs. What do we do with RGBW drivers? Like the LP5562. RGB can be grouped as a single LED but does the white LED get grouped too or should it register as a new LED? I am assuming the answer is that it would register as a new LED. > > I don't think we want to support both RGB and HSV in the kernel. It is > math, and not a nice one. > > Yes, both have advantages and disadvantages, but having _both_ in > kernel has disadvantages of both. > I agree. Beyond the math I think giving developers a choice may bring about driver discussion of why the developer is using RGB over HSV or vice versa. > One way I could imagine the interface: > > RGB LED presented as one LED. > We would have to figure out a cluster schema that would group these LEDs. Difficulty would come if you have different drivers driving different LEDs. I personally have not seen this but the possibility is there. We don't have to put that in the initial design and if a developer needs this level of functionality they can submit a patch. > brightness -- controls brightness of whole RGB module. > This would be nice assuming the product groups them. They should also be individual LEDs like we have today. > pwm_channels -- "1000 240 300" -- "red part should be full on, green > should be pwm controlled to 240/1000, blue should be 300/1000" > Why 1000? Why not based on a duty cycle percentage and let the LED driver figure out what that means to the device? pwm_channels -- "100 24 30" -- red part is full on 100%, green is 24% duty cycle and blue is 30% duty cycle. > pwm_white -- "1000 500 400" -- tells userspace what to write to PWM > channels to get approximately white color. > Same as above on the duty cycle. But I still am in disagreement with the kernel detailing what is white or how to achieve white. Based on my earlier email about light pipes and the LED vendors and aging I think product developers will need to fine tune what is "white" on their product from user space or even a DT node. Not sure that the driver needs to have that level of intelligence because it has no idea what vendor or what shade of the LED it is driving or even if the LED it is driving is the color defined. All be it that the LP50xx data sheet does suggest that the LEDs be connected to particular outputs but that does not mean they will. Wondering if it would be better to set a DT node property like rgb-white that defines the values the driver would need for the hardware to produce a white color. Then the driver would register a set_color_white call back and the driver would take the hardware property and set the color values as defined. Then the user space can set either "color" to 0xff 0xff 0xff or pwm_channels to 100 100 100 and the framework would call the set_color_white call back as opposed to the set_color call back. If rbg-white does not exist in the dt node then no call back is set and the the set_color call back is only called. rgb-white can be an optional child property. And this eliminates the need for a user ABI Thoughts? > This would assume that RGB LEDs are always pwm controlled. That > seems to be true for hardware I seen. > GPIO LEDs can be binary or use PWM depending on what is needed. > + no complex math in kernel We can eliminate the math if we use the "color" file that was proposed and pass in absolute hex/decimal numbers for color. > > + userspace knows enough to display arbitrary colors > > + userspace can use full range of available PWM intensities > Do we need a pwm_max file to indicate to the user space what the max values of the pwm is for each color? > + existing triggers will work nicely > > - userland needs to do non-trivial math to get colors it wants > Could this be done via a DT node that exposes an attenuation constant for each LED? Then user space can query and apply the attenuation to the associated LED. Or userland may not care and this can be done in the driver. It would be optional child property. And this eliminates the need for a user ABI > - not sure how to migrate existing devices > > Thoughts? Other possible interfaces? Userland would need to know what RGBs are available. The product could use a green and blue or red and green or whatever combo. So userland can know that not all colors are available. Dan > > Best regards, > Pavel > -- ------------------ Dan Murphy
Hi Jacek et al, On 17/01/2019 23.08, Jacek Anaszewski wrote: > On 1/16/19 11:55 AM, Pavel Machek wrote: >> Hi! >> >>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>> Hi! >>>> >>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>> +XX - Do not care ignored by the driver >>>>>> +RR - is the 8 bit Red LED value >>>>>> +GG - is the 8 bit Green LED value >>>>>> +BB - is the 8 bit Blue LED value >>>>>> + >>>>>> +Example: >>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>> + >>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>> + >>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>> >>>>> This part with example cans remain in Documentation/leds if you >>>>>> like. >>>> >>>> Does it actually work like that on hardware? >>> >>> What? >> >> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >> does it actually produce white? With all the different RGB modules >> manufacturers can use with lp5024P? >> >> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does >> it actually produce yellow, with all the different RGB modules >> manufacturers can use with lp5024P? > > Vesa proposed using icc-profiles to make the colors looking similar > on various LEDs. It was in reply to your message with requirements > for RGB LED class. > > For this device we can however do without it. Videos showing the > performance of this particular device with a reference design using > RGB LEDs prove it works nice. > >>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>> >>> Monitors are not an application for this part. >> >> You did not answer the question. When you talk about yellow, is it >> same yellow the rest of world talks about? >> >>>> Because 100% PWM on all channels does not result in white on hardware >>>> I have. >>> >>> I don't know I am usually blinded by the light and have no diffuser over >>> the LEDs to disperse the light so when I look I see all 3 colors. >> >> How can we have useful discussion about colors when you don't see the >> colors? >> >> Place a piece of paper over the LEDs.... >> >>>> But... >>>> >>>> I believe we should have a reasonable design before we do something >>>> like this. There's no guarantee someone will not use lp50xx with just >>>> the white LEDs for example. How will this work? Plus existing hardware >>>> already uses three separate LEDs for RGB LED. Why not provide same >>>> interface? >>> >>> Which existing hardware? Are they using this part? >> >> Nokia N900. They are not using this part, but any interface we invent >> should work there, too. >> >>> <rant> >>> Why are we delaying getting the RGB framework or HSV in? >>> I would rather design against something you want instead of having >>> everyone complain about every implementation I post. >>> </rant> >> >> Because you insist on creating new kernel interfaces, when existing >> interfaces work, and are doing that badly. >> >> Because your patches are of lower quality than is acceptable for linux >> kernel. > > Lets keep things civil please. > > This sentence should look familiar to you - it's not the first time > you resort to this type of narration. > >> Because you don't seem to be reading the emails. >> >> I sent list of requirements for RGB led support. This does not meet >> them. > > And you didn't respond to the comments from Vesa. The requirements > has not been acclaimed. > >>> It is not a normal RGB driver. The device collates the individual RGB >>> clusters into a single brightness register and you can modify the >>> intensity of the individual >>> LEDs via other registers. If brightness is 0 then the cluster is OFF >>> regardless of the color >>> set in the individual registers. >> >> I understand that. So just set cluster brightness to 255 and you have >> normal RGB driver you can control with existing interfaces. You don't >> have to use every feature your hardware has. > > This feature is one of the core advantages of this device. > >> You did not answer the "what if someone uses this with all white LEDs" >> question. >> >> You know what? First, submit driver with similar functionality to >> existing RGB drivers, using same interface existing drivers are >> using. When that is accepted, we can talk about extending >> kernel<->user interfaces. > > This stands in contradiction with my request. > > Provided there will be no other NAKs, I am eager to accept the > interface with color and brightness files. > > Moreover, I think that RGB LED class with configurable > brightness-model, and with possible color range adjustments via > icc-profiles or something similar, is the best solution that has been > proposed so far. It is just flexible. > > I'd like to capitalize on the ideas shared in this thread and have > finally LED RGB class materialized. > I have now updated my github code with my understanding of the discussion: https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led Commits: - dt-bindings: leds: Introduce linux,default-brightness-model for all leds https://github.com/vesajaaskelainen/linux/commit/4ffb21d644056686096226bbede7c8c78b0254c2 - drivers: leds: Add core support for multi color element LEDs https://github.com/vesajaaskelainen/linux/commit/627f38bb78cebc694b8e6d735fb088c87925435d - dt-bindings: leds: leds-pwm: Introduce multi color element leds support https://github.com/vesajaaskelainen/linux/commit/ef6c5730d621e79ea0b02470caa83bc39439536a - WIP: drivers: leds: leds-pwm: Add multi color element LED support. https://github.com/vesajaaskelainen/linux/commit/0430a27823d9162926424b32c23be1c53eb9cbe2 First two commits are common and could be taken before I am happy with the pwm led driver changes. This new conditional feature flag makes it a bit harder. Of course one option would be to require it to be enabled. Current set of concepts: - brightness-model: hardware, onoff, linear - could be extended in future with other modes like hsv if wanted - led_common_setup_of: helper for setting up common parts of led class device from devicetree. - led_color_element_setup_of: helper for setting up color element parts of led class device from devicetree. - led_scale_color_elements: single point helper for handling brightness-model. - From user space point-of-view atomic changes for color elements and brightness. Sysfs interfaces: - brightness_model: Change of brightness model on the fly. Similar if as trigger selection. - color: configuring all color element values as array of values with support for optional brightness entry as last value. - color_names: information for user space about color element values in 'color' array. - max_color: information for user space about maximum values for entries in 'color' array. Example usage: $ cd /sys/class/leds/status $ ls brightness brightness_model color color_names device max_brightness max_color power subsystem trigger uevent $ cat brightness_model hardware [onoff] linear $ cat color_names red green blue $ cat max_color 255 255 255 $ cat brightness 0 # Setting up color to not so bright purple with brightness set to 255 $ echo "32 0 32 255" > color # Setting up color to a bit brighter purple with brightness $ echo "128 0 128 255" > color # Activate heartbeat blinking $ echo "heartbeat" > trigger # Now led is blinking with purple color # Change color to green $ echo "0 255 0" > color # Now led is blinking with green color And in devicetree I had now: multi-color-leds { compatible = "pwm-leds"; status-led { label = "status"; linux,default-brightness-model = "onoff"; element-red { pwms = <&ehrpwm0 0 100000 0>; }; element-green { pwms = <&ehrpwm1 0 100000 0>; }; element-blue { pwms = <&ehrpwm1 1 100000 0>; }; }; }; What do you think is this something we agree and could go forward? If it is it would be a good idea to get feedback from Dan on how easy it is to use and other possible issues if he takes common commits and tries it out with his lp50xx driver. Thanks, Vesa Jääskeläinen
Hi Pavel, On 19/01/2019 23.46, Pavel Machek wrote: > Hi! > >>> Moreover, I think that RGB LED class with configurable >>> brightness-model, and with possible color range adjustments via >>> icc-profiles or something similar, is the best solution that has been >>> proposed so far. It is just flexible. >>> >>> I'd like to capitalize on the ideas shared in this thread and have >>> finally LED RGB class materialized. >>> >> >> I have now updated my github code with my understanding of the discussion: >> https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led >> >> Commits: >> - dt-bindings: leds: Introduce linux,default-brightness-model for all leds >> https://github.com/vesajaaskelainen/linux/commit/4ffb21d644056686096226bbede7c8c78b0254c2 >> - drivers: leds: Add core support for multi color element LEDs >> https://github.com/vesajaaskelainen/linux/commit/627f38bb78cebc694b8e6d735fb088c87925435d >> - dt-bindings: leds: leds-pwm: Introduce multi color element leds support >> https://github.com/vesajaaskelainen/linux/commit/ef6c5730d621e79ea0b02470caa83bc39439536a >> - WIP: drivers: leds: leds-pwm: Add multi color element LED support. >> https://github.com/vesajaaskelainen/linux/commit/0430a27823d9162926424b32c23be1c53eb9cbe2 >> >> First two commits are common and could be taken before I am happy with the >> pwm led driver changes. This new conditional feature flag makes it a bit >> harder. Of course one option would be to require it to be enabled. >> >> Current set of concepts: >> - brightness-model: hardware, onoff, linear >> - could be extended in future with other modes like hsv if wanted > > Would it be enough to tell userspace what is relation between values > it writes and output power? > > Onoff is subset of linear, I guess. We already have max_brightness in > the API. > >> # Setting up color to not so bright purple with brightness set to 255 >> $ echo "32 0 32 255" > color > >> # Setting up color to a bit brighter purple with brightness >> $ echo "128 0 128 255" > color > > This would require colorspace conversion in kernel. I have: > > scales = (1., 0.39, 0.11) # for n900 > val = map(lambda x: int((x**2.2)*255), val) > (r, g, b) = val > > (r_, g_, b_) = m.scales > red = r*r_ > ... > > x**2.2 is simplified, real expression is more complex. But it is > floating point math... > > Do we want to do that? If we would have range in user space let say 0..255 for components and then in devicetree we could define ramp for each of the element like what is done for backlight pwm [1] [2]. I believe this would generate same result and user space interface would be a bit easier than writing direct pwm/power values. You could then tune this ramp based on properties of your led. And if you ran your software in another device it could behave similarly only requiring tuning in devicetree. For some of our lcd backlights we had generate non-linear ramp definition in devicetree to have linearly looking response when user configures brightness values from 0-100. Definition could be something like this in devicetree: element-red { pwms = <&ehrpwm0 0 100000 0>; brightness-levels = <0 128 256 1024 2048 4096 8192 16384 65535>; num-interpolated-steps = <32>; }; Don't know if better name would actually be in this case "ramp-levels" or such. Used pwm-backlights terminology in this example. [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/leds/backlight/pwm-backlight.txt [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/video/backlight/pwm_bl.c?h=v5.0-rc2#n253 Thanks, Vesa Jääskeläinen
Hi Dan, On 18/01/2019 15.58, Dan Murphy wrote: > Jacek > > On 1/18/19 7:45 AM, Dan Murphy wrote: >> Jacek >> >> On 1/17/19 3:10 PM, Jacek Anaszewski wrote: >>> Hi Dan, >>> >>> On 1/16/19 7:41 PM, Dan Murphy wrote: >>>> Hello >>>> >>>> On 1/16/19 4:55 AM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>>>>> Hi! >>>>>>> >>>>>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>>>>> +XX - Do not care ignored by the driver >>>>>>>>> +RR - is the 8 bit Red LED value >>>>>>>>> +GG - is the 8 bit Green LED value >>>>>>>>> +BB - is the 8 bit Blue LED value >>>>>>>>> + >>>>>>>>> +Example: >>>>>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>>>>> + >>>>>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>>>>> + >>>>>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>>>>> >>>>>>>> This part with example cans remain in Documentation/leds if you >>>>>>>>> like. >>>>>>> >>>>>>> Does it actually work like that on hardware? >>>>>> >>>>>> What? >>>>> >>>>> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >>>>> does it actually produce white? With all the different RGB modules >>>>> manufacturers can use with lp5024P? >>>>> >>>>> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does >>>>> it actually produce yellow, with all the different RGB modules >>>>> manufacturers can use with lp5024P? >>>>> >>>> >>>> I believe the answer to the general questions is no for any RGB cluster and driver out there. >>>> Because if you set the same values on each and every RGB device out there you will get varying shades of the color. >>>> But for this device yes the color does appear to be yellow to me versus what was displayed on my monitor by the HSL picker. >>>> But everyone interprets colors differently. >>>> >>>> If you write the same value for yellow or white on a droid 4 and the N900 do they produce the same color side by side? >>>> Most probably not. >>>> >>>> As you pointed out the PWM needs to be modified to obtain the correct white color to account for LED and other device constraints. >>>> >>>> But we need to take into account the light pipe. Pools nowadays have RGB LED spot lights in them. It can >>>> be set to white. On my pool right off the lens the color has a purplish hue to it. As the light is diffracted into >>>> the pool the color becomes white. The pool is clear. When I add chemicals to the pool and make it cloudy >>>> and turn on the lights the color off the lens is now white. This is an example on a large scale but the issue >>>> scales down to the hand helds and smart home applications. >>>> >>>> If the cluster is piped through a flexible optic 0xffffff may produce the "white" you want on its output. >>>> >>>> So an expectation of certain color without proper piping based on a single RGB value may be a little unreasonable. >>>> There may need to be a way to attenuate the values based on the hardware aspect of the equation ie light pipe (or lack thereof) and LED vendor. >>>> So if we write 0xffffff to the RGB driver the driver could adjust the intensity of the individual LEDs based on the diffraction >>>> coefficients. >>>> >>>> I also think that is an unreasonable expectation here that writing a single value to any LED RGB driver would produce >>>> a "rest of the world" absolute color. Maybe it can produce something similar but not identical. >>>> As you indicated in the requirements there is more involved here then just the LED and the values written. >>>> The colors should be close but may not be identical. >>>> >>>> A 10 year old N900 should not be considered the gold standard for color production due to advancements in LED, >>>> light pipe and LED driver technology. >>>> The single package RGB clusters on the board I am testing is about the size of a single RGB LED from 10 years ago. >>>> >>>> I agree that the interface developed should work on the device but the algorithm derived to obtain the color needs to have >>>> a hardware aspect to the calculation. >>>> >>>>>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>>>>> >>>>>> Monitors are not an application for this part. >>>>> >>>>> You did not answer the question. When you talk about yellow, is it >>>>> same yellow the rest of world talks about? >>>>> >>>> >>>> See above. It is close to what was on my monitor displayed. >>>> >>>>>>> Because 100% PWM on all channels does not result in white on hardware >>>>>>> I have. >>>>>> >>>>>> I don't know I am usually blinded by the light and have no diffuser over >>>>>> the LEDs to disperse the light so when I look I see all 3 colors. >>>>> >>>>> How can we have useful discussion about colors when you don't see the >>>>> colors? >>>>> >>>>> Place a piece of paper over the LEDs.... >>>>> >>>> >>>> Good suggestion for a rough test. >>>> >>>>>>> But... >>>>>>> >>>>>>> I believe we should have a reasonable design before we do something >>>>>>> like this. There's no guarantee someone will not use lp50xx with just >>>>>>> the white LEDs for example. How will this work? Plus existing hardware >>>>>>> already uses three separate LEDs for RGB LED. Why not provide same >>>>>>> interface? >>>>>> >>>>>> Which existing hardware? Are they using this part? >>>>> >>>>> Nokia N900. They are not using this part, but any interface we invent >>>>> should work there, too. >>>>> >>>> >>>> Yes a common interface would be nice with some sort of hardware tuning coefficient. >>>> >>>>>> <rant> >>>>>> Why are we delaying getting the RGB framework or HSV in? >>>>>> I would rather design against something you want instead of having >>>>>> everyone complain about every implementation I post. >>>>>> </rant> >>>>> >>>>> Because you insist on creating new kernel interfaces, when existing >>>>> interfaces work, and are doing that badly. >>>>> >>>>> Because your patches are of lower quality than is acceptable for linux >>>>> kernel. >>>>> >>>>> Because you don't seem to be reading the emails. >>>>> >>>>> I sent list of requirements for RGB led support. This does not meet >>>>> them. >>>>> >>>> >>>> Sigh. You did not answer my question. >>>> >>>> Your requirements seem to be centered around monitors but that is only one application of the current >>>> RGB LED landscape. >>>> >>>> I am willing to work with you on the HSV and adapting the LP50xx part to this framework. >>>> Or any RGB framework for that matter. I still don't agree with the kernel needing to declare colors >>>> maybe color capabilities but not specific colors. >>> >>> Dan, if you have a bandwidth for LED RGB class implementation >>> then please go ahead. It would be good to compare colors produced >>> by software HSV->RGB algorithm to what can be achieved with >>> LEDn_BRIGHTNESS feature. >>> >>> The requirements for LED RGB class as I would see it: >>> >>> sysfs interface: >>> >>> brightness-model: space separated list of available options: >>> - rgb (default): >>> - creates color file with "red green blue" decimal values >> >> What about other colored LEDs? Presenting RGB for an Amber LED does not seem right. >> Should the LED color come from the DT? >> > > I thought about this, other non-RGB LEDs would not use the RGB framework. > But should they have the same interfaces as RGB? > > Should PWM control be a global interface? In order to being able to set multi color element led at one go I would recommend using then model: color_names: "red green blue white" echo "32 43 0 128" > color This way all elements would be set at same time from user space point of view. This of course requires that it is part of the physical/logical led that is being controlled. If it is a separate thing then it would logically be differently controlled mono color led. If you look what kinds of leds are available lets say from digikey you get all kinds of combos: - red, green, blue - red, green, blue, amber - red, green, blue, white - red, green, blue, white - cool - red, green, blue, white - neutral - red, green, blue, white - warm - red, orange - purple, ultraviolet - amber, blue - amber, blue, cyan, green, red, violet, white - cool - amber, blue, green - amber, green, blue - and then lots of single special colors Better models even list some properties about how different color elements behave. I would expect red, green, blue to be the most common case. So automating for that for chip like what you are working now is reasonable. For simple case where we can expect red, green, blue mapping we can auto-configure the multi color element led with "red", "green" and "blue" color elements. But TI also have other kind of driver chips that could be used in "more freely" enabling use all of those combos. If user would have "amber", "green" and "blue" then we would need to add this definition in devicetree in order for user space to be able to recognize that now we have different config. Or if user would have most complex of those then in devicetree you would configure 7 color element led like "amber", "blue", "cyan", "green", "red", "violet", "white" as color elements. Then you could set them in one go: echo "32 76 43 2 43 76 54" > color Now in this special case if we would have hsl brightness model and lets say it only knows red, green and blue. Then it would pick red, green and blue color element names and only adjust those others would be linear should there be non-zero value. In this case however you might be better doing it in user space as it might be preferable to control all values let say in sRGB space and then let the software transform result to all color elements and then setup entries to kernel. In here the ICC color profile might be the way to go to model your let and let user space software do the fancier math. As this latest example is the fanciest I would like to remind that the red, green, blue is the most common. I would make their life easier and then when you go to more advanced setups then you are are already mentally ready to do more complex things :) As an example in our case we can do with "red", "green", and "red", "green", "blue" combos fill all our current requirements we have. Perhaps white could be added as one thing in future (white is currently used only in lcd module's background). Latest proposal I have for the interface now lets you do all of those things so it should be easy enough for most cases and still flexes to more complex cases while keeping kernel space simple. Thanks, Vesa Jääskeläinen
Vesa On 1/19/19 1:11 PM, Vesa Jääskeläinen wrote: > Hi Jacek et al, > > On 17/01/2019 23.08, Jacek Anaszewski wrote: >> On 1/16/19 11:55 AM, Pavel Machek wrote: >>> Hi! >>> >>>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>>> Hi! >>>>> >>>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>>> +XX - Do not care ignored by the driver >>>>>>> +RR - is the 8 bit Red LED value >>>>>>> +GG - is the 8 bit Green LED value >>>>>>> +BB - is the 8 bit Blue LED value >>>>>>> + >>>>>>> +Example: >>>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>>> + >>>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>>> + >>>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>>> >>>>>> This part with example cans remain in Documentation/leds if you >>>>>>> like. >>>>> >>>>> Does it actually work like that on hardware? >>>> >>>> What? >>> >>> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >>> does it actually produce white? With all the different RGB modules >>> manufacturers can use with lp5024P? >>> >>> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does >>> it actually produce yellow, with all the different RGB modules >>> manufacturers can use with lp5024P? >> >> Vesa proposed using icc-profiles to make the colors looking similar >> on various LEDs. It was in reply to your message with requirements >> for RGB LED class. >> >> For this device we can however do without it. Videos showing the >> performance of this particular device with a reference design using >> RGB LEDs prove it works nice. >> >>>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>>> >>>> Monitors are not an application for this part. >>> >>> You did not answer the question. When you talk about yellow, is it >>> same yellow the rest of world talks about? >>> >>>>> Because 100% PWM on all channels does not result in white on hardware >>>>> I have. >>>> >>>> I don't know I am usually blinded by the light and have no diffuser over >>>> the LEDs to disperse the light so when I look I see all 3 colors. >>> >>> How can we have useful discussion about colors when you don't see the >>> colors? >>> >>> Place a piece of paper over the LEDs.... >>> >>>>> But... >>>>> >>>>> I believe we should have a reasonable design before we do something >>>>> like this. There's no guarantee someone will not use lp50xx with just >>>>> the white LEDs for example. How will this work? Plus existing hardware >>>>> already uses three separate LEDs for RGB LED. Why not provide same >>>>> interface? >>>> >>>> Which existing hardware? Are they using this part? >>> >>> Nokia N900. They are not using this part, but any interface we invent >>> should work there, too. >>> >>>> <rant> >>>> Why are we delaying getting the RGB framework or HSV in? >>>> I would rather design against something you want instead of having >>>> everyone complain about every implementation I post. >>>> </rant> >>> >>> Because you insist on creating new kernel interfaces, when existing >>> interfaces work, and are doing that badly. >>> >>> Because your patches are of lower quality than is acceptable for linux >>> kernel. >> >> Lets keep things civil please. >> >> This sentence should look familiar to you - it's not the first time >> you resort to this type of narration. >> >>> Because you don't seem to be reading the emails. >>> >>> I sent list of requirements for RGB led support. This does not meet >>> them. >> >> And you didn't respond to the comments from Vesa. The requirements >> has not been acclaimed. >> >>>> It is not a normal RGB driver. The device collates the individual RGB >>>> clusters into a single brightness register and you can modify the intensity of the individual >>>> LEDs via other registers. If brightness is 0 then the cluster is OFF regardless of the color >>>> set in the individual registers. >>> >>> I understand that. So just set cluster brightness to 255 and you have >>> normal RGB driver you can control with existing interfaces. You don't >>> have to use every feature your hardware has. >> >> This feature is one of the core advantages of this device. >> >>> You did not answer the "what if someone uses this with all white LEDs" >>> question. >>> >>> You know what? First, submit driver with similar functionality to >>> existing RGB drivers, using same interface existing drivers are >>> using. When that is accepted, we can talk about extending >>> kernel<->user interfaces. >> >> This stands in contradiction with my request. >> >> Provided there will be no other NAKs, I am eager to accept the >> interface with color and brightness files. >> >> Moreover, I think that RGB LED class with configurable >> brightness-model, and with possible color range adjustments via >> icc-profiles or something similar, is the best solution that has been proposed so far. It is just flexible. >> >> I'd like to capitalize on the ideas shared in this thread and have >> finally LED RGB class materialized. >> > > I have now updated my github code with my understanding of the discussion: > https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led > Maybe I did not see it but a RFC to the list would be good for discussion points. > Commits: > - dt-bindings: leds: Introduce linux,default-brightness-model for all leds > https://github.com/vesajaaskelainen/linux/commit/4ffb21d644056686096226bbede7c8c78b0254c2 > - drivers: leds: Add core support for multi color element LEDs > https://github.com/vesajaaskelainen/linux/commit/627f38bb78cebc694b8e6d735fb088c87925435d > - dt-bindings: leds: leds-pwm: Introduce multi color element leds support > https://github.com/vesajaaskelainen/linux/commit/ef6c5730d621e79ea0b02470caa83bc39439536a > - WIP: drivers: leds: leds-pwm: Add multi color element LED support. > https://github.com/vesajaaskelainen/linux/commit/0430a27823d9162926424b32c23be1c53eb9cbe2 > > First two commits are common and could be taken before I am happy with the pwm led driver changes. This new conditional feature flag makes it a bit harder. Of course one option would be to require it to be enabled. > > Current set of concepts: > - brightness-model: hardware, onoff, linear Seem to be missing logarithmic > - could be extended in future with other modes like hsv if wanted > - led_common_setup_of: helper for setting up common parts of led class device from devicetree. > - led_color_element_setup_of: helper for setting up color element parts of led class device from devicetree. > - led_scale_color_elements: single point helper for handling brightness-model. > - From user space point-of-view atomic changes for color elements and brightness. > > Sysfs interfaces: > - brightness_model: Change of brightness model on the fly. Similar if as trigger selection. > - color: configuring all color element values as array of values with support for optional brightness entry as last value. > - color_names: information for user space about color element values in 'color' array. > - max_color: information for user space about maximum values for entries in 'color' array. > > Example usage: > > $ cd /sys/class/leds/status > > $ ls > brightness brightness_model color color_names device max_brightness max_color power subsystem trigger uevent > > $ cat brightness_model > hardware [onoff] linear > $ cat color_names > red green blue > $ cat max_color > 255 255 255 > $ cat brightness > 0 > What happens if brightness is set? Is that ignored? What is the driver supposed to do? The LP50xx can set brightness independent of color but other devices cannot do that. > # Setting up color to not so bright purple with brightness set to 255 > $ echo "32 0 32 255" > color > > # Setting up color to a bit brighter purple with brightness > $ echo "128 0 128 255" > color > > # Activate heartbeat blinking > $ echo "heartbeat" > trigger > > # Now led is blinking with purple color > > # Change color to green > $ echo "0 255 0" > color > > # Now led is blinking with green color > > And in devicetree I had now: > > multi-color-leds { > compatible = "pwm-leds"; > > status-led { > label = "status"; > linux,default-brightness-model = "onoff"; > > element-red { > pwms = <&ehrpwm0 0 100000 0>; > }; > element-green { > pwms = <&ehrpwm1 0 100000 0>; > }; > element-blue { > pwms = <&ehrpwm1 1 100000 0>; > }; > }; > }; > > What do you think is this something we agree and could go forward? > > If it is it would be a good idea to get feedback from Dan on how easy it is to use and other possible issues if he takes common commits and tries it out with his lp50xx driver. > The LP50xx pwm is not standard. It defines a PWM dithering mode which phase shifts the LED outputs and does not really control the PWM width of the LEDs. I can try to stitch this together for testing. I will need to go through this code and docs a bit more. Dan > Thanks, > Vesa Jääskeläinen -- ------------------ Dan Murphy
Hi Dan, On 21/01/2019 15.27, Dan Murphy wrote: > Vesa > > On 1/19/19 1:11 PM, Vesa Jääskeläinen wrote: >> Hi Jacek et al, >> >> On 17/01/2019 23.08, Jacek Anaszewski wrote: >>> On 1/16/19 11:55 AM, Pavel Machek wrote: >>>> Hi! >>>> >>>>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>>>> +XX - Do not care ignored by the driver >>>>>>>> +RR - is the 8 bit Red LED value >>>>>>>> +GG - is the 8 bit Green LED value >>>>>>>> +BB - is the 8 bit Blue LED value >>>>>>>> + >>>>>>>> +Example: >>>>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>>>> + >>>>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>>>> + >>>>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>>>> >>>>>>> This part with example cans remain in Documentation/leds if you >>>>>>>> like. >>>>>> >>>>>> Does it actually work like that on hardware? >>>>> >>>>> What? >>>> >>>> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >>>> does it actually produce white? With all the different RGB modules >>>> manufacturers can use with lp5024P? >>>> >>>> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does >>>> it actually produce yellow, with all the different RGB modules >>>> manufacturers can use with lp5024P? >>> >>> Vesa proposed using icc-profiles to make the colors looking similar >>> on various LEDs. It was in reply to your message with requirements >>> for RGB LED class. >>> >>> For this device we can however do without it. Videos showing the >>> performance of this particular device with a reference design using >>> RGB LEDs prove it works nice. >>> >>>>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>>>> >>>>> Monitors are not an application for this part. >>>> >>>> You did not answer the question. When you talk about yellow, is it >>>> same yellow the rest of world talks about? >>>> >>>>>> Because 100% PWM on all channels does not result in white on hardware >>>>>> I have. >>>>> >>>>> I don't know I am usually blinded by the light and have no diffuser over >>>>> the LEDs to disperse the light so when I look I see all 3 colors. >>>> >>>> How can we have useful discussion about colors when you don't see the >>>> colors? >>>> >>>> Place a piece of paper over the LEDs.... >>>> >>>>>> But... >>>>>> >>>>>> I believe we should have a reasonable design before we do something >>>>>> like this. There's no guarantee someone will not use lp50xx with just >>>>>> the white LEDs for example. How will this work? Plus existing hardware >>>>>> already uses three separate LEDs for RGB LED. Why not provide same >>>>>> interface? >>>>> >>>>> Which existing hardware? Are they using this part? >>>> >>>> Nokia N900. They are not using this part, but any interface we invent >>>> should work there, too. >>>> >>>>> <rant> >>>>> Why are we delaying getting the RGB framework or HSV in? >>>>> I would rather design against something you want instead of having >>>>> everyone complain about every implementation I post. >>>>> </rant> >>>> >>>> Because you insist on creating new kernel interfaces, when existing >>>> interfaces work, and are doing that badly. >>>> >>>> Because your patches are of lower quality than is acceptable for linux >>>> kernel. >>> >>> Lets keep things civil please. >>> >>> This sentence should look familiar to you - it's not the first time >>> you resort to this type of narration. >>> >>>> Because you don't seem to be reading the emails. >>>> >>>> I sent list of requirements for RGB led support. This does not meet >>>> them. >>> >>> And you didn't respond to the comments from Vesa. The requirements >>> has not been acclaimed. >>> >>>>> It is not a normal RGB driver. The device collates the individual RGB >>>>> clusters into a single brightness register and you can modify the intensity of the individual >>>>> LEDs via other registers. If brightness is 0 then the cluster is OFF regardless of the color >>>>> set in the individual registers. >>>> >>>> I understand that. So just set cluster brightness to 255 and you have >>>> normal RGB driver you can control with existing interfaces. You don't >>>> have to use every feature your hardware has. >>> >>> This feature is one of the core advantages of this device. >>> >>>> You did not answer the "what if someone uses this with all white LEDs" >>>> question. >>>> >>>> You know what? First, submit driver with similar functionality to >>>> existing RGB drivers, using same interface existing drivers are >>>> using. When that is accepted, we can talk about extending >>>> kernel<->user interfaces. >>> >>> This stands in contradiction with my request. >>> >>> Provided there will be no other NAKs, I am eager to accept the >>> interface with color and brightness files. >>> >>> Moreover, I think that RGB LED class with configurable >>> brightness-model, and with possible color range adjustments via >>> icc-profiles or something similar, is the best solution that has been proposed so far. It is just flexible. >>> >>> I'd like to capitalize on the ideas shared in this thread and have >>> finally LED RGB class materialized. >>> >> >> I have now updated my github code with my understanding of the discussion: >> https://github.com/vesajaaskelainen/linux/tree/wip-multi-color-led >> > > Maybe I did not see it but a RFC to the list would be good for discussion points. Ok. I can try to do that soonish. I want to clean PWM driver a bit first. >> Commits: >> - dt-bindings: leds: Introduce linux,default-brightness-model for all leds >> https://github.com/vesajaaskelainen/linux/commit/4ffb21d644056686096226bbede7c8c78b0254c2 >> - drivers: leds: Add core support for multi color element LEDs >> https://github.com/vesajaaskelainen/linux/commit/627f38bb78cebc694b8e6d735fb088c87925435d >> - dt-bindings: leds: leds-pwm: Introduce multi color element leds support >> https://github.com/vesajaaskelainen/linux/commit/ef6c5730d621e79ea0b02470caa83bc39439536a >> - WIP: drivers: leds: leds-pwm: Add multi color element LED support. >> https://github.com/vesajaaskelainen/linux/commit/0430a27823d9162926424b32c23be1c53eb9cbe2 >> >> First two commits are common and could be taken before I am happy with the pwm led driver changes. This new conditional feature flag makes it a bit harder. Of course one option would be to require it to be enabled. >> >> Current set of concepts: >> - brightness-model: hardware, onoff, linear > > Seem to be missing logarithmic How would you model that? As a ramp curve and then use linear? Do you think devicetree would be enough to define the ramp curve? >> - could be extended in future with other modes like hsv if wanted >> - led_common_setup_of: helper for setting up common parts of led class device from devicetree. >> - led_color_element_setup_of: helper for setting up color element parts of led class device from devicetree. >> - led_scale_color_elements: single point helper for handling brightness-model. >> - From user space point-of-view atomic changes for color elements and brightness. >> >> Sysfs interfaces: >> - brightness_model: Change of brightness model on the fly. Similar if as trigger selection. >> - color: configuring all color element values as array of values with support for optional brightness entry as last value. >> - color_names: information for user space about color element values in 'color' array. >> - max_color: information for user space about maximum values for entries in 'color' array. >> >> Example usage: >> >> $ cd /sys/class/leds/status >> >> $ ls >> brightness brightness_model color color_names device max_brightness max_color power subsystem trigger uevent >> >> $ cat brightness_model >> hardware [onoff] linear >> $ cat color_names >> red green blue >> $ cat max_color >> 255 255 255 >> $ cat brightness >> 0 >> > > What happens if brightness is set? Is that ignored? > What is the driver supposed to do? > The LP50xx can set brightness independent of color but other devices cannot do that. What happens is a call from led framework class to execute brightness set operation to the driver (cdev::brightness_set_blocking which in pwm driver is led_pwm_set). Driver can then (optionally) call for led_scale_color_elements() which would then calculate brightness model operation from set logical value to raw value. If mode is hardware then led_scale_color_elements() just copies the logical value to raw value. Then it is up to driver to decide what to do with those input information. Eg. in hardware brightness model case probably following: Iterate raw color element values and set registers based on those. Take brightness value from the call and set it to register. If brightness model is something else then it might be better to set brightness to 0 or 255. >> # Setting up color to not so bright purple with brightness set to 255 >> $ echo "32 0 32 255" > color >> >> # Setting up color to a bit brighter purple with brightness >> $ echo "128 0 128 255" > color >> >> # Activate heartbeat blinking >> $ echo "heartbeat" > trigger >> >> # Now led is blinking with purple color >> >> # Change color to green >> $ echo "0 255 0" > color >> >> # Now led is blinking with green color >> >> And in devicetree I had now: >> >> multi-color-leds { >> compatible = "pwm-leds"; >> >> status-led { >> label = "status"; >> linux,default-brightness-model = "onoff"; >> >> element-red { >> pwms = <&ehrpwm0 0 100000 0>; >> }; >> element-green { >> pwms = <&ehrpwm1 0 100000 0>; >> }; >> element-blue { >> pwms = <&ehrpwm1 1 100000 0>; >> }; >> }; >> }; >> >> What do you think is this something we agree and could go forward? >> >> If it is it would be a good idea to get feedback from Dan on how easy it is to use and other possible issues if he takes common commits and tries it out with his lp50xx driver. >> > > The LP50xx pwm is not standard. > It defines a PWM dithering mode which phase shifts the LED outputs and does not really control the PWM width of the LEDs. > > I can try to stitch this together for testing. I will need to go through this code and docs a bit more. Then it is even more important to test if the interface is suitable. Thanks, Vesa Jääskeläinen
Hi all, On 1/20/19 7:42 AM, Vesa Jääskeläinen wrote: > Hi Dan, > > On 18/01/2019 15.58, Dan Murphy wrote: >> Jacek >> >> On 1/18/19 7:45 AM, Dan Murphy wrote: >>> Jacek >>> >>> On 1/17/19 3:10 PM, Jacek Anaszewski wrote: >>>> Hi Dan, >>>> >>>> On 1/16/19 7:41 PM, Dan Murphy wrote: >>>>> Hello >>>>> >>>>> On 1/16/19 4:55 AM, Pavel Machek wrote: >>>>>> Hi! >>>>>> >>>>>>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>>>>>> Hi! >>>>>>>> >>>>>>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>>>>>> +XX - Do not care ignored by the driver >>>>>>>>>> +RR - is the 8 bit Red LED value >>>>>>>>>> +GG - is the 8 bit Green LED value >>>>>>>>>> +BB - is the 8 bit Blue LED value >>>>>>>>>> + >>>>>>>>>> +Example: >>>>>>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>>>>>> + >>>>>>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>>>>>> + >>>>>>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>>>>>> >>>>>>>>> This part with example cans remain in Documentation/leds if you >>>>>>>>>> like. >>>>>>>> >>>>>>>> Does it actually work like that on hardware? >>>>>>> >>>>>>> What? >>>>>> >>>>>> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >>>>>> does it actually produce white? With all the different RGB modules >>>>>> manufacturers can use with lp5024P? >>>>>> >>>>>> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, >>>>>> does >>>>>> it actually produce yellow, with all the different RGB modules >>>>>> manufacturers can use with lp5024P? >>>>>> >>>>> >>>>> I believe the answer to the general questions is no for any RGB >>>>> cluster and driver out there. >>>>> Because if you set the same values on each and every RGB device out >>>>> there you will get varying shades of the color. >>>>> But for this device yes the color does appear to be yellow to me >>>>> versus what was displayed on my monitor by the HSL picker. >>>>> But everyone interprets colors differently. >>>>> >>>>> If you write the same value for yellow or white on a droid 4 and >>>>> the N900 do they produce the same color side by side? >>>>> Most probably not. >>>>> >>>>> As you pointed out the PWM needs to be modified to obtain the >>>>> correct white color to account for LED and other device constraints. >>>>> >>>>> But we need to take into account the light pipe. Pools nowadays >>>>> have RGB LED spot lights in them. It can >>>>> be set to white. On my pool right off the lens the color has a >>>>> purplish hue to it. As the light is diffracted into >>>>> the pool the color becomes white. The pool is clear. When I add >>>>> chemicals to the pool and make it cloudy >>>>> and turn on the lights the color off the lens is now white. This >>>>> is an example on a large scale but the issue >>>>> scales down to the hand helds and smart home applications. >>>>> >>>>> If the cluster is piped through a flexible optic 0xffffff may >>>>> produce the "white" you want on its output. >>>>> >>>>> So an expectation of certain color without proper piping based on a >>>>> single RGB value may be a little unreasonable. >>>>> There may need to be a way to attenuate the values based on the >>>>> hardware aspect of the equation ie light pipe (or lack thereof) and >>>>> LED vendor. >>>>> So if we write 0xffffff to the RGB driver the driver could adjust >>>>> the intensity of the individual LEDs based on the diffraction >>>>> coefficients. >>>>> >>>>> I also think that is an unreasonable expectation here that writing >>>>> a single value to any LED RGB driver would produce >>>>> a "rest of the world" absolute color. Maybe it can produce >>>>> something similar but not identical. >>>>> As you indicated in the requirements there is more involved here >>>>> then just the LED and the values written. >>>>> The colors should be close but may not be identical. >>>>> >>>>> A 10 year old N900 should not be considered the gold standard for >>>>> color production due to advancements in LED, >>>>> light pipe and LED driver technology. >>>>> The single package RGB clusters on the board I am testing is about >>>>> the size of a single RGB LED from 10 years ago. >>>>> >>>>> I agree that the interface developed should work on the device but >>>>> the algorithm derived to obtain the color needs to have >>>>> a hardware aspect to the calculation. >>>>> >>>>>>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>>>>>> >>>>>>> Monitors are not an application for this part. >>>>>> >>>>>> You did not answer the question. When you talk about yellow, is it >>>>>> same yellow the rest of world talks about? >>>>>> >>>>> >>>>> See above. It is close to what was on my monitor displayed. >>>>> >>>>>>>> Because 100% PWM on all channels does not result in white on >>>>>>>> hardware >>>>>>>> I have. >>>>>>> >>>>>>> I don't know I am usually blinded by the light and have no >>>>>>> diffuser over >>>>>>> the LEDs to disperse the light so when I look I see all 3 colors. >>>>>> >>>>>> How can we have useful discussion about colors when you don't see the >>>>>> colors? >>>>>> >>>>>> Place a piece of paper over the LEDs.... >>>>>> >>>>> >>>>> Good suggestion for a rough test. >>>>> >>>>>>>> But... >>>>>>>> >>>>>>>> I believe we should have a reasonable design before we do something >>>>>>>> like this. There's no guarantee someone will not use lp50xx with >>>>>>>> just >>>>>>>> the white LEDs for example. How will this work? Plus existing >>>>>>>> hardware >>>>>>>> already uses three separate LEDs for RGB LED. Why not provide same >>>>>>>> interface? >>>>>>> >>>>>>> Which existing hardware? Are they using this part? >>>>>> >>>>>> Nokia N900. They are not using this part, but any interface we invent >>>>>> should work there, too. >>>>>> >>>>> >>>>> Yes a common interface would be nice with some sort of hardware >>>>> tuning coefficient. >>>>> >>>>>>> <rant> >>>>>>> Why are we delaying getting the RGB framework or HSV in? >>>>>>> I would rather design against something you want instead of having >>>>>>> everyone complain about every implementation I post. >>>>>>> </rant> >>>>>> >>>>>> Because you insist on creating new kernel interfaces, when existing >>>>>> interfaces work, and are doing that badly. >>>>>> >>>>>> Because your patches are of lower quality than is acceptable for >>>>>> linux >>>>>> kernel. >>>>>> >>>>>> Because you don't seem to be reading the emails. >>>>>> >>>>>> I sent list of requirements for RGB led support. This does not meet >>>>>> them. >>>>>> >>>>> >>>>> Sigh. You did not answer my question. >>>>> >>>>> Your requirements seem to be centered around monitors but that is >>>>> only one application of the current >>>>> RGB LED landscape. >>>>> >>>>> I am willing to work with you on the HSV and adapting the LP50xx >>>>> part to this framework. >>>>> Or any RGB framework for that matter. I still don't agree with the >>>>> kernel needing to declare colors >>>>> maybe color capabilities but not specific colors. >>>> >>>> Dan, if you have a bandwidth for LED RGB class implementation >>>> then please go ahead. It would be good to compare colors produced >>>> by software HSV->RGB algorithm to what can be achieved with >>>> LEDn_BRIGHTNESS feature. >>>> >>>> The requirements for LED RGB class as I would see it: >>>> >>>> sysfs interface: >>>> >>>> brightness-model: space separated list of available options: >>>> - rgb (default): >>>> - creates color file with "red green blue" decimal values >>> >>> What about other colored LEDs? Presenting RGB for an Amber LED does >>> not seem right. >>> Should the LED color come from the DT? >>> >> >> I thought about this, other non-RGB LEDs would not use the RGB framework. >> But should they have the same interfaces as RGB? >> >> Should PWM control be a global interface? > > In order to being able to set multi color element led at one go I would > recommend using then model: > > color_names: "red green blue white" > > echo "32 43 0 128" > color > > This way all elements would be set at same time from user space point of > view. > > This of course requires that it is part of the physical/logical led that > is being controlled. If it is a separate thing then it would logically > be differently controlled mono color led. > > If you look what kinds of leds are available lets say from digikey you > get all kinds of combos: > > - red, green, blue > - red, green, blue, amber > - red, green, blue, white > - red, green, blue, white - cool > - red, green, blue, white - neutral > - red, green, blue, white - warm > - red, orange > - purple, ultraviolet > - amber, blue > - amber, blue, cyan, green, red, violet, white - cool > - amber, blue, green > - amber, green, blue > - and then lots of single special colors It suggested me another solution. Instead of LED RGB class we would have LED multi-color class. Sysfs interface design: ----------------------- colors: directory containing files that map to the brightness of particular LEDs; there would be predefined color names that LED class driver should map iouts to, e.g.: - red - green - blue - white - sync: accepts "write" and "read", which executes write/readout to/from a device respectively. brightness-model: defines brightness level to color configuration mapping - "hardware": for devices with feature like LEDn_BRIGHTNESS of lp50xx - "rgb-<hue>": available only when all three red,green,blue colors are present in the colors directory. <hue> is a placeholder for given DT hue presets. - "rgbw-<hue>": Same as above, but available only when white color (any of amber or white-{cool|neutral|warm} can be mapped to white) is also available. In this mode max_brightness equals num-of-hue-presets + 1, and max_brightness, when set, turns the "white" LED on - "rgb-linear": I'm not sure if it should be available - it will have unpredictable results brightness: sets/reads brightness in the way specific to the current brightness-model. When more colors are available (e.g. amber, blue, cyan, green, red, violet, white), they are not touched by write to brightness). HSV->RGB conversion is left entirely to the userspace, which can set any color with use of the proposed interface. Let's agree on sysfs interface first, and only after that move to the DT details. Best regards, Jacek Anaszewski > Better models even list some properties about how different color > elements behave. > > I would expect red, green, blue to be the most common case. So > automating for that for chip like what you are working now is reasonable. > > For simple case where we can expect red, green, blue mapping we can > auto-configure the multi color element led with "red", "green" and > "blue" color elements. > > But TI also have other kind of driver chips that could be used in "more > freely" enabling use all of those combos. > > If user would have "amber", "green" and "blue" then we would need to add > this definition in devicetree in order for user space to be able to > recognize that now we have different config. > > Or if user would have most complex of those then in devicetree you would > configure 7 color element led like "amber", "blue", "cyan", "green", > "red", "violet", "white" as color elements. > > Then you could set them in one go: > echo "32 76 43 2 43 76 54" > color > > Now in this special case if we would have hsl brightness model and lets > say it only knows red, green and blue. Then it would pick red, green and > blue color element names and only adjust those others would be linear > should there be non-zero value. > > In this case however you might be better doing it in user space as it > might be preferable to control all values let say in sRGB space and then > let the software transform result to all color elements and then setup > entries to kernel. In here the ICC color profile might be the way to go > to model your let and let user space software do the fancier math. > > As this latest example is the fanciest I would like to remind that the > red, green, blue is the most common. I would make their life easier and > then when you go to more advanced setups then you are are already > mentally ready to do more complex things :) > > As an example in our case we can do with "red", "green", and "red", > "green", "blue" combos fill all our current requirements we have. > Perhaps white could be added as one thing in future (white is currently > used only in lcd module's background). > > Latest proposal I have for the interface now lets you do all of those > things so it should be easy enough for most cases and still flexes to > more complex cases while keeping kernel space simple. > > Thanks, > Vesa Jääskeläinen >
Jacek On 1/22/19 3:39 PM, Jacek Anaszewski wrote: > Hi all, > > On 1/20/19 7:42 AM, Vesa Jääskeläinen wrote: >> Hi Dan, >> >> On 18/01/2019 15.58, Dan Murphy wrote: >>> Jacek >>> >>> On 1/18/19 7:45 AM, Dan Murphy wrote: >>>> Jacek >>>> >>>> On 1/17/19 3:10 PM, Jacek Anaszewski wrote: >>>>> Hi Dan, >>>>> >>>>> On 1/16/19 7:41 PM, Dan Murphy wrote: >>>>>> Hello >>>>>> >>>>>> On 1/16/19 4:55 AM, Pavel Machek wrote: >>>>>>> Hi! >>>>>>> >>>>>>>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>>>>>>> Hi! >>>>>>>>> >>>>>>>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>>>>>>> +XX - Do not care ignored by the driver >>>>>>>>>>> +RR - is the 8 bit Red LED value >>>>>>>>>>> +GG - is the 8 bit Green LED value >>>>>>>>>>> +BB - is the 8 bit Blue LED value >>>>>>>>>>> + >>>>>>>>>>> +Example: >>>>>>>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>>>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>>>>>>> + >>>>>>>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>>>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>>>>>>> + >>>>>>>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>>>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>>>>>>> >>>>>>>>>> This part with example cans remain in Documentation/leds if you >>>>>>>>>>> like. >>>>>>>>> >>>>>>>>> Does it actually work like that on hardware? >>>>>>>> >>>>>>>> What? >>>>>>> >>>>>>> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >>>>>>> does it actually produce white? With all the different RGB modules >>>>>>> manufacturers can use with lp5024P? >>>>>>> >>>>>>> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does >>>>>>> it actually produce yellow, with all the different RGB modules >>>>>>> manufacturers can use with lp5024P? >>>>>>> >>>>>> >>>>>> I believe the answer to the general questions is no for any RGB cluster and driver out there. >>>>>> Because if you set the same values on each and every RGB device out there you will get varying shades of the color. >>>>>> But for this device yes the color does appear to be yellow to me versus what was displayed on my monitor by the HSL picker. >>>>>> But everyone interprets colors differently. >>>>>> >>>>>> If you write the same value for yellow or white on a droid 4 and the N900 do they produce the same color side by side? >>>>>> Most probably not. >>>>>> >>>>>> As you pointed out the PWM needs to be modified to obtain the correct white color to account for LED and other device constraints. >>>>>> >>>>>> But we need to take into account the light pipe. Pools nowadays have RGB LED spot lights in them. It can >>>>>> be set to white. On my pool right off the lens the color has a purplish hue to it. As the light is diffracted into >>>>>> the pool the color becomes white. The pool is clear. When I add chemicals to the pool and make it cloudy >>>>>> and turn on the lights the color off the lens is now white. This is an example on a large scale but the issue >>>>>> scales down to the hand helds and smart home applications. >>>>>> >>>>>> If the cluster is piped through a flexible optic 0xffffff may produce the "white" you want on its output. >>>>>> >>>>>> So an expectation of certain color without proper piping based on a single RGB value may be a little unreasonable. >>>>>> There may need to be a way to attenuate the values based on the hardware aspect of the equation ie light pipe (or lack thereof) and LED vendor. >>>>>> So if we write 0xffffff to the RGB driver the driver could adjust the intensity of the individual LEDs based on the diffraction >>>>>> coefficients. >>>>>> >>>>>> I also think that is an unreasonable expectation here that writing a single value to any LED RGB driver would produce >>>>>> a "rest of the world" absolute color. Maybe it can produce something similar but not identical. >>>>>> As you indicated in the requirements there is more involved here then just the LED and the values written. >>>>>> The colors should be close but may not be identical. >>>>>> >>>>>> A 10 year old N900 should not be considered the gold standard for color production due to advancements in LED, >>>>>> light pipe and LED driver technology. >>>>>> The single package RGB clusters on the board I am testing is about the size of a single RGB LED from 10 years ago. >>>>>> >>>>>> I agree that the interface developed should work on the device but the algorithm derived to obtain the color needs to have >>>>>> a hardware aspect to the calculation. >>>>>> >>>>>>>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>>>>>>> >>>>>>>> Monitors are not an application for this part. >>>>>>> >>>>>>> You did not answer the question. When you talk about yellow, is it >>>>>>> same yellow the rest of world talks about? >>>>>>> >>>>>> >>>>>> See above. It is close to what was on my monitor displayed. >>>>>> >>>>>>>>> Because 100% PWM on all channels does not result in white on hardware >>>>>>>>> I have. >>>>>>>> >>>>>>>> I don't know I am usually blinded by the light and have no diffuser over >>>>>>>> the LEDs to disperse the light so when I look I see all 3 colors. >>>>>>> >>>>>>> How can we have useful discussion about colors when you don't see the >>>>>>> colors? >>>>>>> >>>>>>> Place a piece of paper over the LEDs.... >>>>>>> >>>>>> >>>>>> Good suggestion for a rough test. >>>>>> >>>>>>>>> But... >>>>>>>>> >>>>>>>>> I believe we should have a reasonable design before we do something >>>>>>>>> like this. There's no guarantee someone will not use lp50xx with just >>>>>>>>> the white LEDs for example. How will this work? Plus existing hardware >>>>>>>>> already uses three separate LEDs for RGB LED. Why not provide same >>>>>>>>> interface? >>>>>>>> >>>>>>>> Which existing hardware? Are they using this part? >>>>>>> >>>>>>> Nokia N900. They are not using this part, but any interface we invent >>>>>>> should work there, too. >>>>>>> >>>>>> >>>>>> Yes a common interface would be nice with some sort of hardware tuning coefficient. >>>>>> >>>>>>>> <rant> >>>>>>>> Why are we delaying getting the RGB framework or HSV in? >>>>>>>> I would rather design against something you want instead of having >>>>>>>> everyone complain about every implementation I post. >>>>>>>> </rant> >>>>>>> >>>>>>> Because you insist on creating new kernel interfaces, when existing >>>>>>> interfaces work, and are doing that badly. >>>>>>> >>>>>>> Because your patches are of lower quality than is acceptable for linux >>>>>>> kernel. >>>>>>> >>>>>>> Because you don't seem to be reading the emails. >>>>>>> >>>>>>> I sent list of requirements for RGB led support. This does not meet >>>>>>> them. >>>>>>> >>>>>> >>>>>> Sigh. You did not answer my question. >>>>>> >>>>>> Your requirements seem to be centered around monitors but that is only one application of the current >>>>>> RGB LED landscape. >>>>>> >>>>>> I am willing to work with you on the HSV and adapting the LP50xx part to this framework. >>>>>> Or any RGB framework for that matter. I still don't agree with the kernel needing to declare colors >>>>>> maybe color capabilities but not specific colors. >>>>> >>>>> Dan, if you have a bandwidth for LED RGB class implementation >>>>> then please go ahead. It would be good to compare colors produced >>>>> by software HSV->RGB algorithm to what can be achieved with >>>>> LEDn_BRIGHTNESS feature. >>>>> >>>>> The requirements for LED RGB class as I would see it: >>>>> >>>>> sysfs interface: >>>>> >>>>> brightness-model: space separated list of available options: >>>>> - rgb (default): >>>>> - creates color file with "red green blue" decimal values >>>> >>>> What about other colored LEDs? Presenting RGB for an Amber LED does not seem right. >>>> Should the LED color come from the DT? >>>> >>> >>> I thought about this, other non-RGB LEDs would not use the RGB framework. >>> But should they have the same interfaces as RGB? >>> >>> Should PWM control be a global interface? >> >> In order to being able to set multi color element led at one go I would recommend using then model: >> >> color_names: "red green blue white" >> >> echo "32 43 0 128" > color >> >> This way all elements would be set at same time from user space point of view. >> >> This of course requires that it is part of the physical/logical led that is being controlled. If it is a separate thing then it would logically be differently controlled mono color led. >> >> If you look what kinds of leds are available lets say from digikey you get all kinds of combos: >> >> - red, green, blue >> - red, green, blue, amber >> - red, green, blue, white >> - red, green, blue, white - cool >> - red, green, blue, white - neutral >> - red, green, blue, white - warm >> - red, orange >> - purple, ultraviolet >> - amber, blue >> - amber, blue, cyan, green, red, violet, white - cool >> - amber, blue, green >> - amber, green, blue >> - and then lots of single special colors > > It suggested me another solution. Instead of LED RGB class > we would have LED multi-color class. > I was thinking the same thing this morning. But I was thinking that the RGB class should be an additional class to stand on its own or can register to the multi color class. > Sysfs interface design: > ----------------------- > > colors: directory containing files that map to > the brightness of particular LEDs; there > would be predefined color names that LED class > driver should map iouts to, e.g.: Filling in the missing ideas with questions. Is it a directory or a file? If it is a directory does that not break the current directory label model? so the path would be /sys/class/leds/colors ? (It is probably not this but needs clarification) How would this look if I had 2 of the same color LEDs? The Beagle bone black has 2 Blue LEDs. They are assigned to different triggers and have different directories. Both are GPIO controlled. Or are you saying it would be something like (More then likely this is what you intended) /sys/class/leds/input4::numlock/colors? Maybe it is mandated that "multi" be added to the label when the class is registered so the caller knows that this is a multi color class and not a single LED color class. What about providing a file called colors_raw which takes in the raw decimal values to obtain other color variants when RGB is only available? And this can also present a single write to the kernel with the new color values. I am not a fan of hard coding preset colors as you can see there are to many of them and variations of the color. In addition this severely limits the ability of the user. Unless we stick to primary colors only and not secondary colors. Defining and hard coding hte colors can get out of control and not maintainable moving forward. Especially if we start adding defines like white_warm, white_neutral and other variations to the color. What about IR LEDs used for night vision products? Do these fall into the multi color class? We do have a driver I submitted that had an IR LED and a White LED combined. It was created against the flash class but it could be a multi color LED as well. How would traversing through the full color palette work if you were to want to produce a multi color ring like the LP50xx where the pattern can traverse from one end of the color spectrum and back? Or in a product like the gaming keyboards that will change color or change backlight brightness? Not sure what color LEDs the keyboard manufacturers place on their keyboards but does the interface design capable of doing this? https://www.youtube.com/watch?v=pfKv3g2FeBE or something like this https://www.youtube.com/watch?v=PO3auX3f5C4 The LP5036 has this capability. > - red > - green > - blue > - white > - sync: accepts "write" and "read", which executes > write/readout to/from a device respectively. > What are these above, the values or the files under the colors directory? I am assuming they are files. Are they mandatory or optional? > brightness-model: defines brightness level to color configuration > mapping > - "hardware": for devices with feature like LEDn_BRIGHTNESS of lp50xx > - "rgb-<hue>": available only when all three red,green,blue colors > are present in the colors directory. > <hue> is a placeholder for given DT hue presets. > - "rgbw-<hue>": Same as above, but available only when white color > (any of amber or white-{cool|neutral|warm} can be > mapped to white) is also available. In this mode > max_brightness equals num-of-hue-presets + 1, and > max_brightness, when set, turns the "white" LED on Why do we need white combined here? Should this not be its own entity? Again I don't like limiting the color palette from the DT. I think that the user space can see what colors are available for that device and makes its own decision on what color to present. For the RGBw what about RGB amber and RGB purple. Are the white LEDs always part of the same function trying to be achieved by the system designer? The RGB can be used to denote notification status and the white can be used to denote that a charger is connected. Motorola Droid did this. > - "rgb-linear": I'm not sure if it should be available - it will > have unpredictable results > > brightness: sets/reads brightness in the way specific to the > current brightness-model. When more colors are available > (e.g. amber, blue, cyan, green, red, violet, white), they > are not touched by write to brightness). > > HSV->RGB conversion is left entirely to the userspace, which can set any > color with use of the proposed interface. > > Let's agree on sysfs interface first, and only after that move > to the DT details. > DT's are meant to describe hardware and not describe the product. Unless Rob does not see an issue with defining product capabilities in the DT then we should keep that out of the DT. Dan > Best regards, > Jacek Anaszewski > >> Better models even list some properties about how different color elements behave. >> >> I would expect red, green, blue to be the most common case. So automating for that for chip like what you are working now is reasonable. >> >> For simple case where we can expect red, green, blue mapping we can auto-configure the multi color element led with "red", "green" and "blue" color elements. >> >> But TI also have other kind of driver chips that could be used in "more freely" enabling use all of those combos. >> >> If user would have "amber", "green" and "blue" then we would need to add this definition in devicetree in order for user space to be able to recognize that now we have different config. >> >> Or if user would have most complex of those then in devicetree you would configure 7 color element led like "amber", "blue", "cyan", "green", "red", "violet", "white" as color elements. >> >> Then you could set them in one go: >> echo "32 76 43 2 43 76 54" > color >> >> Now in this special case if we would have hsl brightness model and lets say it only knows red, green and blue. Then it would pick red, green and blue color element names and only adjust those others would be linear should there be non-zero value. >> >> In this case however you might be better doing it in user space as it might be preferable to control all values let say in sRGB space and then let the software transform result to all color elements and then setup entries to kernel. In here the ICC color profile might be the way to go to model your let and let user space software do the fancier math. >> >> As this latest example is the fanciest I would like to remind that the red, green, blue is the most common. I would make their life easier and then when you go to more advanced setups then you are are already mentally ready to do more complex things :) >> >> As an example in our case we can do with "red", "green", and "red", "green", "blue" combos fill all our current requirements we have. Perhaps white could be added as one thing in future (white is currently used only in lcd module's background). >> >> Latest proposal I have for the interface now lets you do all of those things so it should be easy enough for most cases and still flexes to more complex cases while keeping kernel space simple. >> >> Thanks, >> Vesa Jääskeläinen >> > > -- ------------------ Dan Murphy
Dan, On 1/22/19 11:44 PM, Dan Murphy wrote: > Jacek > > On 1/22/19 3:39 PM, Jacek Anaszewski wrote: >> Hi all, >> >> On 1/20/19 7:42 AM, Vesa Jääskeläinen wrote: >>> Hi Dan, >>> >>> On 18/01/2019 15.58, Dan Murphy wrote: >>>> Jacek >>>> >>>> On 1/18/19 7:45 AM, Dan Murphy wrote: >>>>> Jacek >>>>> >>>>> On 1/17/19 3:10 PM, Jacek Anaszewski wrote: >>>>>> Hi Dan, >>>>>> >>>>>> On 1/16/19 7:41 PM, Dan Murphy wrote: >>>>>>> Hello >>>>>>> >>>>>>> On 1/16/19 4:55 AM, Pavel Machek wrote: >>>>>>>> Hi! >>>>>>>> >>>>>>>>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>>>>>>>> Hi! >>>>>>>>>> >>>>>>>>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>>>>>>>> +XX - Do not care ignored by the driver >>>>>>>>>>>> +RR - is the 8 bit Red LED value >>>>>>>>>>>> +GG - is the 8 bit Green LED value >>>>>>>>>>>> +BB - is the 8 bit Blue LED value >>>>>>>>>>>> + >>>>>>>>>>>> +Example: >>>>>>>>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>>>>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>>>>>>>> + >>>>>>>>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>>>>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>>>>>>>> + >>>>>>>>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>>>>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>>>>>>>> >>>>>>>>>>> This part with example cans remain in Documentation/leds if you >>>>>>>>>>>> like. >>>>>>>>>> >>>>>>>>>> Does it actually work like that on hardware? >>>>>>>>> >>>>>>>>> What? >>>>>>>> >>>>>>>> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >>>>>>>> does it actually produce white? With all the different RGB modules >>>>>>>> manufacturers can use with lp5024P? >>>>>>>> >>>>>>>> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does >>>>>>>> it actually produce yellow, with all the different RGB modules >>>>>>>> manufacturers can use with lp5024P? >>>>>>>> >>>>>>> >>>>>>> I believe the answer to the general questions is no for any RGB cluster and driver out there. >>>>>>> Because if you set the same values on each and every RGB device out there you will get varying shades of the color. >>>>>>> But for this device yes the color does appear to be yellow to me versus what was displayed on my monitor by the HSL picker. >>>>>>> But everyone interprets colors differently. >>>>>>> >>>>>>> If you write the same value for yellow or white on a droid 4 and the N900 do they produce the same color side by side? >>>>>>> Most probably not. >>>>>>> >>>>>>> As you pointed out the PWM needs to be modified to obtain the correct white color to account for LED and other device constraints. >>>>>>> >>>>>>> But we need to take into account the light pipe. Pools nowadays have RGB LED spot lights in them. It can >>>>>>> be set to white. On my pool right off the lens the color has a purplish hue to it. As the light is diffracted into >>>>>>> the pool the color becomes white. The pool is clear. When I add chemicals to the pool and make it cloudy >>>>>>> and turn on the lights the color off the lens is now white. This is an example on a large scale but the issue >>>>>>> scales down to the hand helds and smart home applications. >>>>>>> >>>>>>> If the cluster is piped through a flexible optic 0xffffff may produce the "white" you want on its output. >>>>>>> >>>>>>> So an expectation of certain color without proper piping based on a single RGB value may be a little unreasonable. >>>>>>> There may need to be a way to attenuate the values based on the hardware aspect of the equation ie light pipe (or lack thereof) and LED vendor. >>>>>>> So if we write 0xffffff to the RGB driver the driver could adjust the intensity of the individual LEDs based on the diffraction >>>>>>> coefficients. >>>>>>> >>>>>>> I also think that is an unreasonable expectation here that writing a single value to any LED RGB driver would produce >>>>>>> a "rest of the world" absolute color. Maybe it can produce something similar but not identical. >>>>>>> As you indicated in the requirements there is more involved here then just the LED and the values written. >>>>>>> The colors should be close but may not be identical. >>>>>>> >>>>>>> A 10 year old N900 should not be considered the gold standard for color production due to advancements in LED, >>>>>>> light pipe and LED driver technology. >>>>>>> The single package RGB clusters on the board I am testing is about the size of a single RGB LED from 10 years ago. >>>>>>> >>>>>>> I agree that the interface developed should work on the device but the algorithm derived to obtain the color needs to have >>>>>>> a hardware aspect to the calculation. >>>>>>> >>>>>>>>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>>>>>>>> >>>>>>>>> Monitors are not an application for this part. >>>>>>>> >>>>>>>> You did not answer the question. When you talk about yellow, is it >>>>>>>> same yellow the rest of world talks about? >>>>>>>> >>>>>>> >>>>>>> See above. It is close to what was on my monitor displayed. >>>>>>> >>>>>>>>>> Because 100% PWM on all channels does not result in white on hardware >>>>>>>>>> I have. >>>>>>>>> >>>>>>>>> I don't know I am usually blinded by the light and have no diffuser over >>>>>>>>> the LEDs to disperse the light so when I look I see all 3 colors. >>>>>>>> >>>>>>>> How can we have useful discussion about colors when you don't see the >>>>>>>> colors? >>>>>>>> >>>>>>>> Place a piece of paper over the LEDs.... >>>>>>>> >>>>>>> >>>>>>> Good suggestion for a rough test. >>>>>>> >>>>>>>>>> But... >>>>>>>>>> >>>>>>>>>> I believe we should have a reasonable design before we do something >>>>>>>>>> like this. There's no guarantee someone will not use lp50xx with just >>>>>>>>>> the white LEDs for example. How will this work? Plus existing hardware >>>>>>>>>> already uses three separate LEDs for RGB LED. Why not provide same >>>>>>>>>> interface? >>>>>>>>> >>>>>>>>> Which existing hardware? Are they using this part? >>>>>>>> >>>>>>>> Nokia N900. They are not using this part, but any interface we invent >>>>>>>> should work there, too. >>>>>>>> >>>>>>> >>>>>>> Yes a common interface would be nice with some sort of hardware tuning coefficient. >>>>>>> >>>>>>>>> <rant> >>>>>>>>> Why are we delaying getting the RGB framework or HSV in? >>>>>>>>> I would rather design against something you want instead of having >>>>>>>>> everyone complain about every implementation I post. >>>>>>>>> </rant> >>>>>>>> >>>>>>>> Because you insist on creating new kernel interfaces, when existing >>>>>>>> interfaces work, and are doing that badly. >>>>>>>> >>>>>>>> Because your patches are of lower quality than is acceptable for linux >>>>>>>> kernel. >>>>>>>> >>>>>>>> Because you don't seem to be reading the emails. >>>>>>>> >>>>>>>> I sent list of requirements for RGB led support. This does not meet >>>>>>>> them. >>>>>>>> >>>>>>> >>>>>>> Sigh. You did not answer my question. >>>>>>> >>>>>>> Your requirements seem to be centered around monitors but that is only one application of the current >>>>>>> RGB LED landscape. >>>>>>> >>>>>>> I am willing to work with you on the HSV and adapting the LP50xx part to this framework. >>>>>>> Or any RGB framework for that matter. I still don't agree with the kernel needing to declare colors >>>>>>> maybe color capabilities but not specific colors. >>>>>> >>>>>> Dan, if you have a bandwidth for LED RGB class implementation >>>>>> then please go ahead. It would be good to compare colors produced >>>>>> by software HSV->RGB algorithm to what can be achieved with >>>>>> LEDn_BRIGHTNESS feature. >>>>>> >>>>>> The requirements for LED RGB class as I would see it: >>>>>> >>>>>> sysfs interface: >>>>>> >>>>>> brightness-model: space separated list of available options: >>>>>> - rgb (default): >>>>>> - creates color file with "red green blue" decimal values >>>>> >>>>> What about other colored LEDs? Presenting RGB for an Amber LED does not seem right. >>>>> Should the LED color come from the DT? >>>>> >>>> >>>> I thought about this, other non-RGB LEDs would not use the RGB framework. >>>> But should they have the same interfaces as RGB? >>>> >>>> Should PWM control be a global interface? >>> >>> In order to being able to set multi color element led at one go I would recommend using then model: >>> >>> color_names: "red green blue white" >>> >>> echo "32 43 0 128" > color >>> >>> This way all elements would be set at same time from user space point of view. >>> >>> This of course requires that it is part of the physical/logical led that is being controlled. If it is a separate thing then it would logically be differently controlled mono color led. >>> >>> If you look what kinds of leds are available lets say from digikey you get all kinds of combos: >>> >>> - red, green, blue >>> - red, green, blue, amber >>> - red, green, blue, white >>> - red, green, blue, white - cool >>> - red, green, blue, white - neutral >>> - red, green, blue, white - warm >>> - red, orange >>> - purple, ultraviolet >>> - amber, blue >>> - amber, blue, cyan, green, red, violet, white - cool >>> - amber, blue, green >>> - amber, green, blue >>> - and then lots of single special colors >> >> It suggested me another solution. Instead of LED RGB class >> we would have LED multi-color class. >> > > I was thinking the same thing this morning. But I was thinking that the RGB > class should be an additional class to stand on its own or can register to the > multi color class. > >> Sysfs interface design: >> ----------------------- >> >> colors: directory containing files that map to >> the brightness of particular LEDs; there >> would be predefined color names that LED class >> driver should map iouts to, e.g.: > > Filling in the missing ideas with questions. > > Is it a directory or a file? If it is a directory does that not break the current > directory label model? > > so the path would be /sys/class/leds/colors ? (It is probably not this but needs clarification) > How would this look if I had 2 of the same color LEDs? The Beagle bone black has 2 Blue LEDs. > They are assigned to different triggers and have different directories. Both are GPIO controlled. > > Or are you saying it would be something like (More then likely this is what you intended) > /sys/class/leds/input4::numlock/colors? Yes, this is what I meant. > Maybe it is mandated that "multi" be added to the label when the class is registered so the caller > knows that this is a multi color class and not a single LED color class. Like I am going to come up with standardized color names in my LED naming related patch, the multi-color names can be defined as well, e.g.: rgb, rgbw, rgbwa, rgbwauv etc. > What about providing a file called colors_raw which takes in the raw decimal values to obtain other color > variants when RGB is only available? And this can also present a single write to the kernel with the new > color values. My design already covers that in the form of files in the colors directory. Like I stated: "files that map to the brightness of particular LEDs". Single write is secured by "echo "write" > sync. > I am not a fan of hard coding preset colors as you can see there are to many of them and variations of the color. > In addition this severely limits the ability of the user. Unless we stick to primary colors only and not secondary > colors. > Defining and hard coding hte colors can get out of control and not maintainable moving forward. Especially > if we start adding defines like white_warm, white_neutral and other variations to the color. We would not limit anything. Every color combination can be achieved by following this exemplary sequence: $ echo 154 > colors/red $ echo 232 > colors/green $ echo 43 > colors/blue # echo "write" > sync //only this changes hardware state brightness-model is provided only to define mapping of legacy brightness levels (governed by brightness file and led_set_brightness() API) to the specific combination of colors. For instance we can define three brightness levels for green hue: DT definition for it would look like below: rgb-green = <0x277c33 0x37b048 0x47e45d>; LED multi color class would then do the following mapping for each of the three brightness levels for rgb-green brightness model: $ echo rgb-green > brightness_model $ echo 1 > brightness // red=0x27, green=0x7c, blue=0x33 $ echo 2 > brightness // red=0x37, green=0xb0, blue=0x48 $ echo 3 > brightness // red=0x47, green=0xe4, blue=0x5d > What about IR LEDs used for night vision products? Do these fall into the multi color class? > We do have a driver I submitted that had an IR LED and a White LED combined. It was created against the > flash class but it could be a multi color LED as well. > > How would traversing through the full color palette work if you were to want to produce a multi > color ring like the LP50xx where the pattern can traverse from one end of the color spectrum and back? > Or in a product like the gaming keyboards that will change color or change backlight brightness? This is not meant as a solution for pattern generator but for consolidated source of multi color light. In the simplest case RGB LED elements like those used for lp5024 reference board, but it could be RGBWAUV LED [0] as well. For patterns traversing many LEDs I see a trigger as the best solution. Hmm, now I see that trigger mechanism actually can serve as very flexible pattern generator. We would need a device that could be configured to register a number of multi-led-patternN triggers, one per LED, and generate events for each trigger in a loop. The device would have to allow for configuring pattern intervals via sysfs, like in case of current pattern trigger. LED class devices would have to register for its events: $/sys/class/leds/led1 echo multi-led-pattern1 > trigger $/sys/class/leds/led2 echo multi-led-pattern2 > trigger $/sys/class/leds/led3 echo multi-led-pattern3 > trigger The ability to define brightness models in DT would add even more flexibility. > Not sure what color LEDs the keyboard manufacturers place on their keyboards but does the interface design > capable of doing this? > > https://www.youtube.com/watch?v=pfKv3g2FeBE > or something like this > > https://www.youtube.com/watch?v=PO3auX3f5C4 > > The LP5036 has this capability. > >> - red >> - green >> - blue >> - white >> - sync: accepts "write" and "read", which executes >> write/readout to/from a device respectively. >> > > What are these above, the values or the files under the colors directory? They are just color specific counterparts of monochrome brightness. In terms of lp50xx they would map to OUTn_COLOR registers. > I am assuming they are files. Right. > Are they mandatory or optional? Mandatory, one per each iout to control. >> brightness-model: defines brightness level to color configuration >> mapping >> - "hardware": for devices with feature like LEDn_BRIGHTNESS of lp50xx >> - "rgb-<hue>": available only when all three red,green,blue colors >> are present in the colors directory. >> <hue> is a placeholder for given DT hue presets. >> - "rgbw-<hue>": Same as above, but available only when white color >> (any of amber or white-{cool|neutral|warm} can be >> mapped to white) is also available. In this mode >> max_brightness equals num-of-hue-presets + 1, and >> max_brightness, when set, turns the "white" LED on > > Why do we need white combined here? Should this not be its own entity? To be able to set white color. We're still talking about one LED element (although they can be be physically few LEDs in one case). This is brightness file, so we've got to stick to the semantics. Max brightness level should be the brightest. With RGBW LEDs we fortunately have a means to achieve pure white, that's why rgbw-<hue> would be beneficial. If you increase L component of HSL color space, the max value gives white for all hues. So maybe this brightness-model would be rather called hsl-<hue>. For RGBW LEDs, we would have to allow for more shades of white too, like in [1]. > Again I don't like limiting the color palette from the DT. I think that the > user space can see what colors are available for that device and makes its own > decision on what color to present. > > For the RGBw what about RGB amber and RGB purple. Are the white LEDs always part of the > same function trying to be achieved by the system designer? The RGB can be used to denote > notification status and the white can be used to denote that a charger is connected. Motorola > Droid did this. I hope I've just clarified my idea. > >> - "rgb-linear": I'm not sure if it should be available - it will >> have unpredictable results >> >> brightness: sets/reads brightness in the way specific to the >> current brightness-model. When more colors are available >> (e.g. amber, blue, cyan, green, red, violet, white), they >> are not touched by write to brightness). >> >> HSV->RGB conversion is left entirely to the userspace, which can set any >> color with use of the proposed interface. >> >> Let's agree on sysfs interface first, and only after that move >> to the DT details. >> > > DT's are meant to describe hardware and not describe the product. Unless Rob does not see > an issue with defining product capabilities in the DT then we should keep that out of the DT. LED element is a device. I see nothing irrelevant for DT in describing the lighting specificity of a device mounted on the board. Please keep in mind that it will not limit the number of colors available to set. It will only allow to define mapping of brightness level to color. We need that for current trigger mechanism to work with LED multi-color class. [0] http://www.cobledarray.com/sale-4942931-10w-rgbwauv-led-diode-6-in-1-high-power-multicolor-led-chip.html [1] https://www.youtube.com/watch?v=NzlFmTqOh9M -- Best regards, Jacek Anaszewski
Dan, On 1/24/19 9:32 PM, Dan Murphy wrote: > Jacek > > Replying to code comments. > > On 1/15/19 3:47 PM, Jacek Anaszewski wrote: >> Hi Da, >> >> Thank you for the v2. >> > > I will probably submit v3 outside the realm of the multi color framework. > We can always convert as Pavel pointed out. > >> I have some remarks below. >> >> On 1/14/19 10:17 PM, Dan Murphy wrote: >>> Introduce the LP5036/30/24/18 RGB LED driver. >>> The difference in these parts are the number of >>> LED outputs where the: >>> >>> LP5036 can control 36 LEDs >>> LP5030 can control 30 LEDs >>> LP5024 can control 24 LEDs >>> LP5018 can control 18 LEDs >>> >>> 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 <dmurphy@ti.com> >>> --- >>> >>> v2 - Changed the mix and module files to a single "color" file, added the LP5030 >>> and LP5036 register mapping, added ABI documentation, updated the parsing of >>> DT and led sources to match DT, renamed driver to leds-lp50xx.c - https://lore.kernel.org/patchwork/patch/1026515/ >>> >>> Documentation/leds/leds-lp50xx.txt | 36 ++ >>> drivers/leds/Kconfig | 7 + >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-lp50xx.c | 754 +++++++++++++++++++++++++++++ >>> 4 files changed, 798 insertions(+) >>> create mode 100644 Documentation/leds/leds-lp50xx.txt >>> create mode 100644 drivers/leds/leds-lp50xx.c >>> >>> diff --git a/Documentation/leds/leds-lp50xx.txt b/Documentation/leds/leds-lp50xx.txt >> >> Please move it to >> >> Documentation/ABI/testing/sysfs-class-led-driver-lp50xx >> >> and use standard ABI documentation format. >> > > Ack. I will add this file as well per the doc format. > >> >>> new file mode 100644 >>> index 000000000000..8b1b01dfdd22 >>> --- /dev/null >>> +++ b/Documentation/leds/leds-lp50xx.txt >>> @@ -0,0 +1,36 @@ >>> +LP5018/LP5024/LP5030/LP5036 Common Driver >>> +================================================= >>> + >>> +Authors: Dan Murphy <dmurphy@ti.com> >>> + >>> +Description >>> +----------- >>> +The LP50XX RGB LED drivers have the ability to group multiple RGB cluster >>> +LEDs into a single group for simultaneous control or expose single RGB cluster >>> +for control. This device exposes different register interfaces to control >>> +the cluster brightness as well as the individual RGB LEDs color intensity. >>> + >>> +RGB Cluster Color Control >>> +------------------------- >>> +The LP50xx driver will expose a file called "color" for each LED class instance >>> +defined. This file will accept a 24-bit RGB value in which the the color of the >>> +RGB LEDs will be set. >>> + >>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>> +XX - Do not care ignored by the driver >>> +RR - is the 8 bit Red LED value >>> +GG - is the 8 bit Green LED value >>> +BB - is the 8 bit Blue LED value >>> + >>> +Example: >>> +LED module output 4 of the LP5024 will be a yellow color: >>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>> + >>> +LED module output 4 of the LP5024 will be dimmed 50%: >>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>> + >>> +LED banked RGBs of the LP5036 will be a white color: >>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >> >> This part with example cans remain in Documentation/leds if you like. >> >>> + >>> +LED banked RGBs of the LP50364 will be dimmed 50%: >>> +echo 0x80 > /sys/class/leds/lp5036\:led_banked/brightness >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index a72f97fca57b..5f413445a667 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_LP50XX >>> + tristate "LED Support for TI LP5036/30/24/18 LED driver chip" >>> + depends on LEDS_CLASS && REGMAP_I2C >>> + help >>> + If you say yes here you get support for the Texas Instruments >>> + LP5036, LP5030, 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..852eff0b773f 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_LP50XX) += leds-lp50xx.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-lp50xx.c b/drivers/leds/leds-lp50xx.c >>> new file mode 100644 >>> index 000000000000..41bb2e0129c8 >>> --- /dev/null >>> +++ b/drivers/leds/leds-lp50xx.c >>> @@ -0,0 +1,754 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* TI LP50XX LED chip family driver >>> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ >>> + */ >> >> Let's use uniform "//" comment style here. > > Ack > >> >>> +#include <linux/gpio/consumer.h> >>> +#include <linux/i2c.h> >>> +#include <linux/init.h> >>> +#include <linux/leds.h> >>> +#include <linux/module.h> >>> +#include <linux/mutex.h> >>> +#include <linux/of.h> >>> +#include <linux/of_gpio.h> >>> +#include <linux/regmap.h> >>> +#include <linux/regulator/consumer.h> >>> +#include <linux/slab.h> >>> +#include <uapi/linux/uleds.h> >>> + >>> +#define LP50XX_DEV_CFG0 0x00 >>> +#define LP50XX_DEV_CFG1 0x01 >>> +#define LP50XX_LED_CFG0 0x02 >>> + >>> +/* LP5018 and LP5024 registers */ >>> +#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 >>> + >>> +/* LP5030 and LP5036 registers */ >>> +#define LP5036_LED_CFG1 0x03 >>> +#define LP5036_BNK_BRT 0x04 >>> +#define LP5036_BNKA_CLR 0x05 >>> +#define LP5036_BNKB_CLR 0x06 >>> +#define LP5036_BNKC_CLR 0x07 >>> +#define LP5036_LED0_BRT 0x08 >>> +#define LP5036_LED1_BRT 0x09 >>> +#define LP5036_LED2_BRT 0x0a >>> +#define LP5036_LED3_BRT 0x0b >>> +#define LP5036_LED4_BRT 0x0c >>> +#define LP5036_LED5_BRT 0x0d >>> +#define LP5036_LED6_BRT 0x0e >>> +#define LP5036_LED7_BRT 0x0f >>> +#define LP5036_LED8_BRT 0x10 >>> +#define LP5036_LED9_BRT 0x11 >>> +#define LP5036_LED10_BRT 0x12 >>> +#define LP5036_LED11_BRT 0x13 >>> + >>> +#define LP5036_OUT0_CLR 0x14 >>> +#define LP5036_OUT1_CLR 0x15 >>> +#define LP5036_OUT2_CLR 0x16 >>> +#define LP5036_OUT3_CLR 0x17 >>> +#define LP5036_OUT4_CLR 0x18 >>> +#define LP5036_OUT5_CLR 0x19 >>> +#define LP5036_OUT6_CLR 0x1a >>> +#define LP5036_OUT7_CLR 0x1b >>> +#define LP5036_OUT8_CLR 0x1c >>> +#define LP5036_OUT9_CLR 0x1d >>> +#define LP5036_OUT10_CLR 0x1e >>> +#define LP5036_OUT11_CLR 0x1f >>> +#define LP5036_OUT12_CLR 0x20 >>> +#define LP5036_OUT13_CLR 0x21 >>> +#define LP5036_OUT14_CLR 0x22 >>> +#define LP5036_OUT15_CLR 0x23 >>> +#define LP5036_OUT16_CLR 0x24 >>> +#define LP5036_OUT17_CLR 0x25 >>> +#define LP5036_OUT18_CLR 0x26 >>> +#define LP5036_OUT19_CLR 0x27 >>> +#define LP5036_OUT20_CLR 0x28 >>> +#define LP5036_OUT21_CLR 0x29 >>> +#define LP5036_OUT22_CLR 0x2a >>> +#define LP5036_OUT23_CLR 0x2b >>> +#define LP5036_OUT24_CLR 0x2c >>> +#define LP5036_OUT25_CLR 0x2d >>> +#define LP5036_OUT26_CLR 0x2e >>> +#define LP5036_OUT27_CLR 0x2f >>> +#define LP5036_OUT28_CLR 0x30 >>> +#define LP5036_OUT29_CLR 0x31 >>> +#define LP5036_OUT30_CLR 0x32 >>> +#define LP5036_OUT31_CLR 0x33 >>> +#define LP5036_OUT32_CLR 0x34 >>> +#define LP5036_OUT33_CLR 0x35 >>> +#define LP5036_OUT34_CLR 0x36 >>> +#define LP5036_OUT35_CLR 0x37 >>> +#define LP5036_RESET 0x38 >>> + >>> +#define LP50XX_SW_RESET 0xff >>> + >>> +#define LP50XX_CHIP_EN BIT(6) >>> + >>> +#define LP5018_MAX_LED_STRINGS 6 >>> +#define LP5024_MAX_LED_STRINGS 8 >>> +#define LP5030_MAX_LED_STRINGS 10 >>> +#define LP5036_MAX_LED_STRINGS 12 >>> + >>> +enum lp50xx_model { >>> + LP5018, >>> + LP5024, >>> + LP5030, >>> + LP5036, >>> +}; >>> + >>> +struct lp50xx_led { >>> + u32 led_strings[LP5036_MAX_LED_STRINGS]; >> >> It is possible to have only one bank, so this can be a property >> of struct lp50xx. Moreover, it doesn't need to be an array, >> but should be: >> >> unsigned long bank_modules; >> >> Then you will be able to use bitops on it, where bit position will >> refer to the id of RGB LED module assigned to the bank. >> > > ACK. Had to think a bit on this to make sure there was enough room > but aligning on LEDn_MODULES/BANKS should be fine > >>> + char label[LED_MAX_NAME_SIZE]; >>> + struct led_classdev led_dev; >>> + struct lp50xx *priv; >>> + int led_number; >>> + u8 ctrl_bank_enabled; >>> +}; >>> + >>> +/** >>> + * struct lp50xx - >>> + * @enable_gpio: Hardware enable gpio >>> + * @regulator: LED supply regulator pointer >>> + * @client: Pointer to the I2C client >>> + * @regmap: Devices register map >>> + * @dev: Pointer to the devices device struct >>> + * @lock: Lock for reading/writing the device >>> + * @model_id: ID of the device >>> + * @leds: Array of LED strings >> >> Please don't use capital letters for property description. >> Still, some of the properties below remain undocumented. >> > > You are referring to ID and I2C or do you mean no capitals to start the > description? The latter. > Yes I added the properties and missed documenting them. > >>> + */ >>> +struct lp50xx { >>> + struct gpio_desc *enable_gpio; >>> + struct regulator *regulator; >>> + struct i2c_client *client; >>> + struct regmap *regmap; >>> + struct device *dev; >>> + struct mutex lock; >>> + enum lp50xx_model model_id; >>> + int max_leds; >>> + int num_of_leds; >>> + >>> + u8 led_brightness0_reg; >>> + u8 mix_out0_reg; >>> + u8 bank_brt_reg; >>> + u8 bank_mix_reg; >>> + u8 reset_reg; >>> + >>> + /* This needs to be at the end of the struct */ >>> + struct lp50xx_led leds[]; >>> +}; >>> + >>> +static const struct reg_default lp5024_reg_defs[] = { >>> + {LP50XX_DEV_CFG0, 0x0}, >>> + {LP50XX_DEV_CFG1, 0x3c}, >>> + {LP50XX_LED_CFG0, 0x0}, >>> + {LP5024_BNK_BRT, 0xff}, >>> + {LP5024_BNKA_CLR, 0x0f}, >>> + {LP5024_BNKB_CLR, 0x0f}, >>> + {LP5024_BNKC_CLR, 0x0f}, >>> + {LP5024_LED0_BRT, 0x0f}, >>> + {LP5024_LED1_BRT, 0xff}, >>> + {LP5024_LED2_BRT, 0xff}, >>> + {LP5024_LED3_BRT, 0xff}, >>> + {LP5024_LED4_BRT, 0xff}, >>> + {LP5024_LED5_BRT, 0xff}, >>> + {LP5024_LED6_BRT, 0xff}, >>> + {LP5024_LED7_BRT, 0xff}, >>> + {LP5024_OUT0_CLR, 0x0f}, >>> + {LP5024_OUT1_CLR, 0x00}, >>> + {LP5024_OUT2_CLR, 0x00}, >>> + {LP5024_OUT3_CLR, 0x00}, >>> + {LP5024_OUT4_CLR, 0x00}, >>> + {LP5024_OUT5_CLR, 0x00}, >>> + {LP5024_OUT6_CLR, 0x00}, >>> + {LP5024_OUT7_CLR, 0x00}, >>> + {LP5024_OUT8_CLR, 0x00}, >>> + {LP5024_OUT9_CLR, 0x00}, >>> + {LP5024_OUT10_CLR, 0x00}, >>> + {LP5024_OUT11_CLR, 0x00}, >>> + {LP5024_OUT12_CLR, 0x00}, >>> + {LP5024_OUT13_CLR, 0x00}, >>> + {LP5024_OUT14_CLR, 0x00}, >>> + {LP5024_OUT15_CLR, 0x00}, >>> + {LP5024_OUT16_CLR, 0x00}, >>> + {LP5024_OUT17_CLR, 0x00}, >>> + {LP5024_OUT18_CLR, 0x00}, >>> + {LP5024_OUT19_CLR, 0x00}, >>> + {LP5024_OUT20_CLR, 0x00}, >>> + {LP5024_OUT21_CLR, 0x00}, >>> + {LP5024_OUT22_CLR, 0x00}, >>> + {LP5024_OUT23_CLR, 0x00}, >>> + {LP5024_RESET, 0x00} >>> +}; >>> + >>> +static const struct reg_default lp5036_reg_defs[] = { >>> + {LP50XX_DEV_CFG0, 0x0}, >>> + {LP50XX_DEV_CFG1, 0x3c}, >>> + {LP50XX_LED_CFG0, 0x0}, >>> + {LP5036_LED_CFG1, 0x0}, >>> + {LP5036_BNK_BRT, 0xff}, >>> + {LP5036_BNKA_CLR, 0x0f}, >>> + {LP5036_BNKB_CLR, 0x0f}, >>> + {LP5036_BNKC_CLR, 0x0f}, >>> + {LP5036_LED0_BRT, 0x0f}, >>> + {LP5036_LED1_BRT, 0xff}, >>> + {LP5036_LED2_BRT, 0xff}, >>> + {LP5036_LED3_BRT, 0xff}, >>> + {LP5036_LED4_BRT, 0xff}, >>> + {LP5036_LED5_BRT, 0xff}, >>> + {LP5036_LED6_BRT, 0xff}, >>> + {LP5036_LED7_BRT, 0xff}, >>> + {LP5036_OUT0_CLR, 0x0f}, >>> + {LP5036_OUT1_CLR, 0x00}, >>> + {LP5036_OUT2_CLR, 0x00}, >>> + {LP5036_OUT3_CLR, 0x00}, >>> + {LP5036_OUT4_CLR, 0x00}, >>> + {LP5036_OUT5_CLR, 0x00}, >>> + {LP5036_OUT6_CLR, 0x00}, >>> + {LP5036_OUT7_CLR, 0x00}, >>> + {LP5036_OUT8_CLR, 0x00}, >>> + {LP5036_OUT9_CLR, 0x00}, >>> + {LP5036_OUT10_CLR, 0x00}, >>> + {LP5036_OUT11_CLR, 0x00}, >>> + {LP5036_OUT12_CLR, 0x00}, >>> + {LP5036_OUT13_CLR, 0x00}, >>> + {LP5036_OUT14_CLR, 0x00}, >>> + {LP5036_OUT15_CLR, 0x00}, >>> + {LP5036_OUT16_CLR, 0x00}, >>> + {LP5036_OUT17_CLR, 0x00}, >>> + {LP5036_OUT18_CLR, 0x00}, >>> + {LP5036_OUT19_CLR, 0x00}, >>> + {LP5036_OUT20_CLR, 0x00}, >>> + {LP5036_OUT21_CLR, 0x00}, >>> + {LP5036_OUT22_CLR, 0x00}, >>> + {LP5036_OUT23_CLR, 0x00}, >>> + {LP5036_OUT24_CLR, 0x00}, >>> + {LP5036_OUT25_CLR, 0x00}, >>> + {LP5036_OUT26_CLR, 0x00}, >>> + {LP5036_OUT27_CLR, 0x00}, >>> + {LP5036_OUT28_CLR, 0x00}, >>> + {LP5036_OUT29_CLR, 0x00}, >>> + {LP5036_OUT30_CLR, 0x00}, >>> + {LP5036_OUT31_CLR, 0x00}, >>> + {LP5036_OUT32_CLR, 0x00}, >>> + {LP5036_OUT33_CLR, 0x00}, >>> + {LP5036_OUT34_CLR, 0x00}, >>> + {LP5036_OUT35_CLR, 0x00}, >>> + {LP5036_RESET, 0x00} >>> +}; >>> + >>> +static const struct regmap_config lp5024_regmap_config = { >>> + .reg_bits = 8, >>> + .val_bits = 8, >>> + >>> + .max_register = LP5024_RESET, >>> + .reg_defaults = lp5024_reg_defs, >>> + .num_reg_defaults = ARRAY_SIZE(lp5024_reg_defs), >>> + .cache_type = REGCACHE_RBTREE, >>> +}; >>> + >>> +static const struct regmap_config lp5036_regmap_config = { >>> + .reg_bits = 8, >>> + .val_bits = 8, >>> + >>> + .max_register = LP5036_RESET, >>> + .reg_defaults = lp5036_reg_defs, >>> + .num_reg_defaults = ARRAY_SIZE(lp5036_reg_defs), >>> + .cache_type = REGCACHE_RBTREE, >>> +}; >>> + >>> +static ssize_t color_show(struct device *dev, >>> + struct device_attribute *attr, >>> + char *buf) >>> +{ >>> + struct led_classdev *led_cdev = dev_get_drvdata(dev); >>> + struct lp50xx_led *led = container_of(led_cdev, struct lp50xx_led, >>> + led_dev); >>> + struct lp50xx *priv = led->priv; >>> + unsigned int red_val, green_val, blue_val; >>> + u8 red_reg, green_reg, blue_reg; >>> + u32 mix_value = 0; >>> + u8 led_offset; >>> + int ret; >>> + >>> + if (led->ctrl_bank_enabled) { >>> + red_reg = priv->bank_mix_reg; >>> + green_reg = priv->bank_mix_reg + 1; >>> + blue_reg = priv->bank_mix_reg + 2; >>> + } else { >>> + led_offset = (led->led_number * 3); >>> + red_reg = priv->mix_out0_reg + led_offset; >>> + green_reg = priv->mix_out0_reg + led_offset + 1; >>> + blue_reg = priv->mix_out0_reg + led_offset + 2; >>> + } >>> + >>> + ret = regmap_read(priv->regmap, red_reg, &red_val); >>> + if (ret) { >>> + dev_err(&priv->client->dev, "Cannot read LED value\n"); >>> + goto out; >>> + } >>> + >>> + ret = regmap_read(priv->regmap, green_reg, &green_val); >>> + if (ret) { >>> + dev_err(&priv->client->dev, "Cannot read LED value\n"); >>> + goto out; >>> + } >>> + >>> + ret = regmap_read(priv->regmap, blue_reg, &blue_val); >>> + if (ret) { >>> + dev_err(&priv->client->dev, "Cannot read LED value\n"); >>> + goto out; >>> + } >>> + >>> + mix_value = (red_val << 16 | green_val << 8 | blue_val); >>> + >>> +out: >>> + return scnprintf(buf, PAGE_SIZE, "0x%X\n", mix_value); >>> +} >>> + >>> +static ssize_t color_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t size) >>> +{ >>> + struct led_classdev *led_cdev = dev_get_drvdata(dev); >>> + struct lp50xx_led *led = container_of(led_cdev, struct lp50xx_led, >>> + led_dev); >>> + struct lp50xx *priv = led->priv; >>> + u8 led_offset; >>> + unsigned long mix_value; >>> + u8 red_reg, green_reg, blue_reg; >>> + u8 red_val, green_val, blue_val; >>> + int ret; >>> + >>> + ret = kstrtoul(buf, 0, &mix_value); >>> + if (ret) >>> + return ret; >>> + >>> + if (led->ctrl_bank_enabled) { >>> + red_reg = priv->bank_mix_reg; >>> + green_reg = priv->bank_mix_reg + 1; >>> + blue_reg = priv->bank_mix_reg + 2; >>> + } else { >>> + led_offset = (led->led_number * 3); >>> + red_reg = priv->mix_out0_reg + led_offset; >>> + green_reg = priv->mix_out0_reg + led_offset + 1; >>> + blue_reg = priv->mix_out0_reg + led_offset + 2; >>> + } >>> + >>> + red_val = (mix_value & 0xff0000) >> 16; >>> + green_val = (mix_value & 0xff00) >> 8; >>> + blue_val = (mix_value & 0xff); >> >> I've been rather thinking about space separated list of decimal >> "red green blue" values, but maybe this way it will be less >> controversial. Let's if there will be other opinions. >> > > Has anything changed based on this? > Should I change the file name from "color" to something else? Yes, I'd stick to my most recent proposal: "colors" directory with red,green,blue files mapped to IOUTn_COLOR register, and the file "sync" in this directory accepting "write" and "read" strings. We could think of some alternatives for the "colors" directory and "sync" file names. s/colors/color_intensities/ ? s/sync/rw ? For rw case we could have values "1" for write and "0" for read. >>> + >>> + ret = regmap_write(priv->regmap, red_reg, red_val); >>> + if (ret) { >>> + dev_err(&priv->client->dev, "Cannot write LED value\n"); >>> + goto out; >>> + } >>> + >>> + ret = regmap_write(priv->regmap, green_reg, green_val); >>> + if (ret) { >>> + dev_err(&priv->client->dev, "Cannot write LED value\n"); >>> + goto out; >>> + } >>> + >>> + ret = regmap_write(priv->regmap, blue_reg, blue_val); >>> + if (ret) { >>> + dev_err(&priv->client->dev, "Cannot write LED value\n"); >>> + goto out; >>> + } >>> +out: >>> + return size; >>> +} >>> + >>> +static DEVICE_ATTR_RW(color); >>> + >>> +static struct attribute *lp50xx_led_color_attrs[] = { >>> + &dev_attr_color.attr, >>> + NULL >>> +}; >>> +ATTRIBUTE_GROUPS(lp50xx_led_color); >>> + >>> +static int lp50xx_brightness_set(struct led_classdev *led_cdev, >>> + enum led_brightness brt_val) >>> +{ >>> + struct lp50xx_led *led = container_of(led_cdev, struct lp50xx_led, >>> + led_dev); >>> + int ret = 0; >>> + u8 reg_val; >>> + >>> + mutex_lock(&led->priv->lock); >>> + >>> + if (led->ctrl_bank_enabled) >>> + reg_val = led->priv->bank_brt_reg; >>> + else >>> + reg_val = led->priv->led_brightness0_reg + led->led_number; >>> + >>> + ret = regmap_write(led->priv->regmap, reg_val, brt_val); >>> + >>> + mutex_unlock(&led->priv->lock); >>> + >>> + return ret; >>> +} >>> + >>> +static enum led_brightness lp50xx_brightness_get(struct led_classdev *led_cdev) >>> +{ >>> + struct lp50xx_led *led = container_of(led_cdev, struct lp50xx_led, >>> + led_dev); >>> + unsigned int brt_val; >>> + u8 reg_val; >>> + int ret; >>> + >>> + mutex_lock(&led->priv->lock); >>> + >>> + if (led->ctrl_bank_enabled) >>> + reg_val = led->priv->bank_brt_reg; >>> + else >>> + reg_val = led->priv->led_brightness0_reg + led->led_number; >>> + >>> + ret = regmap_read(led->priv->regmap, reg_val, &brt_val); >>> + >>> + mutex_unlock(&led->priv->lock); >>> + >>> + return brt_val; >>> +} >>> + >>> +static void lp50xx_set_led_values(struct lp50xx *priv) >>> +{ >>> + if (priv->model_id == LP5018 || priv->model_id == LP5024) { >>> + priv->led_brightness0_reg = LP5024_LED0_BRT; >>> + priv->mix_out0_reg = LP5024_OUT0_CLR; >>> + priv->bank_brt_reg = LP5024_BNK_BRT; >>> + priv->bank_mix_reg = LP5024_BNKA_CLR; >>> + priv->reset_reg = LP5024_RESET; >>> + } else { >>> + priv->led_brightness0_reg = LP5036_LED0_BRT; >>> + priv->mix_out0_reg = LP5036_OUT0_CLR; >>> + priv->bank_brt_reg = LP5036_BNK_BRT; >>> + priv->bank_mix_reg = LP5036_BNKA_CLR; >>> + priv->reset_reg = LP5036_RESET; >>> + } >>> +} >>> + >>> +static int lp50xx_set_banks(struct lp50xx *priv) >>> +{ >>> + struct lp50xx_led *led; >>> + u8 led_ctrl_enable = 0; >>> + u8 led1_ctrl_enable = 0; >>> + u8 ctrl_ext = 0; >>> + int i, j; >>> + int ret; >>> + >>> + for (i = 0; i <= priv->num_of_leds; i++) { >>> + led = &priv->leds[i]; >>> + if (!led->ctrl_bank_enabled) >>> + continue; >>> + >>> + for (j = 0; j <= priv->max_leds - 1; j++) { >>> + if (led->led_strings[j] > (LP5024_MAX_LED_STRINGS - 1)) { >>> + ctrl_ext = led->led_strings[j] - LP5024_MAX_LED_STRINGS; >>> + led1_ctrl_enable |= (1 << ctrl_ext); >>> + } else { >>> + led_ctrl_enable |= (1 << led->led_strings[j]); >>> + } >>> + } >>> + } >> >> With centralized bank_modules flags it should look simpler. >> > > I don't think so. The LP5030 and LP5036 have 2 registers to denote Module vs banked. > > But once I convert it we will know. > >>> + >>> + ret = regmap_write(priv->regmap, LP50XX_LED_CFG0, led_ctrl_enable); >>> + >>> + if (led1_ctrl_enable) >>> + ret = regmap_write(priv->regmap, LP5036_LED_CFG1, >>> + led1_ctrl_enable); >>> + >>> + return ret; >>> +} >>> + >>> +static int lp50xx_init(struct lp50xx *priv) >>> +{ >>> + int ret; >>> + >>> + lp50xx_set_led_values(priv); >>> + >>> + if (priv->enable_gpio) { >>> + gpiod_direction_output(priv->enable_gpio, 1); >>> + } else { >>> + ret = regmap_write(priv->regmap, priv->reset_reg, >>> + LP50XX_SW_RESET); >>> + if (ret) { >>> + dev_err(&priv->client->dev, >>> + "Cannot reset the device\n"); >>> + goto out; >>> + } >>> + } >>> + >>> + ret = lp50xx_set_banks(priv); >>> + if (ret) { >>> + dev_err(&priv->client->dev, "Cannot set the banks\n"); >>> + goto out; >>> + } >>> + >>> + ret = regmap_write(priv->regmap, LP50XX_DEV_CFG0, LP50XX_CHIP_EN); >>> + if (ret) >>> + dev_err(&priv->client->dev, "Cannot write ctrl enable\n"); >>> + >>> +out: >>> + return ret; >>> +} >>> + >>> +static int lp50xx_probe_dt(struct lp50xx *priv) >>> +{ >>> + struct fwnode_handle *child = NULL; >>> + struct lp50xx_led *led; >>> + int control_bank_defined = 0; >>> + const char *name; >>> + int led_number; >>> + size_t i = 0; >>> + int ret; >>> + >>> + priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev, >>> + "enable", GPIOD_OUT_LOW); >>> + if (IS_ERR(priv->enable_gpio)) { >>> + ret = PTR_ERR(priv->enable_gpio); >>> + dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n", >>> + ret); >>> + return ret; >>> + } >>> + >>> + priv->regulator = devm_regulator_get(&priv->client->dev, "vled"); >>> + if (IS_ERR(priv->regulator)) >>> + priv->regulator = NULL; >>> + >>> + if (priv->model_id == LP5018) >>> + priv->max_leds = LP5018_MAX_LED_STRINGS; >>> + else if (priv->model_id == LP5024) >>> + priv->max_leds = LP5024_MAX_LED_STRINGS; >>> + else if (priv->model_id == LP5030) >>> + priv->max_leds = LP5030_MAX_LED_STRINGS; >>> + else >>> + priv->max_leds = LP5036_MAX_LED_STRINGS; >> >> Let's change STRINGS to MODULEs. >> > > ACK. > >>> + >>> + device_for_each_child_node(&priv->client->dev, child) { >>> + led = &priv->leds[i]; >>> + >>> + if (fwnode_property_present(child, "ti,led-bank")) { >>> + led->ctrl_bank_enabled = 1; >>> + if (!control_bank_defined) >>> + control_bank_defined = 1; >>> + else { >>> + dev_err(&priv->client->dev, >>> + "ti,led-bank defined twice\n"); >>> + fwnode_handle_put(child); >>> + goto child_out; >>> + } >>> + } else { >>> + led->ctrl_bank_enabled = 0; >>> + } >> >> Any bit set in bank_modules will signify that bank is defined >> and enabled. >> > > This is stored so that when the brightness or color is called the correct > register either bank or LEDn_MODULES is used. > > This way I don't have to go out and read the devices bank enable register > and try to determine what was banked and what was not. You don't need to read the register but its cached state (even via regmap - if you don't mark register volatile then it will return cached register state). I suppose it doesn't change in time in hardware? I meant that instead of: if (!control_bank_defined) you will be able to do the following check: if (!bank_modules) Also you will not need led->ctrl_bank_enabled, because bank_modules == 0 will mean bank disabled. >>> + if (led->ctrl_bank_enabled) { >>> + ret = fwnode_property_read_u32_array(child, >>> + "ti,led-bank", >>> + NULL, 0); >>> + ret = fwnode_property_read_u32_array(child, >>> + "ti,led-bank", >>> + led->led_strings, >>> + ret); >>> + >>> + led->led_number = led->led_strings[0]; >>> + >>> + } else { >>> + ret = fwnode_property_read_u32(child, "ti,led-module", >>> + &led_number); >>> + >>> + led->led_number = led_number; >>> + } >>> + if (ret) { >>> + dev_err(&priv->client->dev, >>> + "led sourcing property missing\n"); >>> + fwnode_handle_put(child); >>> + goto child_out; >>> + } >>> + >>> + if (led_number > priv->max_leds) { >>> + dev_err(&priv->client->dev, >>> + "led-sources property is invalid\n"); >>> + ret = -EINVAL; >>> + fwnode_handle_put(child); >>> + goto child_out; >>> + } >>> + >>> + ret = fwnode_property_read_string(child, "label", &name); >>> + if (ret) >>> + snprintf(led->label, sizeof(led->label), >>> + "%s::", priv->client->name); >>> + else >>> + snprintf(led->label, sizeof(led->label), >>> + "%s:%s", priv->client->name, name); >>> + >>> + fwnode_property_read_string(child, "linux,default-trigger", >>> + &led->led_dev.default_trigger); >>> + >>> + led->priv = priv; >>> + led->led_dev.name = led->label; >>> + led->led_dev.brightness_set_blocking = lp50xx_brightness_set; >>> + led->led_dev.brightness_get = lp50xx_brightness_get; >>> + led->led_dev.groups = lp50xx_led_color_groups; >>> + >>> + ret = devm_led_classdev_register(&priv->client->dev, >>> + &led->led_dev); >>> + if (ret) { >>> + dev_err(&priv->client->dev, "led register err: %d\n", >>> + ret); >>> + fwnode_handle_put(child); >>> + goto child_out; >>> + } >>> + i++; >>> + } >>> + priv->num_of_leds = i; >>> + >>> +child_out: >>> + return ret; >>> +} >>> + >>> +static int lp50xx_probe(struct i2c_client *client, >>> + const struct i2c_device_id *id) >>> +{ >>> + struct lp50xx *led; >>> + int count; >>> + int ret; >>> + >>> + count = device_get_child_node_count(&client->dev); >>> + if (!count) { >>> + dev_err(&client->dev, "LEDs are not defined in device tree!"); >>> + return -ENODEV; >>> + } >>> + >>> + led = devm_kzalloc(&client->dev, struct_size(led, leds, count), >>> + GFP_KERNEL); >>> + if (!led) >>> + return -ENOMEM; >>> + >>> + mutex_init(&led->lock); >>> + led->client = client; >>> + led->dev = &client->dev; >>> + led->model_id = id->driver_data; >>> + i2c_set_clientdata(client, led); >>> + >>> + if (led->model_id == LP5018 || led->model_id == LP5024) >>> + led->regmap = devm_regmap_init_i2c(client, >>> + &lp5024_regmap_config); >>> + else >>> + led->regmap = devm_regmap_init_i2c(client, >>> + &lp5036_regmap_config); >>> + >>> + if (IS_ERR(led->regmap)) { >>> + ret = PTR_ERR(led->regmap); >>> + dev_err(&client->dev, "Failed to allocate register map: %d\n", >>> + ret); >>> + return ret; >>> + } >>> + >>> + ret = lp50xx_probe_dt(led); >>> + if (ret) >>> + �� return ret; >>> + >>> + ret = lp50xx_init(led); >>> + if (ret) >>> + return ret; >>> + >>> + return 0; >>> +} >>> + >>> +static int lp50xx_remove(struct i2c_client *client) >>> +{ >>> + struct lp50xx *led = i2c_get_clientdata(client); >>> + int ret; >>> + >>> + ret = regmap_update_bits(led->regmap, LP50XX_DEV_CFG0, >>> + LP50XX_CHIP_EN, 0); >>> + if (ret) { >>> + dev_err(&led->client->dev, "Failed to disable regulator\n"); >>> + return ret; >>> + } >>> + >>> + if (led->enable_gpio) >>> + gpiod_direction_output(led->enable_gpio, 0); >>> + >>> + if (led->regulator) { >>> + ret = regulator_disable(led->regulator); >>> + if (ret) >>> + dev_err(&led->client->dev, >>> + "Failed to disable regulator\n"); >>> + } >>> + >>> + mutex_destroy(&led->lock); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct i2c_device_id lp50xx_id[] = { >>> + { "lp5018", LP5018 }, >>> + { "lp5024", LP5024 }, >>> + { "lp5030", LP5030 }, >>> + { "lp5036", LP5036 }, >>> + { } >>> +}; >>> +MODULE_DEVICE_TABLE(i2c, lp50xx_id); >>> + >>> +static const struct of_device_id of_lp50xx_leds_match[] = { >>> + { .compatible = "ti,lp5018", }, >>> + { .compatible = "ti,lp5024", }, >>> + { .compatible = "ti,lp5030", }, >>> + { .compatible = "ti,lp5036", }, >>> + {}, >>> +}; >>> +MODULE_DEVICE_TABLE(of, of_lp50xx_leds_match); >>> + >>> +static struct i2c_driver lp50xx_driver = { >>> + .driver = { >>> + .name = "lp50xx", >>> + .of_match_table = of_lp50xx_leds_match, >>> + }, >>> + .probe = lp50xx_probe, >>> + .remove = lp50xx_remove, >>> + .id_table = lp50xx_id, >>> +}; >>> +module_i2c_driver(lp50xx_driver); >>> + >>> +MODULE_DESCRIPTION("Texas Instruments LP5024 LED driver"); >>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>"); >>> +MODULE_LICENSE("GPL v2"); >>> >> > > -- Best regards, Jacek Anaszewski
Hi! Could you, trim the parts of email you do not need in the reply? This is quite annoying to follow: On Tue 2019-01-29 14:26:20, Dan Murphy wrote: > Jacek > > On 1/29/19 2:19 PM, Jacek Anaszewski wrote: > > Hi Dan, > > > > On 1/29/19 2:56 PM, Dan Murphy wrote: > >> Jacek > >> > >> On 1/24/19 3:55 PM, Jacek Anaszewski wrote: > >>> Dan > >>> > >>> On 1/24/19 10:00 PM, Dan Murphy wrote: > >>>> Jacek > >>>> > >>>> On 1/23/19 3:52 PM, Jacek Anaszewski wrote: > >>>>> Dan, > >>>>> > >>>>> On 1/22/19 11:44 PM, Dan Murphy wrote: > >>>>>> Jacek > >>>>>> > >>>>>> On 1/22/19 3:39 PM, Jacek Anaszewski wrote: > >>>>>>> Hi all, > >>>>>>> > >>>>>>> On 1/20/19 7:42 AM, Vesa Jääskeläinen wrote: > >>>>>>>> Hi Dan, > >>>>>>>> > >>>>>>>> On 18/01/2019 15.58, Dan Murphy wrote: > >>>>>>>>> Jacek > >>>>>>>>> > >>>>>>>>> On 1/18/19 7:45 AM, Dan Murphy wrote: > >>>>>>>>>> Jacek > >>>>>>>>>> > >>>>>>>>>> On 1/17/19 3:10 PM, Jacek Anaszewski wrote: > >>>>>>>>>>> Hi Dan, > >>>>>>>>>>> > >>>>>>>>>>> On 1/16/19 7:41 PM, Dan Murphy wrote: > >>>>>>>>>>>> Hello > >>>>>>>>>>>> > >>>>>>>>>>>> On 1/16/19 4:55 AM, Pavel Machek wrote: > >>>>>>>>>>>>> Hi! > >>>>>>>>>>>>> > >>>>>>>>>>>>>> On 1/15/19 4:22 PM, Pavel Machek wrote: > >>>>>>>>>>>>>>> Hi! > >>>>>>>>>>>>>>> ... > >> If this is OK I can post the patch with this update. > >> > >> But I would rather put the file creation in a class and have the colors directory. > > > > Yes, please post your work when it is ready. > > OK. I will derive the class and try to post it by the EOW. You can do that, but if you post the description of kernel<->user interface, first, you may save yourself a bit of coding... Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Dan, On 1/29/19 9:26 PM, Dan Murphy wrote: > Jacek > > On 1/29/19 2:19 PM, Jacek Anaszewski wrote: >> Hi Dan, >> >> On 1/29/19 2:56 PM, Dan Murphy wrote: >>> Jacek >>> >>> On 1/24/19 3:55 PM, Jacek Anaszewski wrote: >>>> Dan >>>> >>>> On 1/24/19 10:00 PM, Dan Murphy wrote: >>>>> Jacek >>>>> >>>>> On 1/23/19 3:52 PM, Jacek Anaszewski wrote: >>>>>> Dan, >>>>>> >>>>>> On 1/22/19 11:44 PM, Dan Murphy wrote: >>>>>>> Jacek >>>>>>> >>>>>>> On 1/22/19 3:39 PM, Jacek Anaszewski wrote: >>>>>>>> Hi all, >>>>>>>> >>>>>>>> On 1/20/19 7:42 AM, Vesa Jääskeläinen wrote: >>>>>>>>> Hi Dan, >>>>>>>>> >>>>>>>>> On 18/01/2019 15.58, Dan Murphy wrote: >>>>>>>>>> Jacek >>>>>>>>>> >>>>>>>>>> On 1/18/19 7:45 AM, Dan Murphy wrote: >>>>>>>>>>> Jacek >>>>>>>>>>> >>>>>>>>>>> On 1/17/19 3:10 PM, Jacek Anaszewski wrote: >>>>>>>>>>>> Hi Dan, >>>>>>>>>>>> >>>>>>>>>>>> On 1/16/19 7:41 PM, Dan Murphy wrote: >>>>>>>>>>>>> Hello >>>>>>>>>>>>> >>>>>>>>>>>>> On 1/16/19 4:55 AM, Pavel Machek wrote: >>>>>>>>>>>>>> Hi! >>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 1/15/19 4:22 PM, Pavel Machek wrote: >>>>>>>>>>>>>>>> Hi! >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> +The 24-bit RGB value passed in follows the pattern 0xXXRRGGBB >>>>>>>>>>>>>>>>>> +XX - Do not care ignored by the driver >>>>>>>>>>>>>>>>>> +RR - is the 8 bit Red LED value >>>>>>>>>>>>>>>>>> +GG - is the 8 bit Green LED value >>>>>>>>>>>>>>>>>> +BB - is the 8 bit Blue LED value >>>>>>>>>>>>>>>>>> + >>>>>>>>>>>>>>>>>> +Example: >>>>>>>>>>>>>>>>>> +LED module output 4 of the LP5024 will be a yellow color: >>>>>>>>>>>>>>>>>> +echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color >>>>>>>>>>>>>>>>>> + >>>>>>>>>>>>>>>>>> +LED module output 4 of the LP5024 will be dimmed 50%: >>>>>>>>>>>>>>>>>> +echo 0x80 > /sys/class/leds/lp5024\:led4_mod/brightness >>>>>>>>>>>>>>>>>> + >>>>>>>>>>>>>>>>>> +LED banked RGBs of the LP5036 will be a white color: >>>>>>>>>>>>>>>>>> +echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> This part with example cans remain in Documentation/leds if you >>>>>>>>>>>>>>>>>> like. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Does it actually work like that on hardware? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> What? >>>>>>>>>>>>>> >>>>>>>>>>>>>> If you do echo 0xffffff > /sys/class/leds/lp5036\:led_banked/color, >>>>>>>>>>>>>> does it actually produce white? With all the different RGB modules >>>>>>>>>>>>>> manufacturers can use with lp5024P? >>>>>>>>>>>>>> >>>>>>>>>>>>>> If you do echo 0xe6de00 > /sys/class/leds/lp5024\:led4_mod/color, does >>>>>>>>>>>>>> it actually produce yellow, with all the different RGB modules >>>>>>>>>>>>>> manufacturers can use with lp5024P? >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> I believe the answer to the general questions is no for any RGB cluster and driver out there. >>>>>>>>>>>>> Because if you set the same values on each and every RGB device out there you will get varying shades of the color. >>>>>>>>>>>>> But for this device yes the color does appear to be yellow to me versus what was displayed on my monitor by the HSL picker. >>>>>>>>>>>>> But everyone interprets colors differently. >>>>>>>>>>>>> >>>>>>>>>>>>> If you write the same value for yellow or white on a droid 4 and the N900 do they produce the same color side by side? >>>>>>>>>>>>> Most probably not. >>>>>>>>>>>>> >>>>>>>>>>>>> As you pointed out the PWM needs to be modified to obtain the correct white color to account for LED and other device constraints. >>>>>>>>>>>>> >>>>>>>>>>>>> But we need to take into account the light pipe. Pools nowadays have RGB LED spot lights in them. It can >>>>>>>>>>>>> be set to white. On my pool right off the lens the color has a purplish hue to it. As the light is diffracted into >>>>>>>>>>>>> the pool the color becomes white. The pool is clear. When I add chemicals to the pool and make it cloudy >>>>>>>>>>>>> and turn on the lights the color off the lens is now white. This is an example on a large scale but the issue >>>>>>>>>>>>> scales down to the hand helds and smart home applications. >>>>>>>>>>>>> >>>>>>>>>>>>> If the cluster is piped through a flexible optic 0xffffff may produce the "white" you want on its output. >>>>>>>>>>>>> >>>>>>>>>>>>> So an expectation of certain color without proper piping based on a single RGB value may be a little unreasonable. >>>>>>>>>>>>> There may need to be a way to attenuate the values based on the hardware aspect of the equation ie light pipe (or lack thereof) and LED vendor. >>>>>>>>>>>>> So if we write 0xffffff to the RGB driver the driver could adjust the intensity of the individual LEDs based on the diffraction >>>>>>>>>>>>> coefficients. >>>>>>>>>>>>> >>>>>>>>>>>>> I also think that is an unreasonable expectation here that writing a single value to any LED RGB driver would produce >>>>>>>>>>>>> a "rest of the world" absolute color. Maybe it can produce something similar but not identical. >>>>>>>>>>>>> As you indicated in the requirements there is more involved here then just the LED and the values written. >>>>>>>>>>>>> The colors should be close but may not be identical. >>>>>>>>>>>>> >>>>>>>>>>>>> A 10 year old N900 should not be considered the gold standard for color production due to advancements in LED, >>>>>>>>>>>>> light pipe and LED driver technology. >>>>>>>>>>>>> The single package RGB clusters on the board I am testing is about the size of a single RGB LED from 10 years ago. >>>>>>>>>>>>> >>>>>>>>>>>>> I agree that the interface developed should work on the device but the algorithm derived to obtain the color needs to have >>>>>>>>>>>>> a hardware aspect to the calculation. >>>>>>>>>>>>> >>>>>>>>>>>>>>>> Is it supposed to support "normal" RGB colors as seen on monitors? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Monitors are not an application for this part. >>>>>>>>>>>>>> >>>>>>>>>>>>>> You did not answer the question. When you talk about yellow, is it >>>>>>>>>>>>>> same yellow the rest of world talks about? >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> See above. It is close to what was on my monitor displayed. >>>>>>>>>>>>> >>>>>>>>>>>>>>>> Because 100% PWM on all channels does not result in white on hardware >>>>>>>>>>>>>>>> I have. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I don't know I am usually blinded by the light and have no diffuser over >>>>>>>>>>>>>>> the LEDs to disperse the light so when I look I see all 3 colors. >>>>>>>>>>>>>> >>>>>>>>>>>>>> How can we have useful discussion about colors when you don't see the >>>>>>>>>>>>>> colors? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Place a piece of paper over the LEDs.... >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Good suggestion for a rough test. >>>>>>>>>>>>> >>>>>>>>>>>>>>>> But... >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I believe we should have a reasonable design before we do something >>>>>>>>>>>>>>>> like this. There's no guarantee someone will not use lp50xx with just >>>>>>>>>>>>>>>> the white LEDs for example. How will this work? Plus existing hardware >>>>>>>>>>>>>>>> already uses three separate LEDs for RGB LED. Why not provide same >>>>>>>>>>>>>>>> interface? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Which existing hardware? Are they using this part? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Nokia N900. They are not using this part, but any interface we invent >>>>>>>>>>>>>> should work there, too. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Yes a common interface would be nice with some sort of hardware tuning coefficient. >>>>>>>>>>>>> >>>>>>>>>>>>>>> <rant> >>>>>>>>>>>>>>> Why are we delaying getting the RGB framework or HSV in? >>>>>>>>>>>>>>> I would rather design against something you want instead of having >>>>>>>>>>>>>>> everyone complain about every implementation I post. >>>>>>>>>>>>>>> </rant> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Because you insist on creating new kernel interfaces, when existing >>>>>>>>>>>>>> interfaces work, and are doing that badly. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Because your patches are of lower quality than is acceptable for linux >>>>>>>>>>>>>> kernel. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Because you don't seem to be reading the emails. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I sent list of requirements for RGB led support. This does not meet >>>>>>>>>>>>>> them. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Sigh. You did not answer my question. >>>>>>>>>>>>> >>>>>>>>>>>>> Your requirements seem to be centered around monitors but that is only one application of the current >>>>>>>>>>>>> RGB LED landscape. >>>>>>>>>>>>> >>>>>>>>>>>>> I am willing to work with you on the HSV and adapting the LP50xx part to this framework. >>>>>>>>>>>>> Or any RGB framework for that matter. I still don't agree with the kernel needing to declare colors >>>>>>>>>>>>> maybe color capabilities but not specific colors. >>>>>>>>>>>> >>>>>>>>>>>> Dan, if you have a bandwidth for LED RGB class implementation >>>>>>>>>>>> then please go ahead. It would be good to compare colors produced >>>>>>>>>>>> by software HSV->RGB algorithm to what can be achieved with >>>>>>>>>>>> LEDn_BRIGHTNESS feature. >>>>>>>>>>>> >>>>>>>>>>>> The requirements for LED RGB class as I would see it: >>>>>>>>>>>> >>>>>>>>>>>> sysfs interface: >>>>>>>>>>>> >>>>>>>>>>>> brightness-model: space separated list of available options: >>>>>>>>>>>> - rgb (default): >>>>>>>>>>>> - creates color file with "red green blue" decimal values >>>>>>>>>>> >>>>>>>>>>> What about other colored LEDs? Presenting RGB for an Amber LED does not seem right. >>>>>>>>>>> Should the LED color come from the DT? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I thought about this, other non-RGB LEDs would not use the RGB framework. >>>>>>>>>> But should they have the same interfaces as RGB? >>>>>>>>>> >>>>>>>>>> Should PWM control be a global interface? >>>>>>>>> >>>>>>>>> In order to being able to set multi color element led at one go I would recommend using then model: >>>>>>>>> >>>>>>>>> color_names: "red green blue white" >>>>>>>>> >>>>>>>>> echo "32 43 0 128" > color >>>>>>>>> >>>>>>>>> This way all elements would be set at same time from user space point of view. >>>>>>>>> >>>>>>>>> This of course requires that it is part of the physical/logical led that is being controlled. If it is a separate thing then it would logically be differently controlled mono color led. >>>>>>>>> >>>>>>>>> If you look what kinds of leds are available lets say from digikey you get all kinds of combos: >>>>>>>>> >>>>>>>>> - red, green, blue >>>>>>>>> - red, green, blue, amber >>>>>>>>> - red, green, blue, white >>>>>>>>> - red, green, blue, white - cool >>>>>>>>> - red, green, blue, white - neutral >>>>>>>>> - red, green, blue, white - warm >>>>>>>>> - red, orange >>>>>>>>> - purple, ultraviolet >>>>>>>>> - amber, blue >>>>>>>>> - amber, blue, cyan, green, red, violet, white - cool >>>>>>>>> - amber, blue, green >>>>>>>>> - amber, green, blue >>>>>>>>> - and then lots of single special colors >>>>>>>> >>>>>>>> It suggested me another solution. Instead of LED RGB class >>>>>>>> we would have LED multi-color class. >>>>>>>> >>>>>>> >>>>>>> I was thinking the same thing this morning. But I was thinking that the RGB >>>>>>> class should be an additional class to stand on its own or can register to the >>>>>>> multi color class. >>>>>>> >>>>>>>> Sysfs interface design: >>>>>>>> ----------------------- >>>>>>>> >>>>>>>> colors: directory containing files that map to >>>>>>>> the brightness of particular LEDs; there >>>>>>>> would be predefined color names that LED class >>>>>>>> driver should map iouts to, e.g.: >>>>>>> >>>>>>> Filling in the missing ideas with questions. >>>>>>> >>>>>>> Is it a directory or a file? If it is a directory does that not break the current >>>>>>> directory label model? >>>>>>> >>>>>>> so the path would be /sys/class/leds/colors ? (It is probably not this but needs clarification) >>>>>>> How would this look if I had 2 of the same color LEDs? The Beagle bone black has 2 Blue LEDs. >>>>>>> They are assigned to different triggers and have different directories. Both are GPIO controlled. >>>>>>> >>>>>>> Or are you saying it would be something like (More then likely this is what you intended) >>>>>>> /sys/class/leds/input4::numlock/colors? >>>>>> >>>>>> Yes, this is what I meant. >>>>>> >>>>> >>>>> OK. Thanks for the clarification >>>>> >>>>>>> Maybe it is mandated that "multi" be added to the label when the class is registered so the caller >>>>>>> knows that this is a multi color class and not a single LED color class. >>>>>> >>>>>> Like I am going to come up with standardized color names >>>>>> in my LED naming related patch, the multi-color names >>>>>> can be defined as well, e.g.: rgb, rgbw, rgbwa, rgbwauv etc. >>>>>> >>>>> >>>>> That may be better it is descriptive off the command line. >>>>> >>>>>>> What about providing a file called colors_raw which takes in the raw decimal values to obtain other color >>>>>>> variants when RGB is only available? And this can also present a single write to the kernel with the new >>>>>>> color values. >>>>>> >>>>>> My design already covers that in the form of files in the colors >>>>>> directory. Like I stated: "files that map to the brightness of >>>>>> particular LEDs". Single write is secured by "echo "write" > sync. >>>>>> >>>>> >>>>> OK. So set the new values and then tell the set the sync which will make the device driver write the >>>>> device. >>>>> >>>>> Sounds good. how about echo 1 > sync and we can stay away from a long string conversion >>>> >>>> We need also to be able to do a readout. >>>> >>>> If we changed "sync" to "rw" then we could come up with intuitive >>>> semantics: >>>> >>>> echo 1 > rw // write >>>> echo 0 > rw // read >>>> >>>> rw file would have WO permission. >>> >>> I have re-written the lp50xx driver to show red, green, blue, sync and sync_enable files for each registered LED. >> >> Did you change your mind regarding LED multi color class implementation? >> > > No. I just did not want to put effort into the class if the interfaces were not agreed upon. > So if we are good with the interfaces I can start the MC class. I'm good with that. It would be nice to see some input from the other people. >>> I added the sync_enable file so that the there can be real time control of the individual LEDs as well. >> >> Yes, it's a good idea. >> >>> Turning on sync means that the user will write to one of the color files and the setting won't take affect until >>> sync is written. >>> >>> If sync is off then the LED register is updated immediately. >>> >>> Being able to turn on and off syncing maybe better. A developer may choose to set up the color and then sync >>> or they may want to ramp a specific color. This will help reduce user space writes but also reduce the number >>> of peripheral writes when color values do not change. >>> >>> I am having difficulty in creating the colors directory in the device driver. >>> This appears to need to be done in the class when the class pointer is available. >> >> Not necessarily, you need only dev->kobj. Please follow the implementation of drivers/usb/core/ledtrig-usbport.c. However I would >> really prefer if it became a part of new LED multi color class. >> > > It will. I should be able to create the colors directory there as well. > I need to figure out how to cleanly tell the MC class what LEDs are available. Of course the solution from ledtrig-usbport.c is equally good for the class. Related to the LED color identifications in the multi color class - we'd need some defines in e.g. include/dt-bindings/leds/colors.h: #define LED_COLOR_RED 0 #define LED_COLOR_GREEN 1 #define LED_COLOR_BLUE 2 Then, in the device tree we could have: led-controller@8 { ... led@0 { multi-color = <LED_COLOR_RED LED_COLOR_GREEN LED_COLOR_BLUE>; reg = <0x0>; // in case of lp5024 this would identify // RGB LED module; }; }; For pwm LEDs we would need a bit different approach, so I'd not strive to create generic bindings for LED multi color class. We need generic interface only for LED multi color class registration. e.g.: struct led_classdev_multicolor { /* led class device */ struct led_classdev led_cdev; // for legacy brightness support struct led_classdev *colors; // dynamically allocated array void (*multicolor_set) (struct led_classdev_multicolor *led_mcdev); void (*unicolor_set) (struct led_classdev *led_cdev, enum led_brightness); // [color]_store ssyfs // callback will call it }; led_classdev_multicolor_register(struct device *parent, struct led_classdev_multicolor *led_mcdev) ... >>>>>>> I am not a fan of hard coding preset colors as you can see there are to many of them and variations of the color. >>>>>>> In addition this severely limits the ability of the user. Unless we stick to primary colors only and not secondary >>>>>>> colors. >>>>>>> Defining and hard coding hte colors can get out of control and not maintainable moving forward. Especially >>>>>>> if we start adding defines like white_warm, white_neutral and other variations to the color. >>>>>> >>>>>> We would not limit anything. Every color combination can be achieved >>>>>> by following this exemplary sequence: >>>>>> >>>>>> $ echo 154 > colors/red >>>>>> $ echo 232 > colors/green >>>>>> $ echo 43 > colors/blue >>>>>> # echo "write" > sync //only this changes hardware state >>>>>> >>> >>> The code works more like this >>> $ :/sys/class/leds/lp5024:led1_mod# ls >>> blue device max_brightness red sync trigger >>> brightness green power subsystem sync_enable uevent >>> >>> $ cat sync_enable >>> enabled // I can change this to return an int instead >>> $ echo 154 > red >>> $ echo 232 > green >>> $ echo 43 > blue >>> $ echo 1 > sync //this changes hardware state >>> >>> $ echo 0 > sync_enable >>> $ cat sync_enable >>> disabled >>> $ echo 154 > red // changes red LED immediately >>> $ echo 232 > green // changes green LED immediately >>> $ echo 43 > blue // changes blue LED immediately >>> $ echo 1 > sync // Has no affect on the hardware >>> >>> If this is OK I can post the patch with this update. >>> >>> But I would rather put the file creation in a class and have the colors directory. >> >> Yes, please post your work when it is ready. > > OK. I will derive the class and try to post it by the EOW. > >> >>>>>> brightness-model is provided only to define mapping of legacy brightness >>>>>> levels (governed by brightness file and led_set_brightness() API) to >>>>>> the specific combination of colors. >>>>>> >>>>>> For instance we can define three brightness levels for green hue: >>>>>> >>>>>> DT definition for it would look like below: >>>>>> >>>>>> rgb-green = <0x277c33 0x37b048 0x47e45d>; >>>>>> >>>>>> LED multi color class would then do the following mapping for >>>>>> each of the three brightness levels for rgb-green brightness model: >>>>>> >>>>>> $ echo rgb-green > brightness_model >>>>>> $ echo 1 > brightness // red=0x27, green=0x7c, blue=0x33 >>>>>> $ echo 2 > brightness // red=0x37, green=0xb0, blue=0x48 >>>>>> $ echo 3 > brightness // red=0x47, green=0xe4, blue=0x5d >>>>>> >>>>> >>>>> OK I would have to play with this on the LP devices. >>> >>> I have not done anything with this. >>> > > I will omit this until we figure out how to nicely present this to user space from the MC class. Could you share what particularly is your concern? > Or maybe it is something you can add once we have the class ready. OK. But for the cases when neither LEDn_BRIGHTNESS feature nor any DT color ranges are provided please do provide one brightness model "onoff", that for brightness==1 will set brightness of all colors to max, and turn them off on brightness==0. >>>>>>> What about IR LEDs used for night vision products? Do these fall into the multi color class? >>>>>>> We do have a driver I submitted that had an IR LED and a White LED combined. It was created against the >>>>>>> flash class but it could be a multi color LED as well. >>>>>>> >>>>>>> How would traversing through the full color palette work if you were to want to produce a multi >>>>>>> color ring like the LP50xx where the pattern can traverse from one end of the color spectrum and back? >>>>>>> Or in a product like the gaming keyboards that will change color or change backlight brightness? >>>>>> >>>>>> This is not meant as a solution for pattern generator but for >>>>>> consolidated source of multi color light. In the simplest case >>>>>> RGB LED elements like those used for lp5024 reference board, >>>>>> but it could be RGBWAUV LED [0] as well. >>>>>> >>>>>> For patterns traversing many LEDs I see a trigger as the best solution. >>>>>> Hmm, now I see that trigger mechanism actually can serve as very >>>>>> flexible pattern generator. >>>>>> >>>>>> We would need a device that could be configured to register >>>>>> a number of multi-led-patternN triggers, one per LED, and generate >>>>>> events for each trigger in a loop. >>>>>> >>>>>> The device would have to allow for configuring pattern intervals >>>>>> via sysfs, like in case of current pattern trigger. >>>>>> >>>>>> LED class devices would have to register for its events: >>>>>> >>>>>> $/sys/class/leds/led1 echo multi-led-pattern1 > trigger >>>>>> $/sys/class/leds/led2 echo multi-led-pattern2 > trigger >>>>>> $/sys/class/leds/led3 echo multi-led-pattern3 > trigger >>>>>> >>>>> >>>>> A bit off topic but I like the idea. We should save this for another day >>>> >>>> Yes, that's another story. >>>> >>>>>> The ability to define brightness models in DT would >>>>>> add even more flexibility. >>>>>> >>>>> >>>>> brightness models would be mandatory to support in the driver but an optional >>>>> DT entry. >>>>> >>>>> Is that a correct assumption? >>>> >>>> Correct. >>>> >>>>>>> Not sure what color LEDs the keyboard manufacturers place on their keyboards but does the interface design >>>>>>> capable of doing this? >>>>>>> >>>>>>> https://www.youtube.com/watch?v=pfKv3g2FeBE >>>>>>> or something like this >>>>>>> >>>>>>> https://www.youtube.com/watch?v=PO3auX3f5C4 >>>>>>> >>>>>>> The LP5036 has this capability. >>>>>>> >>>>>>>> - red >>>>>>>> - green >>>>>>>> - blue >>>>>>>> - white >>>>>>>> - sync: accepts "write" and "read", which executes >>>>>>>> write/readout to/from a device respectively. >>>>>>>> >>>>>>> >>>>>>> What are these above, the values or the files under the colors directory? >>>>>> >>>>>> They are just color specific counterparts of monochrome brightness. >>>>>> In terms of lp50xx they would map to OUTn_COLOR registers. >>>>>> >>>>>>> I am assuming they are files. >>>>>> >>>>>> Right. >>>>>> >>>>>>> Are they mandatory or optional? >>>>>> >>>>>> Mandatory, one per each iout to control. >>>>>> >>>>> >>>>> OK And all the LEDs within this directory would be considered a LED cluster? >>>>> >>>>> And if there are 2 like colors of the LED defined in the same cluster would we just see a single >>>>> file and write a common value to that file and the driver would have to update each >>>>> red LED within that cluster. No independent control of like colored LEDs within the >>>>> registered LED cluster. >>>>> >>>>> If the developer wants this level of control they would have to register two separate classes >>>>> >>>>> Correct? >>>> >>>> I would abide by one color to one iout mapping. Registering more LEDs >>>> under the same color feels more of a task for trigger. >>>> >>>>>>>> brightness-model: defines brightness level to color configuration >>>>>>>> mapping >>>>>>>> - "hardware": for devices with feature like LEDn_BRIGHTNESS of lp50xx >>>>>>>> - "rgb-<hue>": available only when all three red,green,blue colors >>>>>>>> are present in the colors directory. >>>>>>>> <hue> is a placeholder for given DT hue presets. >>>>>>>> - "rgbw-<hue>": Same as above, but available only when white color >>>>>>>> (any of amber or white-{cool|neutral|warm} can be >>>>>>>> mapped to white) is also available. In this mode >>>>>>>> max_brightness equals num-of-hue-presets + 1, and >>>>>>>> max_brightness, when set, turns the "white" LED on >>>>>>> >>>>>>> Why do we need white combined here? Should this not be its own entity? >>>>>> >>>>>> To be able to set white color. We're still talking about one LED >>>>>> element (although they can be be physically few LEDs in one case). >>>>>> This is brightness file, so we've got to stick to the semantics. >>>>>> Max brightness level should be the brightest. With RGBW LEDs we >>>>>> fortunately have a means to achieve pure white, that's why >>>>>> rgbw-<hue> would be beneficial. If you increase L component of >>>>>> HSL color space, the max value gives white for all hues. >>>>>> So maybe this brightness-model would be rather called hsl-<hue>. >>>>>> >>>>>> For RGBW LEDs, we would have to allow for more shades of white too, >>>>>> like in [1]. >>>>>> >>>>> >>>>> Yep. >>>> >>>> But this all would be left to the DT designer decision. >>>> >>>>>>> Again I don't like limiting the color palette from the DT. I think that the >>>>>>> user space can see what colors are available for that device and makes its own >>>>>>> decision on what color to present. >>>>>>> >>>>>>> For the RGBw what about RGB amber and RGB purple. Are the white LEDs always part of the >>>>>>> same function trying to be achieved by the system designer? The RGB can be used to denote >>>>>>> notification status and the white can be used to denote that a charger is connected. Motorola >>>>>>> Droid did this. >>>>>> >>>>>> I hope I've just clarified my idea. >>>>>> >>>>> >>>>> Its getting clearer. I would like to see it in code and play with it not as a user >>>>> but as a developer. Make sure the paper model works as well as the real implementation. >>>>> >>>>> Is this model clear to the developer? >>>>> How would a developer define what values are appropriate for the brightness-model? >>>> >>>> We could create guidelines e.g. that for hsl-<hue> pattern, the >>>> colors corresponding to brightness levels should be arranged so >>>> that increasing brightness felt like increasing value of >>>> L component of HSL. >>>> >>>> But we wouldn't be able to enforce adherence to a particular scheme. >>>> >>>>> Does the driver have to become overly complex to support simple color generation? >>>> >>>> Not at all. Caching the colors written to the files in colors directory >>>> would be responsibility of LED multi-color class. On echo 1 > sync >>>> the core would call a new op e.g. >>>> >>>> color_set(struct led_classdev_multicolor *mcled_cdev) >>>> >>>> Driver would read in it colors cached in struct led_classdev_multicolor >>>> and write them to the hardware. >>>> >>>>> Thoughts on putting code to idea? >>>>> >>>>> >>>>>>> >>>>>>>> - "rgb-linear": I'm not sure if it should be available - it will >>>>>>>> have unpredictable results >>>>>>>> >>>>>>>> brightness: sets/reads brightness in the way specific to the >>>>>>>> current brightness-model. When more colors are available >>>>>>>> (e.g. amber, blue, cyan, green, red, violet, white), they >>>>>>>> are not touched by write to brightness). >>>>>>>> >>>>>>>> HSV->RGB conversion is left entirely to the userspace, which can set any >>>>>>>> color with use of the proposed interface. >>>>>>>> >>>>>>>> Let's agree on sysfs interface first, and only after that move >>>>>>>> to the DT details. >>>>>>>> >>>>>>> >>>>>>> DT's are meant to describe hardware and not describe the product. Unless Rob does not see >>>>>>> an issue with defining product capabilities in the DT then we should keep that out of the DT. >>>>>> >>>>>> LED element is a device. I see nothing irrelevant for DT in describing the lighting specificity of a device mounted on the board. Please keep >>>>>> in mind that it will not limit the number of colors available to set. >>>>>> It will only allow to define mapping of brightness level to color. >>>>>> We need that for current trigger mechanism to work with LED multi-color >>>>>> class. >>>>>> >>>>> >>>>> I see this now. >>>>> >>>>> Dan >>>>> >>>>>> [0] http://www.cobledarray.com/sale-4942931-10w-rgbwauv-led-diode-6-in-1-high-power-multicolor-led-chip.html >>>>>> [1] https://www.youtube.com/watch?v=NzlFmTqOh9M >>>>>> >>>>> >>>>> >>>> >>> >>> >> > > -- Best regards, Jacek Anaszewski
diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.txt b/Documentation/devicetree/bindings/leds/leds-lp50xx.txt new file mode 100644 index 000000000000..7bc6843ddba4 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.txt @@ -0,0 +1,143 @@ +* Texas Instruments - LP5018/24/30/36 RGB LED driver + +The LP50XX is multi-channel, I2C RGB LED Drivers that can group RGB LEDs into +a LED group or control them individually. + +The difference in these RGB LED drivers is the number of supported RGB strings. + +Required properties: + - compatible: + "ti,lp5018" + "ti,lp5024" + "ti,lp5030" + "ti,lp5036" + - reg : I2C slave address + lp5018/24 - 0x28 + lp5030/36 - 0x30 + - #address-cells : 1 + - #size-cells : 0 + +Optional properties: + - enable-gpios : gpio pin to enable/disable the device. + - vled-supply : LED supply + +Required child properties: + - reg : Is the child node iteration. + +Required Child properties but only one should be defined per child: +Either one of these two properties are required for each node. The +property ti,led-bank takes precedence over the ti,led-module within the same +node. + + - ti,led-module : This property denotes the single LED module number + that will be controlled in the LED class instance. + - ti,led-bank : This property denotes the LED module numbers that will + be controlled as a single RGB cluster. Each LED module + number will be controlled by a single LED class instance. + There can only be one instance of the ti,led-bank + property for each device node. + +The LED outpus associated with the LED modules are defined in Table 1 of the +corresponding data sheets. + +LP5018 - 6 Total RGB cluster LED outputs 0-5 +LP5024 - 8 Total RGB cluster LED outputs 0-7 +LP5030 - 10 Total RGB cluster LED outputs 0-9 +LP5036 - 12 Total RGB cluster LED outputs 0-11 + +Optional child properties: + - label : see Documentation/devicetree/bindings/leds/common.txt + - linux,default-trigger : + see Documentation/devicetree/bindings/leds/common.txt + +Examples: +LP5018 and LP5024 example: +led-controller@29 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "ti,lp5024"; + reg = <0x29>; + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; + vled-supply = <&vmmcsd_fixed>; + + led@0 { + reg = <0>; + label = "led1_mod"; + ti,led-module = <1>; + }; + + led@1 { + reg = <1>; + label = "banked_leds"; + ti,led-bank = <0 2 5 3 >; + }; + + led@2 { + reg = <2>; + label = "led4_mod"; + ti,led-module = <4>; + }; + + led@3 { + reg = <3>; + label = "led7_mod"; + ti,led-module = <7>; + }; + + led@4 { + reg = <4>; + label = "led6_mod"; + ti,led-module = <6>; + }; +}; + +LP5030 and LP5036 example: +led-controller@30 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "ti,lp5036"; + reg = <0x30>; + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>; + vled-supply = <&vmmcsd_fixed>; + + led@0 { + reg = <0>; + label = "led1_mod"; + ti,led-module = <1>; + }; + + led@1 { + reg = <1>; + label = "led_banked"; + ti,led-bank = <0 2 5 3 9 10 >; + }; + + led@2 { + reg = <2>; + label = "led4_mod"; + ti,led-module = <4>; + }; + + led@3 { + reg = <3>; + label = "led7_mod"; + ti,led-module = <7>; + }; + + led@4 { + reg = <4>; + label = "led6_mod"; + ti,led-module = <6>; + }; + + led@5 { + reg = <5>; + label = "led8_mod"; + ti,led-module = <8>; + }; +}; + + +For more product information please see the link below: +http://www.ti.com/lit/ds/symlink/lp5024.pdf +http://www.ti.com/lit/ds/symlink/lp5036.pdf
Introduce the bindings for the Texas Instruments LP5036, LP5030, LP5024 and the LP5018 RGB LED device driver. The LP5036/3024/18 can control RGB LEDs individually or as part of a control bank group. These devices have the ability to adjust the mixing control for the RGB LEDs to obtain different colors independent of the overall brightness of the LED grouping. Datasheet: http://www.ti.com/lit/ds/symlink/lp5024.pdf http://www.ti.com/lit/ds/symlink/lp5036.pdf Signed-off-by: Dan Murphy <dmurphy@ti.com> --- v2 - Added the LP5030/36 devices, defined the modules vs banked properties renamed the file from lp5024 to lp50xx. - https://lore.kernel.org/patchwork/patch/1026514/ .../devicetree/bindings/leds/leds-lp50xx.txt | 143 ++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.txt -- 2.20.1.98.gecbdaf0899