Re: [PATCH v11 06/37] dt-bindings: pinctrl: airoha: en7581: allow configuration of pcie_reset pins as gpio or pwm

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `gpio-ranges` example specifies an incorrect linear pin mapping due to a physical hardware gap, and the schema's `maxItems: 1` constraint prevents specifying the correct discontinuous mapping.
- [High] The dt-bindings introduce group names and functions for the EN7581 SoC that conflict with the corresponding pinctrl driver's definitions, creating an API contract violation.
--

--- Patch [6]: [PATCH v11 06/37] dt-bindings: pinctrl: airoha: en7581: allow configuration of pcie_reset pins as gpio or pwm ---
commit e5dd844dc6505525d8f5010d6aec0311493b74f7
Author: Mikhail Kshevetskiy <[email protected]>

dt-bindings: pinctrl: airoha: en7581: allow configuration of pcie_reset pins as gpio or pwm

This commit updates the en7581 pinctrl binding to allow configuring the
pcie_reset pins as gpio or pwm. It introduces new pin groups and functions,
and updates the device tree example to include a gpio-ranges property.

> diff --git a/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
> index 0c5a7eb6d563e..0cf1826c691fc 100644
> --- a/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/airoha,en7581-pinctrl.yaml

[ ... ]

> @@ -187,6 +187,14 @@ patternProperties:
>                properties:
>                  groups:
>                    enum: [pnand]
> +          - if:
> +              properties:
> +                function:
> +                  const: gpio
> +            then:
> +              properties:
> +                groups:
> +                  enum: [gpio47, gpio48, gpio49]
[Severity: High]
Will this configuration cause a runtime failure during device probe?

The pinctrl driver for EN7581 (drivers/pinctrl/airoha/pinctrl-airoha.c)
does not appear to register a "gpio" function, nor does it define the
"gpio47", "gpio48", or "gpio49" groups (which are defined for the AN7583
variant). 

If a device tree uses this documented API, will the pinctrl core
fail to find the requested function or groups?

[ ... ]

> @@ -347,6 +356,8 @@ examples:
>        interrupt-controller;
>        #interrupt-cells = <2>;
>  
> +      gpio-ranges = <&pinctrl 0 13 50>;
> +
[Severity: High]
Does this linear mapping misalign the pins due to a physical hardware gap?

The block "0 13 50" maps GPIO 47 to physical pin 60, but pin 60 does not
exist on the EN7581 SoC (pcie_reset0 is at pin 61). This forces GPIO 48 to
manipulate pcie_reset0 instead of pcie_reset1, leaving pcie_reset2
inaccessible.

Additionally, because the schema limits gpio-ranges to a single tuple
(maxItems: 1 defined earlier in the schema), developers cannot specify the
two tuples required to skip the pin 60 gap without violating the schema. 

Should the schema be updated to allow a discontinuous mapping and provide
an example that correctly maps the pins?

>        pcie1-rst-pins {
>          conf {
>            pins = "pcie_reset1";

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