From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH 1/2] dt: bindings: lp5024: Introduce the lp5024 and lp5018 RGB driver Date: Thu, 10 Jan 2019 23:03:26 +0100 Message-ID: <459a4d7a-980b-5a46-9bd8-7a7afb37e1c3@gmail.com> References: <20181219162626.12297-1-dmurphy@ti.com> <20181219162626.12297-2-dmurphy@ti.com> <2d2d5dcd-9c23-e901-daac-9b79aa5a5e82@ti.com> <6c62956e-7789-58ba-5437-f2e033f2825c@gmail.com> <366cbf6d-94fa-fea9-be58-07ddb09cff3a@ti.com> <1702dfd6-b08f-c1ff-e46d-1366618bedb0@gmail.com> <72112839-11d4-54be-df94-b2322f77cb0f@ti.com> <8b126077-c200-ed34-03b7-6d43a22fb0c9@gmail.com> <92cc81dc-7280-8bf0-9536-9c4d990eaf3b@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <92cc81dc-7280-8bf0-9536-9c4d990eaf3b@ti.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Dan Murphy , robh+dt@kernel.org, pavel@ucw.cz Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org List-Id: devicetree@vger.kernel.org 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 >>>>>>>>>>>>> --- >>>>>>>>>>>>>        .../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. >>> 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? 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? > 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. I would also add Table 1 contents (Bank Number and LED Number Assignment) to the DT bindings. > If you define LED0 and LED1 as banked then OUT0->5 are banked and it would be considered a single virtual output. > > LED_CONFIG0 even uses the modular approach to define what is banked and what is not. With the modular approach to led-sources > the mapping of the sources to the CONFIG register become 1-1. > > 1 = LED7 bank control mode enabled > 0 = LED7 independent control mode enabled > -- Best regards, Jacek Anaszewski