Re: [PATCH v3 1/4] dt-bindings: connector: Add fsl,aud-io-slot binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: connector: controller properties on passive nexus node
- [Low] dt-bindings: connector: reference to non-existent gpio-nexus-node.yaml
- [Low] dt-bindings: connector: 2-space indent and raw GPIO flag in example
--

commit e914717ab225094e8907a50041ee664ef2ac92f2
Author: Chancel Liu <chancel.liu@nxp.com>

dt-bindings: connector: Add fsl,aud-io-slot binding

This patch adds a Device Tree binding schema for the NXP AUD-IO slot
connector found on the i.MX95 19x19 EVK and i.MX952 EVK base boards. The
connector acts as a nexus node that remaps GPIOs, clocks, and interrupts
to base board providers for the IMX-AUD-IO expansion card.

> diff --git a/Documentation/devicetree/bindings/connector/fsl,aud-io-slot.yaml b/Documentation/devicetree/bindings/connector/fsl,aud-io-slot.yaml
> new file mode 100644
> index 0000000000000..fc3896d0fafab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/fsl,aud-io-slot.yaml
> @@ -0,0 +1,106 @@

[ ... ]

> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  gpio-map:
> +    minItems: 1
> +    maxItems: 3
> +
> +  gpio-map-mask:
> +    items:
> +      - const: 0xf
> +      - const: 0x0
> +
> +  gpio-map-pass-thru:
> +    items:
> +      - const: 0x0
> +      - const: 0x1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  clock-map:
> +    minItems: 1
> +    maxItems: 2
> +
> +  '#address-cells':
> +    const: 0
> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupt-map:
> +    minItems: 1
> +    maxItems: 2
> +
> +  interrupt-map-mask:
> +    items:
> +      - const: 0xf
> +      - const: 0x0
> +
> +allOf:
> +  - $ref: /schemas/gpio/gpio-nexus-node.yaml#
> +  - $ref: /schemas/clock/clock-nexus-node.yaml#

[Severity: Low]
Will this cause dt_binding_check to fail when resolving the allOf references?

There is no Documentation/devicetree/bindings/gpio/gpio-nexus-node.yaml in
the kernel tree or in dtschema (where gpio-map, gpio-map-mask, and
gpio-map-pass-thru are defined in schemas/gpio/gpio.yaml).

Also, Documentation/devicetree/bindings/clock/clock-nexus-node.yaml defines
select: true so it is applied globally by dtschema rather than being
referenced via allOf in individual bindings. Should the allOf block be
dropped here?

> +
> +required:
> +  - compatible
> +  - gpio-controller

[Severity: Medium]
Should a passive connector nexus node declare or require gpio-controller and
interrupt-controller?

The binding description notes that all actual hardware providers reside on
the base board and the connector node only defines index-based mappings via
gpio-map, clock-map, and interrupt-map. Per the Devicetree Specification, a
nexus node that remaps specifiers is not itself a controller.

In the kernel, gpio_shared_of_traverse() in drivers/gpio/gpiolib-shared.c
calls of_parse_phandle_with_args() (which does not traverse gpio-map) and
checks for gpio-controller to identify the backing hardware controller:

drivers/gpio/gpiolib-shared.c:gpio_shared_of_traverse() {
    ...
    ret = of_parse_phandle_with_args(curr, prop->name,
                                     "#gpio-cells", i,
                                     &args);
    if (ret)
        continue;

    np = args.np;

    if (!of_property_present(np, "gpio-controller"))
        continue;
    ...
}

When gpio-controller is present on the connector node, gpiolib-shared
records the passive connector node as the GPIO controller device instead of
the actual GPIO controller on the base board.

Similarly, in of_irq_parse_raw() in drivers/of/irq.c:

drivers/of/irq.c:of_irq_parse_raw() {
    ...
    bool intc = of_property_read_bool(ipar, "interrupt-controller");

    imap = of_get_property(ipar, "interrupt-map", &imaplen);
    if (intc &&
        (!imap || of_device_compatible_match(ipar, of_irq_imap_abusers))) {
        pr_debug(" -> got it !\n");
        return 0;
    }
    ...
}

If a node sets interrupt-controller without interrupt-map (which is optional
in this schema), interrupt resolution stops at the passive connector node
where no irq_domain exists.

Could gpio-controller and interrupt-controller be removed from properties,
required, and the example?

> +  - '#gpio-cells'
> +  - gpio-map
> +  - gpio-map-mask
> +  - gpio-map-pass-thru
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    connector {
> +      compatible = "fsl,imx95-19x19-evk-aud-io";
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +      gpio-map = <0 0 &gpio1 8 1>;
> +      gpio-map-mask = <0xf 0x0>;
> +      gpio-map-pass-thru = <0x0 0x1>;
> +      #clock-cells = <1>;
> +      clock-map = <0 &clk 1>;
> +      #address-cells = <0>;
> +      interrupt-controller;
> +      #interrupt-cells = <2>;
> +      interrupt-map-mask = <0xf 0x0>;
> +      interrupt-map = <0 0 &gpio2 27 IRQ_TYPE_LEVEL_LOW>;
> +    };

[Severity: Low]
This isn't a bug, but the properties inside the connector node in the
example use 2-space indentation instead of 4-space indentation, which
triggers a warning in scripts/dtc/dt-check-style --mode=strict. Also,
could gpio-map include dt-bindings/gpio/gpio.h and use a symbolic GPIO flag
macro instead of the raw integer 1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103070433.2307145-1-chancel.liu@oss.nxp.com?part=1