| Message ID | 1645182064-15843-2-git-send-email-quic_c_skakit@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V7,1/5] dt-bindings: mfd: pm8008: Add pm8008 regulators | expand |
On 2/19/2022 7:09 AM, Stephen Boyd wrote: > Quoting Satya Priya (2022-02-18 03:00:59) >> Add regulators and their supply nodes. Add separate compatible >> "qcom,pm8008-regulators" to differentiate between pm8008 infra >> and pm8008 regulators mfd devices. >> >> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> >> --- > Is the register layout compatible with SPMI regulators? The gpio node > seems to be fully compatible and the same driver probes there for SPMI > and i2c, so I wonder why we can't extend the existing SPMI gpio and > regulator bindings to have the new compatible strings for pm8008. Is > anything really different, or do we have the same device talking i2c > instead of SPMI now? Possibly it's exposing the different hardware > blocks inside the PMIC at different i2c addresses. It looks like the i2c > address is 0x8 and then there's 16-bits of address space inside the i2c > device to do things. 0x9 is the i2c address for the regulators and then > each ldo is at some offset in there? The register layout is not compatible with spmi regulators, I see some differences w.r.t VOLTAGE_CTL, EN_CTL, MODE_CTL registers. Also, there is no headroom related stuff in the spmi driver. >> Changes in V2: >> - As per Rob's comments changed "pm8008[a-z]?-regulator" to >> "^pm8008[a-z]?-regulators". >> >> Changes in V3: >> - Fixed bot errors. >> - As per stephen's comments, changed "^pm8008[a-z]?-regulators$" to >> "regulators". >> >> Changes in V4: >> - Changed compatible string to "qcom,pm8008-regulators" >> >> Changes in V5: >> - Remove compatible for regulators node. >> - Move supply nodes of the regulators to chip level. >> >> Changes in V6: >> - No changes. >> >> Changes in V7: >> - Removed the intermediate regulators node and added ldos >> directly under mfd node. >> >> .../devicetree/bindings/mfd/qcom,pm8008.yaml | 50 +++++++++++++++++++--- >> 1 file changed, 43 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml >> index ec3138c..6b3b53e 100644 >> --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml >> +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml >> @@ -16,7 +16,9 @@ description: | >> >> properties: >> compatible: >> - const: qcom,pm8008 >> + enum: >> + - qcom,pm8008 >> + - qcom,pm8008-regulators >> >> reg: >> description: >> @@ -44,6 +46,21 @@ properties: >> "#size-cells": >> const: 0 >> >> + vdd_l1_l2-supply: >> + description: Input supply phandle of ldo1 and ldo2 regulators. >> + >> + vdd_l3_l4-supply: >> + description: Input supply phandle of ldo3 and ldo4 regulators. >> + >> + vdd_l5-supply: >> + description: Input supply phandle of ldo5 regulator. >> + >> + vdd_l6-supply: >> + description: Input supply phandle of ldo6 regulator. >> + >> + vdd_l7-supply: >> + description: Input supply phandle of ldo7 regulator. >> + >> patternProperties: >> "^gpio@[0-9a-f]+$": >> type: object >> @@ -85,13 +102,16 @@ patternProperties: >> >> additionalProperties: false >> >> + "^ldo[1-7]$": >> + type: object >> + $ref: "../regulator/regulator.yaml#" >> + description: PM8008 regulator peripherals of PM8008 regulator device >> + >> required: >> - compatible >> - reg >> - - interrupts >> - "#address-cells" >> - "#size-cells" >> - - "#interrupt-cells" >> >> additionalProperties: false >> >> @@ -102,13 +122,11 @@ examples: >> qupv3_se13_i2c { >> #address-cells = <1>; >> #size-cells = <0>; >> - pm8008i@8 { >> + pm8008_infra: pm8008@8 { >> compatible = "qcom,pm8008"; >> reg = <0x8>; >> #address-cells = <1>; >> #size-cells = <0>; >> - interrupt-controller; >> - #interrupt-cells = <2>; >> >> interrupt-parent = <&tlmm>; >> interrupts = <32 IRQ_TYPE_EDGE_RISING>; > I still fail to see what this part of the diff has to do with > regulators. Can it be split off to a different patch with a clear > description of why interrupt-controller and #interrupt-cells is no > longer required for qcom,pm8008? This diff has nothing to do with regulators, I removed it to avoid yaml errors during dtbs check. I'll move this to a separate patch. > It really looks like we're combining the binding for qcom,pm8008 and > qcom,pm8008-regulators at the same level, which looks wrong. We don't > want to describe the least common denominator between the two bindings. > Why not make two different bindings and files? One for the interrupty > gpio/interrupt controller device (at 0x8) and one for the regulator one > (at 0x9)? Okay, I'll add a different binding for regulators (mfd/qcom,pm8008-regulators.yaml), leave this binding as it is.. and also add separate DT files for pm8008-infra and pm8008-regulators. >> @@ -123,6 +141,24 @@ examples: >> #interrupt-cells = <2>; >> }; >> }; >> - }; >> >> + pm8008_regulators: pm8008@9 { > pmic@9, or regulators@9? The node name should be generic. > >> + compatible = "qcom,pm8008-regulators"; >> + reg = <0x9>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + vdd_l1_l2-supply = <&vreg_s8b_1p2>; >> + vdd_l3_l4-supply = <&vreg_s1b_1p8>; >> + vdd_l5-supply = <&vreg_bob>; >> + vdd_l6-supply = <&vreg_bob>; >> + vdd_l7-supply = <&vreg_bob>; >> + >> + pm8008_l1: ldo1 { >> + regulator-name = "pm8008_l1"; >> + regulator-min-microvolt = <950000>; >> + regulator-max-microvolt = <1300000>; >> + }; >> + }; > For some i2c devices that appear on multiple i2c addresses we make an > i2c client for each address in the driver that attaches to the node we > put in DT. I suppose that won't work easily here. Either way, it would > make it much clearer if this existing binding was left alone. Is there > other functionality inside the i2c address 0x9 register space that isn't > regulators? As mentioned above, I'll make a separate binding for regulators. There is no other functionality apart from regulators in the i2c 0x9 register space.
Quoting Satya Priya Kakitapalli (Temp) (2022-02-28 06:14:56) > > On 2/19/2022 7:09 AM, Stephen Boyd wrote: > > Quoting Satya Priya (2022-02-18 03:00:59) > >> Add regulators and their supply nodes. Add separate compatible > >> "qcom,pm8008-regulators" to differentiate between pm8008 infra > >> and pm8008 regulators mfd devices. > >> > >> Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> > >> --- > > Is the register layout compatible with SPMI regulators? The gpio node > > seems to be fully compatible and the same driver probes there for SPMI > > and i2c, so I wonder why we can't extend the existing SPMI gpio and > > regulator bindings to have the new compatible strings for pm8008. Is > > anything really different, or do we have the same device talking i2c > > instead of SPMI now? Possibly it's exposing the different hardware > > blocks inside the PMIC at different i2c addresses. It looks like the i2c > > address is 0x8 and then there's 16-bits of address space inside the i2c > > device to do things. 0x9 is the i2c address for the regulators and then > > each ldo is at some offset in there? > > > The register layout is not compatible with spmi regulators, I see some > differences w.r.t VOLTAGE_CTL, EN_CTL, MODE_CTL registers. Also, there > is no headroom related stuff in the spmi driver. It sounds like minor differences and/or improvements to the existing SPMI regulator hardware. > >> interrupt-parent = <&tlmm>; > >> interrupts = <32 IRQ_TYPE_EDGE_RISING>; > > I still fail to see what this part of the diff has to do with > > regulators. Can it be split off to a different patch with a clear > > description of why interrupt-controller and #interrupt-cells is no > > longer required for qcom,pm8008? > > > This diff has nothing to do with regulators, I removed it to avoid yaml > errors during dtbs check. > > I'll move this to a separate patch. Ok, thanks!
diff --git a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml index ec3138c..6b3b53e 100644 --- a/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml +++ b/Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml @@ -16,7 +16,9 @@ description: | properties: compatible: - const: qcom,pm8008 + enum: + - qcom,pm8008 + - qcom,pm8008-regulators reg: description: @@ -44,6 +46,21 @@ properties: "#size-cells": const: 0 + vdd_l1_l2-supply: + description: Input supply phandle of ldo1 and ldo2 regulators. + + vdd_l3_l4-supply: + description: Input supply phandle of ldo3 and ldo4 regulators. + + vdd_l5-supply: + description: Input supply phandle of ldo5 regulator. + + vdd_l6-supply: + description: Input supply phandle of ldo6 regulator. + + vdd_l7-supply: + description: Input supply phandle of ldo7 regulator. + patternProperties: "^gpio@[0-9a-f]+$": type: object @@ -85,13 +102,16 @@ patternProperties: additionalProperties: false + "^ldo[1-7]$": + type: object + $ref: "../regulator/regulator.yaml#" + description: PM8008 regulator peripherals of PM8008 regulator device + required: - compatible - reg - - interrupts - "#address-cells" - "#size-cells" - - "#interrupt-cells" additionalProperties: false @@ -102,13 +122,11 @@ examples: qupv3_se13_i2c { #address-cells = <1>; #size-cells = <0>; - pm8008i@8 { + pm8008_infra: pm8008@8 { compatible = "qcom,pm8008"; reg = <0x8>; #address-cells = <1>; #size-cells = <0>; - interrupt-controller; - #interrupt-cells = <2>; interrupt-parent = <&tlmm>; interrupts = <32 IRQ_TYPE_EDGE_RISING>; @@ -123,6 +141,24 @@ examples: #interrupt-cells = <2>; }; }; - }; + pm8008_regulators: pm8008@9 { + compatible = "qcom,pm8008-regulators"; + reg = <0x9>; + #address-cells = <1>; + #size-cells = <0>; + + vdd_l1_l2-supply = <&vreg_s8b_1p2>; + vdd_l3_l4-supply = <&vreg_s1b_1p8>; + vdd_l5-supply = <&vreg_bob>; + vdd_l6-supply = <&vreg_bob>; + vdd_l7-supply = <&vreg_bob>; + + pm8008_l1: ldo1 { + regulator-name = "pm8008_l1"; + regulator-min-microvolt = <950000>; + regulator-max-microvolt = <1300000>; + }; + }; + }; ...
Add regulators and their supply nodes. Add separate compatible "qcom,pm8008-regulators" to differentiate between pm8008 infra and pm8008 regulators mfd devices. Signed-off-by: Satya Priya <quic_c_skakit@quicinc.com> --- Changes in V2: - As per Rob's comments changed "pm8008[a-z]?-regulator" to "^pm8008[a-z]?-regulators". Changes in V3: - Fixed bot errors. - As per stephen's comments, changed "^pm8008[a-z]?-regulators$" to "regulators". Changes in V4: - Changed compatible string to "qcom,pm8008-regulators" Changes in V5: - Remove compatible for regulators node. - Move supply nodes of the regulators to chip level. Changes in V6: - No changes. Changes in V7: - Removed the intermediate regulators node and added ldos directly under mfd node. .../devicetree/bindings/mfd/qcom,pm8008.yaml | 50 +++++++++++++++++++--- 1 file changed, 43 insertions(+), 7 deletions(-)