Message-ID: <9c129b00-39e6-dd29-c2a7-0506a1780fb8@gmail.com>
Date:   Mon, 14 Jan 2019 21:11:35 +0100
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Dan Murphy <dmurphy@...com>, robh+dt@...nel.org, pavel@....cz
Cc:     devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-leds@...r.kernel.org
Subject: Re: [PATCH 1/2] dt: bindings: lp5024: Introduce the lp5024 and lp5018
 RGB driver

Hi Dan,

On 1/14/19 1:27 PM, Dan Murphy wrote:
> Jacek
> 
> On 1/12/19 1:48 PM, Jacek Anaszewski wrote:
>> Hi Dan,
>>
>> On 1/12/19 6:09 PM, Dan Murphy wrote:
>>> Jacek
>>>
>>> On 1/11/19 3:52 PM, Jacek Anaszewski wrote:
>>>> Dan,
>>>>
>>>> On 1/11/19 1:38 PM, Dan Murphy wrote:
>>>>> Jacek
>>>>>
>>>>> Sorry I missed some replies
>>>>>
>>>>> On 1/10/19 4:03 PM, Jacek Anaszewski wrote:
>>>>>> On 1/10/19 9:43 PM, Dan Murphy wrote:
>>>>>>> Jacek
>>>>>>>
>>>>>>> On 1/10/19 1:57 PM, Jacek Anaszewski wrote:
>>>>>>>> Dan,
>>>>>>>>
>>>>>>>> On 1/10/19 8:22 PM, Dan Murphy wrote:
>>>>>>>>> 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@...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
>>>>>>>>
>>>>>>>> Just noticed that I made a mistake in my example, it should have been:
>>>>>>>>
>>>>>>>> Bank B with iouts 1,4,10:  led-sources<1 4 10>;
>>>>>>>>
>>>>>>>>> out 2, 5 are mapped to BANK C
>>>>>>>>
>>>>>>>> Correct.
>>>>>>>>
>>>>>>>>> All done automatically in the hardware and the LED0_BRIGHTNESS and LED1_BRIGHTNESS registers have no affect on the brightness
>>>>>>>>
>>>>>>>> That's right.
>>>>>>>>
>>>>>>>>> If we grouped the LEDs into a bank the led-sources would look more like this
>>>>>>>>> led-sources = < 0 1 2 3 4 5 >;
>>>>>>>>
>>>>>>>> Why? This would be a mix of three banks. Like you listed above.
>>>>>>>> I'm still interpreting led-sources elements as iout identifiers.
>>>>>>>>
>>>>>>>
>>>>>>> I am as well but as I tried to explain that if you define OUT0 as bank controlled then OUT1 and OUT2 are also bank controlled
>>>>>>> within the hardware.  We have no control of that.  If BIT(0) and BIT(1) are set in the LED_CONFIG0 register then OUT0, 1, 2, 3, 4 and 5 are all bank controlled.
>>>>>>
>>>>>> There is naming conflict I noticed just now - LEDn_BANK_EN bits
>>>>>> in LED_CONFIG0 register enable RGB LED modules, and not BANKs (A,B,C).
>>>>>>
>>>>>>> These OUTPUTs will appear as a single RGB LED grouping.
>>>>>>
>>>>>> Single? W would rather expect that we get two RGB LED modules, whose
>>>>>> brightness will be controlled via LED0_BRIGHTNESS and LED1_BRIGHTNESS
>>>>>> registers respectively.
>>>>>>
>>>>>>>>> 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 >;
>>>>>>>>
>>>>>>>> Agreed here. It would be LED0 RGB LED module.
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> To three separate banks, right?
>>>>>>>> OUT0 - bank A, OUT1 - bank B, OUT2 - bank C.
>>>>>>>
>>>>>>> Yes but there is no BANK output pin just like there is no dedicated LEDn output pin.  The banks are grouped internally to the device
>>>>>>> so again if OUT0 and OUT3 are defined as banked then 1, 2, 4, and 5 are all mapped to the bank.  1 BANK brightness register and 3 bank
>>>>>>> color adjustment registers.
>>>>>>
>>>>>> Here as above, I would expect two separate banks - LED0 and LED1.
>>>>>> Moreover - not 3 color adjustment registers, but six - one per iout:
>>>>>> OUT0_COLOR to OUT5_COLOR.
>>>>>>
>>>>>
>>>>> When the LEDs are banked the banked LEDs are controlled by the bank registers not the LEDx registers
>>>>> so you should only see 3 color adjustments on the banked LEDs.
>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> I suspect there is logical clash here due to interpreting
>>>>>>>> led-sources elements as iouts in one case and LEDn modules
>>>>>>>> in the other case.
>>>>>>>>
>>>>>>>
>>>>>>> Yes.  When the RGBs are banked you have to think of them as a single RGB LED cluster and not as separate RGB LED clusters.
>>>>>>
>>>>>> We have RGB LED modules (enabled with LEDn_Bank_EN bits) and three
>>>>>> banks (A,B,C), which are enabled by default, am I right?
>>>>>
>>>>> No.  Independent LED modules are enabled by default.  You have to explicitly enable the banks.
>>>>>
>>>>>>
>>>>>> Bank A iouts: 0, 3 ,6, 9, 12, 15
>>>>>> Bank B iouts: 1, 4, 7, 10, 13, 16
>>>>>> Bank C iouts: 2, 5, 8, 11, 14, 17
>>>>>>
>>>>>> When RGB LED module is enabled (via LEDn_Bank_EN bit),
>>>>>> the BANK_{A.B,C}_COLOR and BANK_BRIGHTNESS registers
>>>>>> lose control over related IOUTs in favour of LEDn_BRIGHTNESS and
>>>>>> related OUTn_COLOR registers. Is it correct?
>>>>>
>>>>> No it is the opposite.  When the bit is enabled LED banking is enabled and the BANK brightness and color registers over
>>>>> ride the LEDx color and brightness registers.
>>>>>
>>>>> Default is independent control of the RGB via the LEDx color and brightness registers.
>>>>>
>>>>>>
>>>>>>> As you know the brightness is controlled by the single BANK_BRIGHTNESS register.  So identifying each output in the led-sources is
>>>>>>> misleading as the hardware does this all on the chip.  This is why I just mapped each output to the Virtual LEDx module.
>>>>>>
>>>>>> Ekhm, I messed something here.
>>>>>>
>>>>>> So for this I would define a single LED class device.
>>>>>> Related DT node would not need led-sources at all,
>>>>>> but only ti,control-bank. The semantics would be:
>>>>>> controls all iouts not taken by RGB LED modules.
>>>>>>
>>>>>
>>>>> Hmm.  I guess I will put that on hold until you read the responses.  I am not sure that would work or
>>>>> that would be really clean.  I still believe that mapping led-sources to the LEDx module number is the cleanest
>>>>> simplest solution since the driver cannot inter mix different outputs for enablement.
>>>>
>>>> I've read the doc again more carefully and hopefully I finally have
>>>> proper understanding. Let's check it.
>>>>
>>>> 1. On reset LED_CONFIG0 bits are zeroed, which means
>>>>      LEDn module independent control mode.
>>>> 2. LEDn modules (i.e. IOUT triplets) are controlled independently,
>>>>      with use of LEDn_BRIGHTNESS registers, and each IOUT color can
>>>>      be adjusted using OUTn_CONTROL registers.
>>>> 3. LEDn_Bank_EN bits, when set to 1, assign given RGB LED module
>>>>      to one global bank, controlled via BANK_BRIGHTNESS and BANK_n_COLOR
>>>>      registers.
>>>>
>>>> Having that, I'd see led-sources definitions as follows
>>>> (led-sources element is IOUT identifier)
>>>>
>>>> 1.
>>>>
>>>> - LED0, LED1, LED2, LED3 modules controlled by separate
>>>>     LED class devices
>>>>
>>>> led-sources = <0 1 2>   // LED0
>>>> led-sources = <3 4 5>   // LED1
>>>> led-sources = <6 7 8>   // LED2
>>>> led-sources = <9 10 11> // LED3
>>>>
>>>> 2.
>>>>
>>>> - LED0 and LED3 modules assigned to the bank, and controlled
>>>>     by one LED class device,
>>>> - LED1 and LED2 modules controlled by separate LED class devices
>>>>
>>>> led-sources = <0 1 2 9 10 11> // Bank with LED0 and LED3
>>>> led-sources = <3 4 5>         // LED1
>>>> led-sources = <6 7 8>         // LED2
>>>>
>>>>
>>>> So now I see your point. It would be indeed easier
>>>> to switch to LEDn module identifiers for led-sources
>>>> elements. With that the definitions would look like
>>>> this:
>>>>
>>>>
>>>> 1.
>>>>
>>>> - LED0, LED1, LED2, LED3 modules controlled by separate
>>>>     LED class devices
>>>>
>>>> led-sources = <0>   // LED0
>>>> led-sources = <1>   // LED1
>>>> led-sources = <2>   // LED2
>>>> led-sources = <3>   // LED3
>>>>
>>>> 2.
>>>>
>>>> - LED0 and LED3 modules assigned to the bank, and controlled
>>>>     by one LED class device,
>>>> - LED1 and LED2 modules controlled by separate LED class devices
>>>>
>>>> led-sources = <0 3> // Bank with LED0 and LED3
>>>> led-sources = <1>   // LED1
>>>> led-sources = <2>   // LED2
>>>>
>>>
>>> This is exactly how I submitted the code.
>>>
>>>>
>>>> But, I don't think use of led-sources is justified in
>>>> this case. I propose to introduce device specific properties:
>>>>
>>>> ti,led-module and ti,led-bank
>>>>
>>>> With that we would have:
>>>>
>>>> ti,led-bank = <0 3>   // Bank with LED0 and LED3 modules
>>>> ti,led-module = <1>   // LED1
>>>> ti.led-module = <2>   // LED2
>>>>
>>>
>>> We are now aligned.  I can change the led-sources to the TI specific if there are no further objections.
>>> In doing this I can eliminate the ti,control-bank property.
>>>
>>>>
>>>>>> I would also add Table 1 contents (Bank Number and LED Number
>>>>>> Assignment) to the DT bindings.
>>>>>>
>>>>>
>>>>> Should I add that to the DT binding or reference the data sheet table since this driver will support 4 different devices
>>>>> with varying number of outputs from 18-36.
>>>>
>>>> My first thought was to show full table, but four different
>>>> mappings would add too much noise. So the reference to the data
>>>> sheet should suffice.
>>>>
>>>
>>> OK
>>>
>>> One last question I am going to add the LP5036 and 30 which have the same technology but slightly different register maps.
>>> Should I rename the driver to LP5036.c as the 30, 24 and 18 would technically be subsets?
>>
>> How about leds-lp50xx.c ? You can also create a library like
>> drivers/leds/leds-lp55xx-common.c if that would simplify the code.
>>
> 
> A library would be overkill.
> Is it just the DT that we don't want to use wild cards in naming?

DT is for concrete board and cpu, so it doesn't make sense to
use wildcards in *.dts file names.

> leds-lp50xx.c is a fine name to me.

Apart of that, I've been also mulling over if we shouldn't go for single
"color" sysfs file for setting r,g,b components at one go.
I don't see any downsides. There is no risk that number of elements will
grow, and the benefit will be an atomic way of setting color - the
feature people are looking for. Vesa was mentioning the case where lack
of it had been a real problem [0].

Let's check the usability of such interface.

[0] https://lkml.org/lkml/2019/1/4/519

-- 
Best regards,
Jacek Anaszewski

Powered by blists - more mailing lists