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

[Robin Murphy]

On 2023-09-06 08:21, Sascha Hauer wrote:
> On Tue, Sep 05, 2023 at 10:03:20AM +0100, Robin Murphy wrote:
> > 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.
>
> In fact it has an "off" state. A IO Domain can be disabled in the SoC
> registers

Oh, is that a thing on newer SoCs? At least in the RK3399 TRM the only 
I/O-domain-related control I can find is the 1.8V/3.0V logic level 
threshold in GRF_IO_VSEL (plus the one outlier in PMUGRF_SOC_CON0).

> and also the corresponding regulator can be disabled.

...which is clearly a property of the regulator, not of its consumers ;)

However it's also not a meaningful state in this context anyway, since 
if the supply was actually off, and thus we were unable to communicate 
with the PMIC to turn it on... oh dear.

Cheers,
Robin.

> > 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.
>
> I agree though that a particular setting instead of a boolean is better
> and could help the bootloader.
>
> > 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.
>
> Ok.
>
> Thanks,
>   Sascha