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
