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%[email protected]/
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
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 <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
---
.../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
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 <[email protected]>
---
.../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
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 <[email protected]>
> Signed-off-by: Andrew Halaney <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
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
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 <[email protected]>
> > Signed-off-by: Andrew Halaney <[email protected]>
>
>
> Best regards,
> Krzysztof
>
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%[email protected]/
>
> 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/[email protected]/
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