Re: [PATCH v11 33/37] dt-bindings: pinctrl: airoha: add support of en7523 pin controller

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] Invalid use of scalar `enum` constraints on the `groups` string-array property.
- [Medium] Missing `reg` property or `syscon` phandle for a memory-mapped hardware block.
- [Medium] The `gpio` function groups suspiciously omit `gpio0` through `gpio27`.
- [Medium] The `pins` property enum in the `conf` node is missing `gpio28` and `gpio29`.
- [Low] Use of a deprecated legacy property format for a boolean property in the example.
--

--- Patch [33]: [PATCH v11 33/37] dt-bindings: pinctrl: airoha: add support of en7523 pin controller ---
commit 0f787c98b7d55d5b83a2bca3f277b8fb38b7b78e
Author: Mikhail Kshevetskiy <[email protected]>

dt-bindings: pinctrl: airoha: add support of en7523 pin controller
    
Introduce device tree binding schema for Airoha EN7523 SoC pin controller.

> diff --git a/Documentation/devicetree/bindings/pinctrl/airoha,en7523-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/airoha,en7523-pinctrl.yaml
> new file mode 100644
> index 0000000000000..599f80e94432b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/airoha,en7523-pinctrl.yaml
> @@ -0,0 +1,378 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/airoha,en7523-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#

[ ... ]

> +        allOf:
> +          - if:
> +              properties:
> +                function:
> +                  const: pon
> +            then:
> +              properties:
> +                groups:
> +                  enum: [pon]

[Severity: Medium]
Will this fail schema validation? The groups property is defined as a
string-array in the core pinmux-node.yaml. Because the parsed instance is a
list, applying a scalar enum constraint directly to the array property will
cause validation to fail.

Should this use the items keyword to constrain the array elements? For
example:

                groups:
                  items:
                    enum: [pon]

This pattern repeats for most functions in this allOf block.

[ ... ]

> +          - if:
> +              properties:
> +                function:
> +                  const: gpio
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio28, gpio29]

[Severity: Medium]
Is it intentional to omit gpio0 through gpio27 here?

Because these pins can be muxed to alternative functions like pwm or phy
leds, device trees typically need to be able to explicitly mux them back
to their standard gpio roles.

[ ... ]

> +      '^conf(-|$)':
> +        type: object
> +
> +        description:
> +          pinconf configuration nodes.
> +
> +        $ref: /schemas/pinctrl/pincfg-node.yaml
> +
> +        properties:
> +          pins:
> +            description:
> +              An array of strings. Each string contains the name of a pin.
> +            items:
> +              enum: [i2c_sda, i2c_scl, spi_cs0, spi_clk, spi_mosi, spi_miso,
> +                     uart1_txd, uart1_rxd, gpio0, gpio1, gpio2, gpio3, gpio4,
> +                     gpio5, gpio6, gpio7, gpio8, gpio9, gpio10, gpio11, gpio12,
> +                     gpio13, gpio14, gpio15, gpio16, gpio17, gpio18, gpio19,
> +                     gpio20, gpio21, gpio22, gpio23, gpio24, gpio25, gpio26,
> +                     gpio27, pcie_reset0, pcie_reset1]
> +            minItems: 1
> +            maxItems: 38

[Severity: Medium]
Does this pins enum artificially restrict pinconf properties for gpio28
and gpio29?

The gpio function block above requires these groups, meaning they physically
exist on the hardware, but they are omitted from this list and any attempt
to configure them will be rejected.

[ ... ]

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    pinctrl {
> +      compatible = "airoha,en7523-pinctrl";
> +
> +      interrupt-parent = <&gic>;

[Severity: Medium]
Is the example missing a parent syscon node or a reg property?

The Airoha pinctrl driver relies on device_node_to_regmap() on the parent
of_node to access registers. This requires the pin controller to be
instantiated as a child of a syscon or MFD device.

The schema does not document this parent constraint, and this example
defines a standalone top-level node that will fail to probe.

> +      interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;

[ ... ]

> +      pcie1-rst-pins {
> +        conf {
> +          pins = "pcie_reset1";
> +          drive-open-drain = <1>;

[Severity: Low]
Should this be an empty flag instead of assigning an integer value?

The generic pincfg-node.yaml defines drive-open-drain as a boolean
property, so this assignment violates the boolean type constraint and
will fail validation during dt_binding_check.

> +        };
> +      };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=33