[PATCH 0/3] regulator: dt-bindings: qcom,rpmh: dt-binding fixups

[PATCH 0/3] regulator: dt-bindings: qcom,rpmh: dt-binding fixups

[Andrew Halaney]

Hi,

This is my poor attempt at getting devicetree validation into a better
state for qcom,rpmh-regulator.yaml. This is a follow-up to Johan's
request for this over here:

    https://lore.kernel.org/linux-arm-msm/Yw8EE%2FESDUnIRf8P@hovoldconsulting.com/

In particular, I'm not certain patch 1 is the correct way to handle
things, and patch 2 makes validation too wide for the *-supply nodes.

I'd love any feedback here as I'm really not experienced in any of the
spaces (regulator, rpmh, or dt schema) so nit picking is welcomed.

Thanks in advance,
Andrew

Andrew Halaney (3):
  regulator: dt-bindings: qcom,rpmh: Use additionalProperties
  regulator: dt-bindings: qcom,rpmh: Specify supply property
  regulator: dt-bindings: qcom,rpmh: Indicate regulator-allow-set-load
    dependencies

 .../bindings/regulator/qcom,rpmh-regulator.yaml       | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.37.2

[PATCH 1/3] regulator: dt-bindings: qcom,rpmh: Use additionalProperties

[Andrew Halaney]

Right now, running make dt_binding_check results in this snippet:

    /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: 'additionalProperties' is a required property
            hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties"
            from schema $id: http://devicetree.org/meta-schemas/base.yaml#
      SCHEMA  Documentation/devicetree/bindings/processed-schema.json
    <snip..>
    /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: ignoring, error in schema:

Which results in the schema not being properly evaluated. Swap out
unevaluatedProperties which doesn't seem to be doing anything for
additionalProperties.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 .../devicetree/bindings/regulator/qcom,rpmh-regulator.yaml    | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
index 9a36bee750af..b3fd60b21610 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
@@ -106,6 +106,8 @@ patternProperties:
     $ref: "regulator.yaml#"
     description: smps/ldo regulator nodes(s).
 
+additionalProperties: false
+
 required:
   - compatible
   - qcom,pmic-id
@@ -351,8 +353,6 @@ allOf:
         "^vdd-l2[01]-supply$": true
         "^vdd-s[1-8]-supply$": true
 
-unevaluatedProperties: false
-
 examples:
   - |
     #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
-- 
2.37.2

[PATCH 2/3] regulator: dt-bindings: qcom,rpmh: Specify supply property

[Andrew Halaney]

The top level RPMh nodes have a supply property, make sure to specify it
so the patternProperties later that are keyed off of the PMIC version
are properly honored. Without this, and the dt-binding containing
additionalProperties: false, you will see the following when running
make dt_binding_check:

      DTEX    Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dts
      DTC     Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dtb
      CHECK   Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dtb
    /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dtb: pm8998-rpmh-regulators: 'vdd-l7-l12-l14-l15-supply' does not match any of the regexes: '^(smps|ldo|lvs)[0-9]+$', 'pinctrl-[0-9]+'
            From schema: /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml

That supply pattern is intended to be considered correct for the
qcom,pm8998-rpmh-regulators compatible, and is no longer complained
about with the supply property described.

Unfortunately this pattern is wide enough that it no longer complains
when you bork the expected supply for a compatible. I.e. for
qcom,pm8998-rpmh-regulators, if I change the example usage in the
binding to:

        vdd-l0-l12-l14-l15-supply = <&pm8998_s5>;

I get no warning, when really it should be of the pattern:

        vdd-l7-l12-l14-l15-supply = <&pm8998_s5>;

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 .../devicetree/bindings/regulator/qcom,rpmh-regulator.yaml     | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
index b3fd60b21610..86265b513de3 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
@@ -106,6 +106,9 @@ patternProperties:
     $ref: "regulator.yaml#"
     description: smps/ldo regulator nodes(s).
 
+  ".*-supply$":
+    description: Input supply phandle(s) for this node
+
 additionalProperties: false
 
 required:
-- 
2.37.2

[PATCH 3/3] regulator: dt-bindings: qcom,rpmh: Indicate regulator-allow-set-load dependencies

[Andrew Halaney]

For RPMH regulators it doesn't make sense to indicate
regulator-allow-set-load without saying what modes you can switch to,
so be sure to indicate a dependency on regulator-allowed-modes.

