Re: [PATCH 2/3] dt-bindings: pinctrl: rockchip: Add io domain properties

[Robin Murphy]

On 2023-09-04 12:58, Sascha Hauer wrote:
> Add rockchip,io-domains property to the Rockchip pinctrl driver. This
> list of phandles points to the IO domain device(s) the pins of the
> pinctrl driver are supplied from.
>
> Also a rockchip,io-domain-boot-on property is added to pin groups
> which can be used for pin groups which themselves are needed to access
> the regulators an IO domain is driven from.
>
> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> ---
>   .../bindings/pinctrl/rockchip,pinctrl.yaml          | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> index 10c335efe619e..92075419d29cf 100644
> --- a/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/rockchip,pinctrl.yaml
> @@ -62,6 +62,11 @@ properties:
>         Required for at least rk3188 and rk3288. On the rk3368 this should
>         point to the PMUGRF syscon.
>   
> +  rockchip,io-domains:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Phandles to io domains
> +
>     "#address-cells":
>       enum: [1, 2]
>   
> @@ -137,7 +142,13 @@ additionalProperties:
>               - description:
>                   The phandle of a node contains the generic pinconfig options
>                   to use as described in pinctrl-bindings.txt.
> -
> +      rockchip,io-domain-boot-on:

I don't think "on" is a particularly descriptive or useful property name 
for something that has no "off" state. Furthermore it's no help at all 
if the DT consumer *is* the bootloader that's expected to configure this 
in the first place. IMO it would seem a lot more sensible to have an 
integer (or enum) property which describes the actual value for the 
initial I/O domain setting. Then Linux can choose to assume the presence 
of the property at all implies that the bootloader should have set it up 
already, but also has the option of actively enforcing it as well if we 
want to.

> +        type: boolean
> +        description:
> +          If true assume that the io domain needed for this pin group has been
> +          configured correctly by the bootloader. This is needed to break cyclic
> +          dependencies introduced when a io domain needs a regulator that can be
> +          accessed through pins configured here.

This is describing a Linux implementation detail, not the binding 
itself. There's no technical reason a DT consumer couldn't already 
figure this much out from the existing topology (by observing that the 
pinctrl consumer is a grandparent of the I/O domain's supply).

Thanks,
Robin.

>   examples:
>     - |
>       #include <dt-bindings/interrupt-controller/arm-gic.h>