Re: [PATCH 2/2] dt-bindings: leds: add mp3326

[Krzysztof Kozlowski]

On 09/08/2023 08:39, Yuxi (Yuxi) Wang wrote:
> Add dt-bindings for Monolithic Power System MP3326.
> 
> Signed-off-by: Yuxi Wang <Yuxi.Wang@xxxxxxxxxxxxxxxxxxx>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by
automated tooling. Performing review on untested code might be a waste
of time, thus I will skip this patch entirely till you follow the
process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

> ---
>  .../devicetree/bindings/leds/leds-mp3326.yaml | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-mp3326.yaml
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-mp3326.yaml b/Documentation/devicetree/bindings/leds/leds-mp3326.yaml
> new file mode 100644
> index 000000000000..3a059340b902
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-mp3326.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-mp3326.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LED driver for MP3326 from Monolithic Power Systems.

Drop final dot.

> +
> +maintainers:
> +  - Yuxi Wang <Yuxi.Wang@xxxxxxxxxxxxxxxxxxx>
> +
> +description: |
> +  Bindings for the Monolithic Power Systems MP3326 LED Drivers.

Drop "Bindings for"

> +
> +  For more product information please see the link below:
> +    https://www.monolithicpower.com/en/products/mp3326.html

Missing blank line.

> +properties:
> +  compatible:
> +    const: MPS,MP3326

Do you see anywhere, absolutely anywhere capital letters in compatibles?

> +
> +  reg:
> +    description: I2C slave address of the controller.

Drop description, obvious.

> +    maxItems: 1
> +
> +  led-protect:
> +    description: LED short protection threshold.

Does not look like common property... missing vendor prefix. Use common
unit suffix, so "-microvolt"

> +    enum:
> +      - 0 #2V
> +      - 1 #3V
> +      - 2 #4V
> +      - 3 #5V
> +
> +  switch_status:
> +    description: Master switch for all channels.
> +    enum:
> +      - 0 #close all channels
> +      - 1 #open all channels

This is so bad that actually disappointing...

1. Missing vendor prefix
2. No underscores in properties
3. Missing type/ref
4. And does not look at all as hardware property. Drop.


> +
> +patternProperties:
> +  "^rgb(-[0-9a-f]+)?$":

Aren't these called "led"?

> +    description: RGB group.
> +    type: object
> +    unevaluatedProperties: false

Missing ref to proper LED schema.

> +    properties:
> +      rgb_r:

Nope, nope.

> +        description: Red light of the RGB group.
> +        maxItems: 16
> +        minItems: 1
> +      rgb_g:
> +        description: Green light of the RGB group.
> +        maxItems: 16
> +        minItems: 1
> +      rgb_b:
> +        description: Blue light of the RGB group.
> +        maxItems: 16
> +        minItems: 1
> +      brightness:
> +        description: Brightness of the RGB group.
> +        maxItems: 63
> +        minItems: 0
> +      required:
> +        - rgb_r
> +        - rgb_g
> +        - rgb_b
> +        - brightness
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +    MP3326@30 {

No, this is neither DTS nor proper bindings. Remove all this. Start FROM
SCRATCH from example-schema.yaml. Entirely from scratch.

> +        compatible = "mps,MP3326";
> +        reg = <0x30>;
> +        led-protect =<3>;
> +        switch_status=<1>;
> +
> +        /*RGB group 1*/
> +        rgb1@0{
> +            rgb_r=<1>;

This is not even coding style for DTS...


Best regards,
Krzysztof