With this in place devicetree validation can catch issues like this:

    /mnt/extrassd/git/linux-next/arch/arm64/boot/dts/qcom/sm8350-hdk.dtb: pm8350-rpmh-regulators: ldo5: 'regulator-allowed-modes' is a dependency of 'regulator-allow-set-load'
            From schema: /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml

Suggested-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 .../devicetree/bindings/regulator/qcom,rpmh-regulator.yaml    | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
index 86265b513de3..1cfd9cfd9ba6 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
@@ -99,12 +99,16 @@ properties:
     type: object
     $ref: "regulator.yaml#"
     description: BOB regulator node.
+    dependencies:
+      regulator-allow-set-load: ["regulator-allowed-modes"]
 
 patternProperties:
   "^(smps|ldo|lvs)[0-9]+$":
     type: object
     $ref: "regulator.yaml#"
     description: smps/ldo regulator nodes(s).
+    dependencies:
+      regulator-allow-set-load: ["regulator-allowed-modes"]
 
   ".*-supply$":
     description: Input supply phandle(s) for this node
-- 
2.37.2

Re: [PATCH 1/3] regulator: dt-bindings: qcom,rpmh: Use additionalProperties

[Rob Herring]

On Fri, 02 Sep 2022 13:51:46 -0500, Andrew Halaney wrote:
> Right now, running make dt_binding_check results in this snippet:
> 
>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: 'additionalProperties' is a required property
>             hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties"
>             from schema $id: http://devicetree.org/meta-schemas/base.yaml#
>       SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>     <snip..>
>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: ignoring, error in schema:
> 
> Which results in the schema not being properly evaluated. Swap out
> unevaluatedProperties which doesn't seem to be doing anything for
> additionalProperties.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
>  .../devicetree/bindings/regulator/qcom,rpmh-regulator.yaml    | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dtb: pm8998-rpmh-regulators: 'vdd-l7-l12-l14-l15-supply' does not match any of the regexes: '^(smps|ldo|lvs)[0-9]+$', 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/3] regulator: dt-bindings: qcom,rpmh: Use additionalProperties

[Krzysztof Kozlowski]

On 02/09/2022 20:51, Andrew Halaney wrote:
> Right now, running make dt_binding_check results in this snippet:
> 
>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: 'additionalProperties' is a required property
>             hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties"
>             from schema $id: http://devicetree.org/meta-schemas/base.yaml#
>       SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>     <snip..>
>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: ignoring, error in schema:
> 
> Which results in the schema not being properly evaluated. Swap out
> unevaluatedProperties which doesn't seem to be doing anything for
> additionalProperties.

