Re: [PATCH 1/2] dt: bindings: lp5024: Introduce the lp5024 and lp5018 RGB driver

[Dan Murphy <dmurphy@ti.com>]

Jacek

On 1/10/19 12:44 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> On 1/9/19 10:31 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 1/9/19 3:28 PM, Jacek Anaszewski wrote:
>>> On 1/9/19 10:12 PM, Dan Murphy wrote:
>>>> On 1/9/19 2:12 PM, Jacek Anaszewski wrote:
>>>>> Hi Dan,
>>>>>
>>>>> On 1/8/19 10:22 PM, Dan Murphy wrote:
>>>>>> On 1/8/19 3:16 PM, Jacek Anaszewski wrote:
>>>>>>> On 1/8/19 9:53 PM, Dan Murphy wrote:
>>>>>>>> Jacek
>>>>>>>>
>>>>>>>> On 1/8/19 2:33 PM, Jacek Anaszewski wrote:
>>>>>>>>> Dan,
>>>>>>>>>
>>>>>>>>> On 12/19/18 5:26 PM, Dan Murphy wrote:
>>>>>>>>>> Introduce the bindings for the Texas Instruments LP5024 and the LP5018
>>>>>>>>>> RGB LED device driver.  The LP5024/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
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>>>>>>>> ---
>>>>>>>>>>       .../devicetree/bindings/leds/leds-lp5024.txt  | 63 +++++++++++++++++++
>>>>>>>>>>       1 file changed, 63 insertions(+)
>>>>>>>>>>       create mode 100644 Documentation/devicetree/bindings/leds/leds-lp5024.txt
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp5024.txt b/Documentation/devicetree/bindings/leds/leds-lp5024.txt
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..9567aa6f7813
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lp5024.txt
>>>>>>>>>> @@ -0,0 +1,63 @@
>>>>>>>>>> +* Texas Instruments - LP5024/18 RGB LED driver
>>>>>>>>>> +
>>>>>>>>>> +The LM3692x is an ultra-compact, highly efficient,
>>>>>>>>>> +white-LED driver designed for LCD display backlighting.
>>>>>>>>>> +
>>>>>>>>>> +The main difference between the LP5024 and L5018 is the number of
>>>>>>>>>> +RGB LEDs they support.  The LP5024 supports twenty four strings while the
>>>>>>>>>> +LP5018 supports eighteen strings.
>>>>>>>>>> +
>>>>>>>>>> +Required properties:
>>>>>>>>>> +    - compatible:
>>>>>>>>>> +        "ti,lp5018"
>>>>>>>>>> +        "ti,lp5024"
>>>>>>>>>> +    - reg :  I2C slave address
>>>>>>>>>> +    - #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.
>>>>>>>>>> +    - led-sources : LP5024 - 0 - 7
>>>>>>>>>> +            LP5018 - 0 - 5
>>>>>>>>>> +            Declares the LED string or strings that the child node
>>>>>>>>>> +            will control.  If ti,control-bank is set then this
>>>>>>>>>> +            property will contain multiple LED IDs.
>>>>>>>>>> +
>>>>>>>>>> +Optional child properties:
>>>>>>>>>> +    - label : see Documentation/devicetree/bindings/leds/common.txt
>>>>>>>>>> +    - linux,default-trigger :
>>>>>>>>>> +       see Documentation/devicetree/bindings/leds/common.txt
>>>>>>>>>> +    - ti,control-bank : Indicates that the LED strings declared in the
>>>>>>>>>> +                led-sources property are grouped within a control
>>>>>>>>>> +                bank for brightness and mixing control.
>>>>>>>>>> +
>>>>>>>>>> +Example:
>>>>>>>>>> +
>>>>>>>>>> +led-controller@28 {
>>>>>>>>>> +    compatible = "ti,lp5024";
>>>>>>>>>> +    reg = <0x28>;
>>>>>>>>>> +    #address-cells = <1>;
>>>>>>>>>> +    #size-cells = <0>;
>>>>>>>>>> +
>>>>>>>>>> +    enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>>>>>>>>>> +    vled-supply = <&vbatt>;
>>>>>>>>>> +
>>>>>>>>>> +    led@0 {
>>>>>>>>>> +        reg = <0>;
>>>>>>>>>> +        led-sources = <1>;
>>>>>>>>>> +    };
>>>>>>>>>> +
>>>>>>>>>> +    led@1 {
>>>>>>>>>> +        reg = <1>;
>>>>>>>>>> +        led-sources = <0 6>;
>>>>>>>>>> +        ti,control-bank;
>>>>>>>>>
>>>>>>>>> Do you really need ti,control-bank? Doesn't led-sources array size
>>>>>>>>> greater than 1 mean that the node describes control bank?
>>>>>>>>>
>>>>>>>>
>>>>>>>> That will work too.
>>>>>>>
>>>>>>>>> Also, does it make sense to have only two LEDs in the bank?
>>>>>>>>
>>>>>>>> The array can populate all 7 LEDs in a single node.  I only show 2 here as the example.
>>>>>>>> See the description above of the led-sources
>>>>>>>
>>>>>>> OK, I confused RGB LED modules with banks.
>>>>>>>
>>>>>>> Shouldn't we allow for defining either strings or RGB LED
>>>>>>> triplets somehow then?
>>>>>>>
>>>>>>
>>>>>> Well that is what this should be doing.  If you define a single LED in LED sources then
>>>>>> the triplet is controlled via the associated LEDx_brightness register.
>>>>>
>>>>> led-sources should map to iouts directly.
>>>>> So, for RGB LED modules I would expect:
>>>>>
>>>>> LED0: led-sources = <0 1 2>;
>>>>> LED1: led-sources = <3 4 5>;
>>>>> LED2: led-sources = <6 7 8>;
>>>>> and so on.
>>>>
>>>>>
>>>>> for banks:
>>>>>
>>>>> Bank A with iouts 0,3,6,9: led-sources<0 3 6 9>;
>>>>> Bank B with iouts 2,4,10:  led-sources<2 4 10>;
>>>>> Bank C with iouts 5,8,11,14,17: led-sources<5 8 11 14 17>;
>>>>>
>>>>
>>>> Ok the led-sources would need to be different then this as I don't define the sources for banks.
>>>>
>>>> The led-sources for the banks and the individual groups will have different meanings within the same
>>>> document.  I was attempting to keep the led-sources mapped to the LEDx_brightness registers as opposed to
>>>> the hardware outputs since the RGB LEDs are controlled and grouped by a single brightness register and if banked then
>>>> it would be controlled by the bank brightness register.
>>>>
>>>> Describing these in the DT seems wrought with potential issues as the data sheet defines what outputs map to what bank and LED
>>>> registers.
>>>
>>> Yes, that's why I mentioned the need for validation of led-sources.
>>> But they have to be iouts. This property was introduced specifically
>>> for such purposes.
>>>
>>
>> Yes Pavel also mentioned that as well.
>>
>> I will look into validating the sources.  But there will be no mapping of the sources to the output that is done
>> in the hardware.  This would just be a data sheet mapping since the outputs are not configurable.
> 
> Hmm, isn't the mapping defined in the hardware via LED_CONFIG0 register?
> I have an impression that it defines whether LED belongs to an RGB LED
> module or to a bank. Basing on that I created my DT example above.
> 

Yes so if you turn on the bank control for LED0 and LED1 then 
out 0, 3 are mapped to BANK A
out 1, 4 are mapped to BANK B
out 2, 5 are mapped to BANK C

All done automatically in the hardware and the LED0_BRIGHTNESS and LED1_BRIGHTNESS registers have no affect on the brightness

If we grouped the LEDs into a bank the led-sources would look more like this
led-sources = < 0 1 2 3 4 5 >;
ti,control-bank; // But this can be omitted as led-sources is greater then 3

non-banked case would be
led-sources = < 0 1 2 >;

But the actual OUT numbers don't matter in the bank case unless we do the validation.  There would need to be an algorithim
that translates these output to the correct LEDx register and CONFIG0 bits.  Basically if OUT0 is mapped to the bank then OUT1 and OUT2
are inherently mapped to the bank.  They cannot be separated so the device theoretically treats the RGB group as a single LED.  And
when banked it treats the groups of RGBs that are defined as a single LED.

This is why it was easier use the LEDx out as the virtual out as we only need to define the group number(s) that are controled by the
LED file presented to the user space.

Dan

-- 
------------------
Dan Murphy