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
