Re: [PATCH v3] dt-bindings: leds: lacie,netxbig-leds: Convert to DT schema

[Uday Kiran]

> > +properties:
> > +  compatible:
> > +    const: lacie,netxbig-leds
> > +
> > +  gpio-ext:
> > +    description: Phandle for the gpio-ext bus.
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +
> > +  timers:
> > +    description: |
> > +      Timer array. Each timer entry is represented by three integers:
> > +      Mode (gpio-ext bus), delay_on and delay_off.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +
> > +patternProperties:
> > +  "^(?!gpio-ext$|pinctrl-[0-9]+$)[a-z][a-z0-9]*(-[a-z0-9]+)+$":
>
> This is an odd pattern. Why is pinctrl an LED? or gpio-ext?
>
> Actually entire pattern is very odd - it has $ in the middle, but then
> followed with other REQUIRED stuff. I am surprised it works, but
> regardless it is absolutely not readable. Looks like a begative pattern,
> but why would you have negative pattern for known properties?

Thanks for the review Krzysztof.

The negative lookahead was added because patternProperties matches all top-
level property names that fit the pattern, so excluded known top-level
properties like gpio-ext and pinctrl-*;

In patch v1, I've used additionalProperties which removes the regex entirely
and allows arbitrary child-node names. But removed and used patternProperties
as per the review feedback.

Could you please suggest to me which way to proceed? with additionalProperties
or patternProperties

>
>
>
>
> > +    type: object
> > +    $ref: common.yaml#
> > +    unevaluatedProperties: false
> > +
> > +    properties:
> > +      label: true
> > +      linux,default-trigger: true
>
> Drop both, common.yaml provides these.

Taken care in the next revision.

>
> > +
> > +      mode-addr:
> > +        description: Mode register address on gpio-ext bus.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +      mode-val:
> > +        description: |
>
> Do not need '|' unless you need to preserve formatting.

Taken care in the next revision.

>
> > +          Mode to value mapping. Each entry is represented by two integers:
> > +          a mode and the corresponding value on the gpio-ext bus.
> > +        $ref: /schemas/types.yaml#/definitions/uint32-array
> > +
> > +      bright-addr:
> > +        description: Brightness register address on gpio-ext bus.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +      max-brightness:
> > +        description: Maximum brightness value.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +    required:
> > +      - mode-addr
> > +      - mode-val
> > +      - bright-addr
> > +      - max-brightness
> > +
> > +required:
> > +  - compatible
> > +  - gpio-ext
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/leds-netxbig.h>
> > +
> > +    netxbig-leds {
>
> Just "leds"

Taken care in the next revision.

>
> > +        compatible = "lacie,netxbig-leds";
>

Regards,
Udaya Kiran Challa