lkml.org 
[lkml]   [2019]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/2] dt: bindings: lp50xx: Introduce the lp50xx family of RGB drivers
From
Date
Hi Dan,

Thank you for the patch.

Seeing the mess with regs and led-modules I think
it will be better to abide by the regs alone.

Please see below how I would structure that.

On 1/14/19 10:17 PM, Dan Murphy wrote:
> Introduce the bindings for the Texas Instruments LP5036, LP5030, LP5024 and the LP5018
> RGB LED device driver. The LP5036/3024/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
> http://www.ti.com/lit/ds/symlink/lp5036.pdf
>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>
> v2 - Added the LP5030/36 devices, defined the modules vs banked properties
> renamed the file from lp5024 to lp50xx. - https://lore.kernel.org/patchwork/patch/1026514/
>
> .../devicetree/bindings/leds/leds-lp50xx.txt | 143 ++++++++++++++++++
> 1 file changed, 143 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.txt b/Documentation/devicetree/bindings/leds/leds-lp50xx.txt
> new file mode 100644
> index 000000000000..7bc6843ddba4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.txt
> @@ -0,0 +1,143 @@
> +* Texas Instruments - LP5018/24/30/36 RGB LED driver
> +
> +The LP50XX is multi-channel, I2C RGB LED Drivers that can group RGB LEDs into
> +a LED group or control them individually.
> +
> +The difference in these RGB LED drivers is the number of supported RGB strings.

Maybe it would be better to replace "strings" with "modules"?
Strings usually mean LEDs connected in a row, which is specific
to backlights.

> +Required properties:
> + - compatible:
> + "ti,lp5018"
> + "ti,lp5024"
> + "ti,lp5030"
> + "ti,lp5036"
> + - reg : I2C slave address
> + lp5018/24 - 0x28
> + lp5030/36 - 0x30
> + - #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.

Let's change it to:

- reg : RGB LED module number. For the node describing RGB cluster bank
it should match first element of ti,led-bank array.

> +
> +Required Child properties but only one should be defined per child:
> +Either one of these two properties are required for each node. The
> +property ti,led-bank takes precedence over the ti,led-module within the same
> +node.
> +
> + - ti,led-module : This property denotes the single LED module number
> + that will be controlled in the LED class instance.

It will be not needed then.

> + - ti,led-bank : This property denotes the LED module numbers that will
> + be controlled as a single RGB cluster. Each LED module
> + number will be controlled by a single LED class instance.
> + There can only be one instance of the ti,led-bank
> + property for each device node.
> +
> +The LED outpus associated with the LED modules are defined in Table 1 of the
> +corresponding data sheets.
> +
> +LP5018 - 6 Total RGB cluster LED outputs 0-5
> +LP5024 - 8 Total RGB cluster LED outputs 0-7
> +LP5030 - 10 Total RGB cluster LED outputs 0-9
> +LP5036 - 12 Total RGB cluster LED outputs 0-11
> +
> +Optional child properties:
> + - label : see Documentation/devicetree/bindings/leds/common.txt
> + - linux,default-trigger :
> + see Documentation/devicetree/bindings/leds/common.txt
> +
> +Examples:
> +LP5018 and LP5024 example:
> +led-controller@29 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "ti,lp5024";
> + reg = <0x29>;
> + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> + vled-supply = <&vmmcsd_fixed>;
> +
> + led@0 {
> + reg = <0>;
> + label = "led1_mod";
> + ti,led-module = <1>;
> + };
> +
> + led@1 {
> + reg = <1>;
> + label = "banked_leds";
> + ti,led-bank = <0 2 5 3 >;
> + };
> +
> + led@2 {
> + reg = <2>;
> + label = "led4_mod";
> + ti,led-module = <4>;
> + };
> +
> + led@3 {
> + reg = <3>;
> + label = "led7_mod";
> + ti,led-module = <7>;
> + };
> +
> + led@4 {
> + reg = <4>;
> + label = "led6_mod";
> + ti,led-module = <6>;
> + };

Without ti,led-module it will look like below:


led@0 {
reg = <0>;
label = "banked_leds";
ti,led-bank = <0 2 5 3 >;
};

led@1 {
reg = <1>;
label = "led1_mod";
};


led@4 {
reg = <4>;
label = "led4_mod";
};

led@6 {
reg = <6>;
label = "led6_mod";
};

led@7 {
reg = <7>;
label = "led7_mod";
};



> +};
> +
> +LP5030 and LP5036 example:
> +led-controller@30 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "ti,lp5036";
> + reg = <0x30>;
> + enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> + vled-supply = <&vmmcsd_fixed>;
> +
> + led@0 {
> + reg = <0>;
> + label = "led1_mod";
> + ti,led-module = <1>;
> + };
> +
> + led@1 {
> + reg = <1>;
> + label = "led_banked";
> + ti,led-bank = <0 2 5 3 9 10 >;
> + };
> +
> + led@2 {
> + reg = <2>;
> + label = "led4_mod";
> + ti,led-module = <4>;
> + };
> +
> + led@3 {
> + reg = <3>;
> + label = "led7_mod";
> + ti,led-module = <7>;
> + };
> +
> + led@4 {
> + reg = <4>;
> + label = "led6_mod";
> + ti,led-module = <6>;
> + };
> +
> + led@5 {
> + reg = <5>;
> + label = "led8_mod";
> + ti,led-module = <8>;
> + };
> +};
> +
> +
> +For more product information please see the link below:
> +http://www.ti.com/lit/ds/symlink/lp5024.pdf
> +http://www.ti.com/lit/ds/symlink/lp5036.pdf
>

--
Best regards,
Jacek Anaszewski

\
 
 \ /
  Last update: 2019-01-15 22:42    [W:0.116 / U:64.388 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site