Message-ID: <CAE-0n51X=LJMjDb9KS0rqQDqLR5srzxCOJCRS4oJgPSXbvaSiQ@mail.gmail.com>
Date:   Fri, 18 Feb 2022 17:39:06 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Satya Priya <quic_c_skakit@...cinc.com>
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

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?

> 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?

Powered by blists - more mailing lists