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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The `gpio-ranges` property is functionally required by the driver but is missing from the `required` list in the binding schema.
- [Medium] The `groups` property incorrectly uses `enum` instead of `items: { enum: ... }` for multi-pin functions like `pwm`, `pon`, `mdio`, and `phy*_led*`.
- [Medium] The `airoha,chip-scu` phandle property is utilized by the driver code but is not documented in the schema.
- [Medium] The schema uses `additionalProperties: false` while also importing standard definitions via `$ref: pinctrl.yaml#`.
- [Low] The 'drive-strength' description erroneously uses Xilinx 'MIO' terminology.
- [Low] The example uses a deprecated integer syntax for the 'drive-open-drain' boolean property.
--

--- Patch [33]: [PATCH v8 33/35] dt-bindings: pinctrl: airoha: add support of en7523 pin controller ---
commit 4bab62ab4565097d36cb334e8d39b5c03b4424c9
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

dt-bindings: pinctrl: airoha: add support of en7523 pin controller

This patch adds 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..db8e39af1a152
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/airoha,en7523-pinctrl.yaml
> @@ -0,0 +1,375 @@
> +# 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#
> +
> +title: Airoha EN7523 Pin Controller
> +
> +maintainers:
> +  - Lorenzo Bianconi <lorenzo@kernel.org>
> +
> +description:
> +  The Airoha's EN7523 Pin controller is used to control SoC pins.
> +
> +properties:
> +  compatible:
> +    const: airoha,en7523-pinctrl
> +
> +  interrupts:
> +    maxItems: 1

[Severity: Medium]
Should the airoha,chip-scu phandle property be documented in this schema?

The driver code in airoha_pinctrl_probe() appears to resolve a system
controller regmap using syscon_regmap_lookup_by_phandle() with this property.
Since additionalProperties is set to false, would users who provide this
optional phandle encounter a schema validation error?

[ ... ]

> +allOf:
> +  - $ref: pinctrl.yaml#
> +
> +required:
> +  - compatible
> +  - interrupts
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - interrupt-controller
> +  - "#interrupt-cells"

[Severity: High]
Does this list need to require the gpio-ranges property?

The driver's airoha_convert_pin_to_reg_offset() relies on
pinctrl_find_gpio_range_from_pin_nolock(). If gpio-ranges is omitted in the
device tree, wouldn't this cause internal callbacks like
airoha_pinmux_set_direction() to fail unconditionally with -EINVAL, even
though the schema validation would pass?

[ ... ]

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

[ ... ]

> +          - if:
> +              properties:
> +                function:
> +                  const: pwm
> +            then:
> +              properties:
> +                groups:
> +                  enum: [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, gpio28, gpio29]

[Severity: Medium]
Should the groups constraints for pon, pwm, mdio, and phy*_led* functions
be defined using items: { enum: [...] } instead of directly using enum?

The groups property implicitly inherits string-array from pinmux-node.yaml.
Applying enum directly to an array requires the entire array instance to
equal one of the literal string values, which will always evaluate to false.

Using items: { enum: [...] } (as is done for uart and spi) would correctly
validate the elements of the array and allow valid device trees to multiplex
multiple pin groups in a single node.

[ ... ]

> +          drive-strength:
> +            description:
> +              Selects the drive strength for MIO pins, in mA.
> +            enum: [2, 4, 6, 8]

[Severity: Low]
Is the reference to "MIO pins" accurate for this SoC?

This terminology is typically associated with Xilinx ZynqMP bindings, while
the Airoha EN7523 uses standard GPIO pins.

[ ... ]

> +    additionalProperties: false
> +
> +additionalProperties: false
> +
> +examples:

[Severity: Medium]
Should this use unevaluatedProperties: false instead?

Since this schema imports standard properties via allOf: - $ref: pinctrl.yaml#,
using additionalProperties: false will improperly reject standard properties
like pinctrl-0 or pinctrl-names if they are used on this device node.

> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    pinctrl {
> +      compatible = "airoha,en7523-pinctrl";
> +
> +      interrupt-parent = <&gic>;
> +      interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> +
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +
> +      interrupt-controller;
> +      #interrupt-cells = <2>;
> +
> +      pcie1-rst-pins {
> +        conf {
> +          pins = "pcie_reset1";
> +          drive-open-drain = <1>;
> +        };
> +      };

[Severity: Low]
Is it possible to use the modern boolean syntax for drive-open-drain here?

The core pincfg-node.yaml schema marks the integer assignment form as
deprecated, preferring just the boolean property form (drive-open-drain;) to
avoid dt_binding_check validation warnings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727174141.664933-1-mikhail.kshevetskiy@iopsys.eu?part=33