Re: [RFC net-next 1/4] dt-bindings: leds: Add IS32FL3207 controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Arbitrary restriction of standard LED properties (`default-state`, `default-brightness`, `retain-state-shutdown`).
- [Medium] The `max-brightness` limit contradicts the 16-bit PWM capability of the hardware.
- [Low] MAINTAINERS file entry breaks strict alphabetical ordering.
--

--- Patch [1]: [RFC net-next 1/4] dt-bindings: leds: Add IS32FL3207 controller ---
commit 7a01cdde2c240d5fb659f162c75c91152d916f4e
Author: Ahmad Byagowi <ahmadexp@gmail.com>

dt-bindings: leds: Add IS32FL3207 controller
    
This commit adds a new device tree binding for the Lumissil IS32FL3207
18-channel LED controller. It describes the controller properties, such as
I2C addressing, output groupings, and current limits.

> diff --git a/Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml b/Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml
> @@ -0,0 +1,204 @@
[ ... ]
> +patternProperties:
> +  "^led@[0-9a-f]+$":
> +    type: object
> +    $ref: common.yaml#
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 17
> +        description: LED output number.
> +
> +      led-max-microamp:
> +        minimum: 1
> +        maximum: 38250
> +
> +      color:
> +        not:
> +          enum: [8, 9]
> +
> +      max-brightness:
> +        minimum: 1
> +        maximum: 255

[Severity: Medium]
Since the commit message states this hardware has up to 16-bit PWM control,
does limiting max-brightness to 255 artificially restrict the hardware's PWM
resolution? Should this allow values up to 65535 instead?

> +
> +      default-state:
> +        const: "off"
> +
> +      default-brightness: false
> +
> +      retain-state-shutdown: false

[Severity: Medium]
Are these standard LED property restrictions necessary? Restricting
default-state to off and disabling default-brightness and
retain-state-shutdown entirely prevents device trees from configuring these
LEDs to be on at boot or retain their state.

> +
> +    required:
> +      - reg
> +      - led-max-microamp
> +
> +  "^multi-led@[0-9a-f]+$":
> +    type: object
> +    $ref: leds-class-multicolor.yaml#
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        minimum: 0
> +        maximum: 17
> +        description: Lowest-numbered LED output used by the group.
> +
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +      max-brightness:
> +        minimum: 1
> +        maximum: 255

[Severity: Medium]
Does this 8-bit limit also restrict the 16-bit PWM capabilities for multicolor
LED groups?

> +
> +      default-state:
> +        const: "off"
> +
> +      default-brightness: false
> +
> +      retain-state-shutdown: false

[Severity: Medium]
Should multicolor LEDs be permitted to use standard LED state configurations
like default-brightness and retain-state-shutdown, similar to the standalone
LEDs above?

[ ... ]
> diff --git a/MAINTAINERS b/MAINTAINERS
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13821,6 +13821,12 @@ F:	Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>  F:	Documentation/hwmon/isl28022.rst
>  F:	drivers/hwmon/isl28022.c
>  
> +ISSI IS32FL3207 LED DRIVER
> +M:	Ahmad Byagowi <ahmadexp@gmail.com>
> +L:	linux-leds@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml
> +
>  ISOFS FILESYSTEM

[Severity: Low]
This isn't a bug, but does inserting ISSI before ISOFS break the strict
alphabetical ordering of the MAINTAINERS file?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785684064.git.ahmadexp@gmail.com?part=1