Re: turris omnia leds again: question

[Marek Behun <kabel@blackhole.sk>]

On Wed, 11 Mar 2020 11:59:07 +0100
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> On 3/10/20 11:23 PM, Marek Behun wrote:
> > On Tue, 10 Mar 2020 22:48:09 +0100
> > Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> >   
> >> Hi Marek,
> >>
> >> On 3/10/20 6:38 PM, Marek Behun wrote:  
> >>> Hi,
> >>>
> >>> I am going to try to send driver for Omnia LEDs again. The last time
> >>> there was a problem: on 05/01/2019 Jacek wrote:
> >>>     
> >>>> I wonder if we're doing right merging this driver in this form.
> >>>> We break the rule one-led-class-device-per-one-channel. We don't
> >>>> have LED multi color support yet, so this should support RGB LEDs
> >>>> in the old manner. Or switch to using LED multi color class.    
> >>>     
> >>>> Once we will have LED multi color class, we will be able to add the
> >>>> support for it to the driver and make the driver configurable to be
> >>>> able to expose old interface or the LED multi color one.    
> >>>     
> >>>> Moreover, the bindings should use led-sources property for grouping
> >>>> three channels under single LED class device. This is certainly to be
> >>>> fixed.    
> >>>
> >>> So I am going to try to modify the driver so that each channel creates
> >>> one LED class device. Do I understand this correctly then, that this
> >>> way when there are three channels (RGB) on one LED, all the 3 device
> >>> tree nodes for should have the same reg property, but different
> >>> led-sources property? Eg:
> >>>
> >>>   led@0,0 {
> >>>     reg = <0>;
> >>>     led-sources = <0>;
> >>>     label = "omnia::heartbeat::red";
> >>>   };
> >>>
> >>>   led@0,1 {
> >>>     reg = <0>;
> >>>     led-sources = <1>;
> >>>     label = "omnia::heartbeat::green";
> >>>   };
> >>>
> >>>   led@0,2 {
> >>>     reg = <0>;
> >>>     led-sources = <2>;
> >>>     label = "omnia::heartbeat::blue";
> >>>   };
> >>>
> >>> Or did I misinterpret the led-sources property?    
> >>
> >> This is what I proposed back then, strangely that message wasn't
> >> archived by bots, or maybe it resides only in my outbox...
> >>
> >> --------------
> >>
> >> LED sub-node properties:
> >>  - reg :                Must be from 0x0 to 0xb, since there are 12 RGB
> >> LEDs on this
> >>                         controller.
> >>  - label :              (optional)
> >>    see Documentation/devicetree/bindings/leds/common.txt
> >>  - linux,default-trigger : (optional)
> >>    see Documentation/devicetree/bindings/leds/common.txt
> >>  - led-sources : Each child node should describe RGB LED it controls,
> >>                  by listing corresponding iout identifiers:
> >>         0 - RGB LED 0: red
> >>         1 - RGB LED 0: green
> >>         2 - RGB LED 0: blue
> >>         3 - RGB LED 1: red
> >>         4 - RGB LED 1: green
> >>         5 - RGB LED 1: blue
> >>         6 - RGB LED 2: red
> >>         7 - RGB LED 2: green
> >>         8 - RGB LED 2: blue
> >>         9 - RGB LED 3: red
> >>         10 - RGB LED 3: green
> >>         11 - RGB LED 3: blue
> >>     ... and list all the iouts, maybe other names will be more
> >>             appropriate for this device, feel free to propose something
> >>
> >>
> >>
> >> Example:
> >>
> >>         led-controller@2b {
> >>                 compatible = "cznic,turris-omnia-leds";
> >>                 reg = <0x2b>;
> >>                 #address-cells = <1>;
> >>                 #size-cells = <0>;
> >>
> >>                 led@0 {
> >>                         reg = <0x0>;
> >>                         label = "userB";
> >>                         linux,default-trigger = "heartbeat";
> >>                         led-sources = <0 1 2>;
> >>                 };
> >>
> >>                 led@1 {
> >>                         reg = <0x1>;
> >>                         label = "userA";
> >>                         led-sources = <3 4 5>;
> >>                 };
> >>
> >>                 led@2 {
> >>                         reg = <0x2>;
> >>                         label = "pci3";
> >>                         led-sources = <6 7 8>;
> >>                 };
> >>
> >>                 led@3 {
> >>                         reg = <0x3>;
> >>                         label = "pci2";
> >>                         led-sources = <9 10 11>;
> >>                 };
> >>                 ...
> >> --------------
> >>
> >>
> >> Of course now label should be replaced with color and function
> >> properties. I've just reviewed that patch set and realized that
> >> we agreed upon setting max_brightness to 1 for all LEDs, right?
> >>  
> > 
> > No, there were subsequent patches which added support for
> > max_brightness = 255.  
> 
> Right.
> 
> > So the driver will register one LED class device per node, with color
> > ID = WHITE. Once RGB LED class is merged, the driver can be remade, but
> > the device tree won't need to be changed.  
> 
> Device Tree will need to be changed to LED mc specific bindings,
> which at current state introduces one more level or nesting
> and LED_COLOR_ID_MULTI for the top level DT node.
> 
> And the driver will need to still support this approach as well
> as the new LED mc class.
> 

Hi Jacek,
I have used the led-sources in such a way that the user can either set
	led-sources = <0 1 2>;
	color = <LED_COLOR_ID_WHITE>;
in which case all three channels will be grouped into one led cdev,
or the user can use just one led-source, for example
	led-sources = <0>;
	color = <LED_COLOR_ID_RED>;
and in this case they can have one led cdev per channel.
Is this acceptable? Or should I just go with the WHITE approach?

In case that this is acceptable I wonder what should be the suggested
device-tree node naming and reg property, when using one led cdev per
channel, for example:
  led@1,0 {
	reg = <1>;
	led-sources = <3>;
	color = <LED_COLOR_ID_RED>;
  };
  led@1,1 {
	reg = <1>;
	led-sources = <4>;
	color = <LED_COLOR_ID_GREEN>;
  };
  led@1,2 {
	reg = <1>;
	led-sources = <5>;
	color = <LED_COLOR_ID_BLUE>;
  };
I don't think different nodes should have the same reg property. Should
in this case the reg property have two values?

Marek