Re: turris omnia leds again: question

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

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.

-- 
Best regards,
Jacek Anaszewski