Re: [PATCH v7 34/34] dt-bindings: pinctrl: airoha: add support of an7563 pin controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The binding lacks a parent syscon relationship or `reg` property, and omits the required `airoha,chip-scu` phandle property.
- [Low] Invalid and deprecated integer value assignment to the boolean property `drive-open-drain` in the device tree example.
- [Low] Incorrect JSON Schema array constraints for the `groups` property in most multiplexing conditional blocks.
--

--- Patch [34]: [PATCH v7 34/34] dt-bindings: pinctrl: airoha: add support of an7563 pin controller ---
commit fedf510207cace3632698b52e7e3162470e8e08a
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

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

This patch adds device tree binding schema for Airoha AN7563 SoC pin
controller.

> diff --git a/Documentation/devicetree/bindings/pinctrl/airoha,an7563-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/airoha,an7563-pinctrl.yaml
> new file mode 100644
> index 0000000000000..e626d34661285
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/airoha,an7563-pinctrl.yaml
> @@ -0,0 +1,348 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/airoha,an7563-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Airoha AN7563 Pin Controller
> +
> +maintainers:
> +  - Lorenzo Bianconi <lorenzo@kernel.org>
> +
> +description:
> +  The Airoha's AN7563 Pin controller is used to control SoC pins.
> +
> +properties:
> +  compatible:
> +    const: airoha,an7563-pinctrl

[Severity: Medium]
Does the schema need to document the parent-child syscon requirement and the
airoha,chip-scu property?

The driver relies on device_node_to_regmap(dev->parent->of_node), meaning
the pin controller node needs to be instantiated as a child of a syscon
node.

It also expects an airoha,chip-scu phandle property in the pin
controller node. Both the schema definition and the example omit these
requirements.

> [ ... ]
> +        allOf:
> +          - if:
> +              properties:
> +                function:
> +                  const: pon
> +            then:
> +              properties:
> +                groups:
> +                  enum: [pon]
> +          - if:
> +              properties:
> +                function:
> +                  const: sipo
> +            then:
> +              properties:
> +                groups:
> +                  enum: [sipo, sipo_rclk]

[Severity: Low]
Is this enum constraint structurally correct for the array?

Since groups is defined as a string-array via
/schemas/pinctrl/pinmux-node.yaml, applying the enum constraint directly to
the array instance means the entire array object must equal one of those
scalar strings, which will fail schema validation.

Should this use the items keyword to restrict the array elements, similar to
how it was done for the uart and spi functions? For example:
  items:
    enum: [sipo, sipo_rclk]

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

[Severity: Medium]
Will this example node fail to probe if developers use it as a template?

As mentioned above, the driver expects this node to be a child of a syscon
node, but the example shows it as a standalone top-level node.

If developers use this as a top-level node, device_node_to_regmap() will fail
and the driver will abort probing.

> +
> +      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 intentional to assign an integer value to drive-open-drain?

The core pincfg-node.yaml schema defines drive-open-drain as a boolean
property, which should be declared merely by its presence (i.e.,
drive-open-drain;).

Assigning it a value of <1> violates the schema and causes dt_binding_check
to throw a type error.

> +        };
> +      };

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