Re: [PATCH net-next v5 1/7] dt-bindings: leds: Add IS32FL3207 controller

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
dt-bindings: leds: Add IS32FL3207 controller

This adds a binding for the Lumissil IS32FL3207 18-channel constant-current
LED controller, describing I2C addressing, VCC supply, the SDB enable GPIO,
the RISET resistor and per-output current limits.  Outputs may be described
either as individual led@N nodes or grouped into multicolor nodes, where the
group node name may use either a unit address or a dash suffix.

> diff --git a/Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml b/Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml
> new file mode 100644
> index 0000000000000..2e53aa1e202dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml
> @@ -0,0 +1,281 @@

[ ... ]

> +  "^multi-led(@[0-9a-f]|-[0-9]+)?$":
> +    type: object
> +    $ref: leds-class-multicolor.yaml#
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 15
> +        description: |
> +          Optional zero-based lowest-numbered output used by a group named
> +          multi-led@N. Values 0 through 15 correspond to OUT1 through OUT16.
> +          For a multi-led-N group without reg, the driver derives the lowest
> +          output from its component nodes. This form permits groups beginning
> +          at channels 16 or 17 while following the common multicolor node-name
> +          schema.

[Severity: Low]

The single pattern here matches multi-led, multi-led@<hex> and
multi-led-<dec>, and reg is declared unconditionally optional and
unconditionally allowed for all three forms.

Since there is no allOf/if-then keyed on $nodename, can the schema still
distinguish the two node-name forms?  Two shapes appear to validate that the
description says should not exist:

    multi-led@5 {           /* unit address, but no reg */
        #address-cells = <1>;
        #size-cells = <0>;
        led@0 { ... };
    };

    multi-led-1 {           /* no unit address, but has reg */
        reg = <5>;
        #address-cells = <1>;
        #size-cells = <0>;
        led@10 { ... };
    };

dtc's unit_address_vs_reg check rejects both ("node has a unit name, but no
reg or ranges property" and "node has a reg or ranges property, but no unit
name"), but that check is off for normal dtb builds:

scripts/Makefile.dtbs:
    # Disable noisy checks by default
    ifeq ($(findstring 1,$(KBUILD_EXTRA_WARN)),)
    DTC_FLAGS += -Wno-unit_address_vs_reg \
    ...

so for a board dts built without W=1 the schema is the only thing left to
catch it.

For the first shape there is also nothing that ties the unit address to the
channels actually used, because is32fl3207_register_multicolor() only compares
reg against the derived first channel when reg is present:

    if (has_group_reg && group_reg != first_channel)
        return dev_err_probe(...);

Would a node named multi-led@5 that in fact drives channels 0..2 be silently
accepted, making the unit address meaningless?

Would something like this express the intent instead:

    allOf:
      - if:
          properties:
            $nodename:
              pattern: '^multi-led@'
        then:
          required:
            - reg
        else:
          properties:
            reg: false

or alternatively splitting the two node-name forms into separate
patternProperties entries?

[ ... ]
-- 
pw-bot: cr