unevaluatedProperties were required due to usage of defs-allOf
(ba5d99609a5e ("regulator: dt-bindings: qcom,rpmh: document supplies per
variant")
).

Are you sure that it works correctly with additionalProperties?

Judging by errors it doesn't....

> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
>  .../devicetree/bindings/regulator/qcom,rpmh-regulator.yaml    | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
> index 9a36bee750af..b3fd60b21610 100644
> --- a/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
> @@ -106,6 +106,8 @@ patternProperties:
>      $ref: "regulator.yaml#"
>      description: smps/ldo regulator nodes(s).
>  
> +additionalProperties: false
> +

Don't move the location. The proper one is above examples.

Best regards,
Krzysztof

Re: [PATCH 2/3] regulator: dt-bindings: qcom,rpmh: Specify supply property

[Krzysztof Kozlowski]

On 02/09/2022 20:51, Andrew Halaney wrote:
> The top level RPMh nodes have a supply property, make sure to specify it
> so the patternProperties later that are keyed off of the PMIC version
> are properly honored. Without this, and the dt-binding containing
> additionalProperties: false, you will see the following when running
> make dt_binding_check:
> 
>       DTEX    Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dts
>       DTC     Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dtb
>       CHECK   Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dtb
>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.example.dtb: pm8998-rpmh-regulators: 'vdd-l7-l12-l14-l15-supply' does not match any of the regexes: '^(smps|ldo|lvs)[0-9]+$', 'pinctrl-[0-9]+'
>             From schema: /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
> 
> That supply pattern is intended to be considered correct for the
> qcom,pm8998-rpmh-regulators compatible, and is no longer complained
> about with the supply property described.

Which supply pattern?

> 
> Unfortunately this pattern is wide enough that it no longer complains
> when you bork the expected supply for a compatible. I.e. for
> qcom,pm8998-rpmh-regulators, if I change the example usage in the
> binding to:
> 
>         vdd-l0-l12-l14-l15-supply = <&pm8998_s5>;
> 
> I get no warning, when really it should be of the pattern:
> 
>         vdd-l7-l12-l14-l15-supply = <&pm8998_s5>;
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>

No, you basically reverse the change I did, without actually addressing
my intentions in that commit. If you want to revert it, please make a
proper revert and explain why such revert is needed.

Best regards,
Krzysztof

Re: [PATCH 3/3] regulator: dt-bindings: qcom,rpmh: Indicate regulator-allow-set-load dependencies

[Krzysztof Kozlowski]

On 02/09/2022 20:51, Andrew Halaney wrote:
> For RPMH regulators it doesn't make sense to indicate
> regulator-allow-set-load without saying what modes you can switch to,
> so be sure to indicate a dependency on regulator-allowed-modes.

Hmmmm.... What about other regulators?

> 
> With this in place devicetree validation can catch issues like this:
> 
>     /mnt/extrassd/git/linux-next/arch/arm64/boot/dts/qcom/sm8350-hdk.dtb: pm8350-rpmh-regulators: ldo5: 'regulator-allowed-modes' is a dependency of 'regulator-allow-set-load'
>             From schema: /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
> 
> Suggested-by: Johan Hovold <johan@kernel.org>
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>


Best regards,
Krzysztof

Re: [PATCH 1/3] regulator: dt-bindings: qcom,rpmh: Use additionalProperties

[Krzysztof Kozlowski]

On 05/09/2022 18:45, Krzysztof Kozlowski wrote:
> On 02/09/2022 20:51, Andrew Halaney wrote:
>> Right now, running make dt_binding_check results in this snippet:
>>
>>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: 'additionalProperties' is a required property
>>             hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties"
>>             from schema $id: http://devicetree.org/meta-schemas/base.yaml#
>>       SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>>     <snip..>
>>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: ignoring, error in schema:
>>
>> Which results in the schema not being properly evaluated. Swap out
>> unevaluatedProperties which doesn't seem to be doing anything for
>> additionalProperties.
> 
> unevaluatedProperties were required due to usage of defs-allOf
> (ba5d99609a5e ("regulator: dt-bindings: qcom,rpmh: document supplies per
> variant")
> ).
> 
> Are you sure that it works correctly with additionalProperties?
> 
> Judging by errors it doesn't....

What's more - I cannot reproduce that error (latest released dtschema)...

Best regards,
Krzysztof

Re: [PATCH 1/3] regulator: dt-bindings: qcom,rpmh: Use additionalProperties

[Andrew Halaney]

On Mon, Sep 05, 2022 at 06:53:23PM +0200, Krzysztof Kozlowski wrote:
> On 05/09/2022 18:45, Krzysztof Kozlowski wrote:
> > On 02/09/2022 20:51, Andrew Halaney wrote:
> >> Right now, running make dt_binding_check results in this snippet:
> >>
> >>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: 'additionalProperties' is a required property
> >>             hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties"
> >>             from schema $id: http://devicetree.org/meta-schemas/base.yaml#
> >>       SCHEMA  Documentation/devicetree/bindings/processed-schema.json
> >>     <snip..>
> >>     /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml: ignoring, error in schema:
> >>
> >> Which results in the schema not being properly evaluated. Swap out
> >> unevaluatedProperties which doesn't seem to be doing anything for
> >> additionalProperties.
> > 
> > unevaluatedProperties were required due to usage of defs-allOf
> > (ba5d99609a5e ("regulator: dt-bindings: qcom,rpmh: document supplies per
> > variant")
> > ).
> > 
> > Are you sure that it works correctly with additionalProperties?
> > 
> > Judging by errors it doesn't....
> 
> What's more - I cannot reproduce that error (latest released dtschema)...
> 

Ugh, I thought maybe I had ran into something here that was only in
linux-next, but no. I've had my environment borked the whole time I was
working on this series. So sorry about that.

I'll send a v2 once I rework things with my environment working
properly. Your comments here make sense to me -- unevaluatedProperties
makes sense here based on what I see in the example binding... so this
patch and the next will get dropped entirely.

Thanks,
Andrew

Re: [PATCH 3/3] regulator: dt-bindings: qcom,rpmh: Indicate regulator-allow-set-load dependencies

[Andrew Halaney]

On Mon, Sep 05, 2022 at 06:50:02PM +0200, Krzysztof Kozlowski wrote:
> On 02/09/2022 20:51, Andrew Halaney wrote:
> > For RPMH regulators it doesn't make sense to indicate
> > regulator-allow-set-load without saying what modes you can switch to,
> > so be sure to indicate a dependency on regulator-allowed-modes.
> 
> Hmmmm.... What about other regulators?
> 

My understanding (which very well might be wrong) is that if your
regulator is allowed to change modes, and sets regulator-allow-set-load,
then you have to describe what modes you can switch to.

But if you don't allow setting modes (for example qcom_rpm-regulator.c)
and just allow yourself to set_load() directly, then you don't need it.

So there is a more general requirement that applies regulator wide, but
I'm not sure how you would apply that at a higher level. I don't see a
good way to figure out in dt-binding land what regulator ops each
binding supports.

Hope that makes sense,
Andrew

> > 
> > With this in place devicetree validation can catch issues like this:
> > 
> >     /mnt/extrassd/git/linux-next/arch/arm64/boot/dts/qcom/sm8350-hdk.dtb: pm8350-rpmh-regulators: ldo5: 'regulator-allowed-modes' is a dependency of 'regulator-allow-set-load'
> >             From schema: /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml
> > 
> > Suggested-by: Johan Hovold <johan@kernel.org>
> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 0/3] regulator: dt-bindings: qcom,rpmh: dt-binding fixups

[Andrew Halaney]

On Fri, Sep 02, 2022 at 01:51:45PM -0500, Andrew Halaney wrote:
> Hi,
> 
> This is my poor attempt at getting devicetree validation into a better
> state for qcom,rpmh-regulator.yaml. This is a follow-up to Johan's
> request for this over here:
> 
>     https://lore.kernel.org/linux-arm-msm/Yw8EE%2FESDUnIRf8P@hovoldconsulting.com/
> 
> In particular, I'm not certain patch 1 is the correct way to handle
> things, and patch 2 makes validation too wide for the *-supply nodes.
> 
> I'd love any feedback here as I'm really not experienced in any of the
> spaces (regulator, rpmh, or dt schema) so nit picking is welcomed.

v2 posted over here: https://lore.kernel.org/linux-arm-msm/20220906201959.69920-1-ahalaney@redhat.com/

Re: [PATCH 3/3] regulator: dt-bindings: qcom,rpmh: Indicate regulator-allow-set-load dependencies

[Krzysztof Kozlowski]

On 06/09/2022 16:41, Andrew Halaney wrote:
> On Mon, Sep 05, 2022 at 06:50:02PM +0200, Krzysztof Kozlowski wrote:
>> On 02/09/2022 20:51, Andrew Halaney wrote:
>>> For RPMH regulators it doesn't make sense to indicate
>>> regulator-allow-set-load without saying what modes you can switch to,
>>> so be sure to indicate a dependency on regulator-allowed-modes.
>>
>> Hmmmm.... What about other regulators?
>>
> 
> My understanding (which very well might be wrong) is that if your
> regulator is allowed to change modes, and sets regulator-allow-set-load,
> then you have to describe what modes you can switch to.
>> But if you don't allow setting modes (for example qcom_rpm-regulator.c)
> and just allow yourself to set_load() directly, then you don't need it.
> 
> So there is a more general requirement that applies regulator wide, but
> I'm not sure how you would apply that at a higher level. I don't see a
> good way to figure out in dt-binding land what regulator ops each
> binding supports.

The bindings don't express it, but the regulator core explicitly asks
for set_mode with set_load callbacks in drms_uA_update(), which depends
on REGULATOR_CHANGE_DRMS (toggled with regulator-allow-set-load).

drms_uA_update() later calls regulator_mode_constrain() which checks if
mode changing is allowed (REGULATOR_CHANGE_MODE).

Therefore based on current implementation and meaning of
set-load/allowed-modes properties, I would say that this applies to all
regulators. I don't think that RPMh is special here.

Best regards,
Krzysztof