Re: [PATCH 1/7] dt-bindings: regulator: Convert regulator bindings to YAML format

skakit
2021-03-03T21:52:25Z

Hi Rob,

Thanks for reviewing the patch!

>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id:
>> http://devicetree.org/schemas/regulator/qcom,rpmh-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies, Inc. RPMh Regulators
>> +
>> +maintainers:
>> + - David Collins <collinsd@codeaurora.org>
>> +
>> +description:
>
> I assume you want the formatting here maintained, so you need a '|' at
> the end.
>

Ok.

>> + rpmh-regulator devices support PMIC regulator management via the
>> Voltage
>> + Regulator Manager (VRM) and Oscillator Buffer (XOB) RPMh
>> accelerators. The APPS
>> + processor communicates with these hardware blocks via a Resource
>> State
>> + Coordinator (RSC) using command packets. The VRM allows changing
>> three
>> + parameters for a given regulator, enable state, output voltage,
>> and operating
>> + mode. The XOB allows changing only a single parameter for a
>> given regulator,
>> + its enable state. Despite its name, the XOB is capable of
>> controlling the
>> + enable state of any PMIC peripheral. It is used for clock
>> buffers, low-voltage
>> + switches, and LDO/SMPS regulators which have a fixed voltage and
>> mode.
>> +
>> + =======================
>> + Required Node Structure
>> + =======================
>> +
>> + RPMh regulators must be described in two levels of device nodes.
>> The first
>> + level describes the PMIC containing the regulators and must
>> reside within an
>> + RPMh device node. The second level describes each regulator
>> within the PMIC
>> + which is to be used on the board. Each of these regulators maps
>> to a single
>> + RPMh resource.
>> +
>> + The names used for regulator nodes must match those supported by
>> a given PMIC.
>> + Supported regulator node names are
>> + For PM8005, smps1 - smps4
>> + For PM8009, smps1 - smps2, ldo1 - ldo7
>> + For PM8150, smps1 - smps10, ldo1 - ldo18
>> + For PM8150L, smps1 - smps8, ldo1 - ldo11, bob, flash, rgb
>
> flash and rgb aren't documented.
>

Ok will add them.

>> + For PM8350, smps1 - smps12, ldo1 - ldo10
>> + For PM8350C, smps1 - smps10, ldo1 - ldo13, bob
>> + For PM8998, smps1 - smps13, ldo1 - ldo28, lvs1 - lvs2
>> + For PMI8998, bob
>> + For PM6150, smps1 - smps5, ldo1 - ldo19
>> + For PM6150L, smps1 - smps8, ldo1 - ldo11, bob
>> + For PMX55, smps1 - smps7, ldo1 - ldo16
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - qcom,pm8005-rpmh-regulators
>> + - qcom,pm8009-rpmh-regulators
>> + - qcom,pm8009-1-rpmh-regulators
>> + - qcom,pm8150-rpmh-regulators
>> + - qcom,pm8150l-rpmh-regulators
>> + - qcom,pm8350-rpmh-regulators
>> + - qcom,pm8350c-rpmh-regulators
>> + - qcom,pm8998-rpmh-regulators
>> + - qcom,pmi8998-rpmh-regulators
>> + - qcom,pm6150-rpmh-regulators
>> + - qcom,pm6150l-rpmh-regulators
>> + - qcom,pmx55-rpmh-regulators
>> +
>> + qcom,pmic-id:
>> + description: RPMh resource name suffix used for the
>> regulators found on
>> + this PMIC. Typical values are "a", "b", "c",
>> "d", "e", "f".
>
> Sounds like constraints. Make the values a schema.
>

Ok

>> + $ref: /schemas/types.yaml#/definitions/string
>> +
>> + qcom,always-wait-for-ack:
>> + description: Boolean flag which indicates that the
>> application processor
>> + must wait for an ACK or a NACK from RPMh for
>> every request
>> + sent for this regulator including those which
>> are for a
>> + strictly lower power state.
>> + $ref: /schemas/types.yaml#/definitions/string
>
> Boolean or string?
>

Ok, will change it to /schemas/types.yaml#/definitions/flag

>> +
>> +patternProperties:
>> + ".*-supply$":
>
> You can drop '.*'. That's already the case without '^'.
>

Ok.

> The supply names need to be defined.
>

you mean I should define like this "^vdd-s|l([0-9]+)-supply$": ?

>> + description: phandle of the parent supply regulator of one or
>> more of the
>> + regulators for this PMIC.
>> +
>> + "^((smps|ldo|lvs)[0-9]*)$":
>
> s/*/+/ as 1 digit is always required, right?
>

ok

>> + type: object
>> + allOf:
>
> Don't need allOf.
>

ok, will drop this.

>> + - $ref: "regulator.yaml#"
>> + description: List of regulator parent supply phandles
>
> This is a node, not a list of phandles.
>

Okay.

>> +
>> + "bob$":
>
> 'foobob' is okay as that would be allowed? If a fixed string, put
> under 'properties'.
>

It is fixed string, will move it to properties.

>> + type: object
>> + allOf:
>> + - $ref: "regulator.yaml#"
>> + description: BOB regulator parent supply phandle
>> +
>> +additionalProperties: false
>> +
>> +required:
>> + - compatible
>> + - qcom,pmic-id
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>> +
>> + pm8998-rpmh-regulators {
>> + compatible = "qcom,pm8998-rpmh-regulators";
>> + qcom,pmic-id = "a";
>> +
>> + vdd-l7-l12-l14-l15-supply = <&pm8998_s5>;
>> +
>> + smps2 {
>> + regulator-min-microvolt = <1100000>;
>> + regulator-max-microvolt = <1100000>;
>> + };
>> +
>> + pm8998_s5: smps5 {
>
> Drop unused labels.
>

Okay.

>> + regulator-min-microvolt = <1904000>;
>> + regulator-max-microvolt = <2040000>;
>> + };
>> +
>> + ldo7 {
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>> + regulator-allowed-modes =
>> + <RPMH_REGULATOR_MODE_LPM
>> + RPMH_REGULATOR_MODE_HPM>;
>> + regulator-allow-set-load;
>> + };
>> +
>> + lvs1 {
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + };
>> + };
>> +
>> + pmi8998-rpmh-regulators {
>> + compatible = "qcom,pmi8998-rpmh-regulators";
>> + qcom,pmic-id = "b";
>> +
>> + bob {
>> + regulator-min-microvolt = <3312000>;
>> + regulator-max-microvolt = <3600000>;
>> + regulator-allowed-modes =
>> + <RPMH_REGULATOR_MODE_AUTO
>> + RPMH_REGULATOR_MODE_HPM>;
>> + regulator-initial-mode = <RPMH_REGULATOR_MODE_AUTO>;
>> + };
>> + };
>> +...
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>

Thanks,
Satya Priya