lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 26 May 2020 19:59:05 -0600
From:   Rob Herring <robh@...nel.org>
To:     Dan Murphy <dmurphy@...com>
Cc:     jacek.anaszewski@...il.com, pavel@....cz,
        devicetree@...r.kernel.org, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v25 03/16] dt: bindings: lp50xx: Introduce the lp50xx
 family of RGB drivers

On Tue, May 26, 2020 at 11:46:39AM -0500, Dan Murphy wrote:
> Introduce the bindings for the Texas Instruments LP5036, LP5030, LP5024,
> LP5018, LP5012 and LP5009 RGB LED device driver.  The LP5036/30/24/18/12/9
> 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/lp5012.pdf
> http://www.ti.com/lit/ds/symlink/lp5024.pdf
> http://www.ti.com/lit/ds/symlink/lp5036.pdf
> 
> Acked-by: Jacek Anaszewski <jacek.anaszewski@...il.com>
> Signed-off-by: Dan Murphy <dmurphy@...com>
> ---
>  .../devicetree/bindings/leds/leds-lp50xx.yaml | 180 ++++++++++++++++++
>  1 file changed, 180 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> new file mode 100644
> index 000000000000..a2ea03e07f6d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lp50xx.yaml
> @@ -0,0 +1,180 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-lp50xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LED driver for LP50XX RGB LED from Texas Instruments.
> +
> +maintainers:
> +  - Dan Murphy <dmurphy@...com>
> +
> +description: |
> +  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
> +  modules.
> +
> +  For more product information please see the link below:
> +  http://www.ti.com/lit/ds/symlink/lp5012.pdf
> +  http://www.ti.com/lit/ds/symlink/lp5024.pdf
> +  http://www.ti.com/lit/ds/symlink/lp5036.pdf
> +
> +properties:
> +  compatible:
> +     oneOf:
> +      - const: ti,lp5009
> +      - const: ti,lp5012
> +      - const: ti,lp5018
> +      - const: ti,lp5024
> +      - const: ti,lp5030
> +      - const: ti,lp5036

Use enum rather than oneOf+const.

> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      I2C slave address
> +      lp5009/12 - 0x14, 0x15, 0x16, 0x17
> +      lp5018/24 - 0x28, 0x29, 0x2a, 0x2b
> +      lp5030/36 - 0x30, 0x31, 0x32, 0x33
> +
> +  enable-gpios:
> +    description: GPIO pin to enable/disable the device.

How many? (maxItems: 1)

> +
> +  vled-supply:
> +    description: LED supply.
> +
> +  child-node:

This literally requires a node called 'child-node'. Not what you want.

You need a $ref to the multi-color schema in here and then only define 
what's specific to this chip.

> +    type: object
> +    properties:
> +      reg:
> +        description: This is the LED module number.

Constraints?

> +
> +      color:
> +        description: Must be LED_COLOR_ID_MULTI
> +
> +      function:
> +        description: see Documentation/devicetree/bindings/leds/common.txt
> +
> +      ti,led-bank:
> +        description:
> +          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.  This is a required node is the LED
> +          modules are to be backed.
> +        $ref: /schemas/types.yaml#definitions/uint32-array

What is reg then? Some made up index? Can't you do:

reg = <1 2 3>;
led@1 {};
led@2 {};
led@2 {};

> +
> +    required:
> +      - reg
> +      - color
> +      - function
> +
> +  grandchild-node:

Again, no.

> +    type: object
> +    properties:
> +      reg:
> +        description:
> +          A single entry denoting the LED output that controls the monochrome LED.

Constraints?

> +
> +      color:
> +        description:
> +          see Documentation/devicetree/bindings/leds/common.txt

Have you read this file recently? Don't add new references to it. (And 
generally freeform references to other files are wrong with schemas).

> +
> +      led-sources:
> +        description:
> +          see Documentation/devicetree/bindings/leds/common.txt
> +          The LED outputs associated with the LED modules are defined in Table 1
> +          of the corresponding data sheets.
> +          LP5009 - 3 Total RGB cluster LED outputs 0-2
> +          LP5012 - 4 Total RGB cluster LED outputs 0-3
> +          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
> +
> +      label:
> +        description: |
> +          Optional node - see Documentation/devicetree/bindings/leds/common.txt
> +
> +      linux,default-trigger:
> +        description: |
> +          Optional node - see Documentation/devicetree/bindings/leds/common.txt
> +
> +    required:
> +      - reg
> +      - color
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/leds/common.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        led-controller@14 {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          compatible = "ti,lp5009";
> +          reg = <0x14>;
> +          enable-gpios = <&gpio1 16>;
> +          multi-led@1 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <1>;
> +            color = <LED_COLOR_ID_MULTI>;
> +            function = LED_FUNCTION_CHARGING;
> +
> +            led@0 {
> +              reg = <0>;
> +              color = <LED_COLOR_ID_RED>;
> +            };
> +
> +            led@1 {
> +              reg = <1>;
> +              color = <LED_COLOR_ID_GREEN>;
> +            };
> +
> +            led@2 {
> +              reg = <2>;
> +              color = <LED_COLOR_ID_BLUE>;
> +            };
> +          };
> +
> +          multi-led@2 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <2>;
> +            color = <LED_COLOR_ID_MULTI>;
> +            function = LED_FUNCTION_STANDBY;
> +            ti,led-bank = <2 3 5>;
> +
> +            led@6 {
> +              reg = <0x6>;
> +              color = <LED_COLOR_ID_RED>;
> +              led-sources = <6 9 15>;
> +            };
> +
> +            led@7 {
> +              reg = <0x7>;
> +              color = <LED_COLOR_ID_GREEN>;
> +              led-sources = <7 10 16>;
> +            };
> +
> +            led@8 {
> +              reg = <0x8>;
> +              color = <LED_COLOR_ID_BLUE>;
> +              led-sources = <8 11 17>;
> +            };
> +         };
> +       };
> +    };
> +
> +...
> -- 
> 2.25.1
> 

Powered by blists - more mailing lists