| Message ID | 1645182064-15843-2-git-send-email-quic_c_skakit@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Qualcomm Technologies, Inc. PM8008 regulator driver | expand |
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? > 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? 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)? > @@ -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?
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(-)