Message-ID: <05780d0a-bc9c-3a81-2676-ea92453d7303@quicinc.com>
Date:   Mon, 28 Feb 2022 19:44:56 +0530
From:   "Satya Priya Kakitapalli (Temp)" <quic_c_skakit@...cinc.com>
To:     Stephen Boyd <swboyd@...omium.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>
CC:     Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Das Srinagesh <gurus@...eaurora.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_collinsd@...cinc.com>,
        <quic_subbaram@...cinc.com>, <quic_jprakash@...cinc.com>
Subject: Re: [PATCH V7 1/5] dt-bindings: mfd: pm8008: Add pm8008 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@...cinc.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.

Powered by blists - more mailing lists