| Message ID | 20220902185148.635292-4-ahalaney@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | regulator: dt-bindings: qcom,rpmh: dt-binding fixups | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 16 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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 >
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
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
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(+)