Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1596445047-2975-1-git-send-email-hsin-hsiung.wang@mediatek.com> <1596445047-2975-3-git-send-email-hsin-hsiung.wang@mediatek.com>
In-Reply-To: <1596445047-2975-3-git-send-email-hsin-hsiung.wang@mediatek.com>
From:   Nicolas Boichat <drinkcat@chromium.org>
Date:   Tue, 4 Aug 2020 10:12:49 +0800
Message-ID: <CANMq1KDDbPBsnxPHvPTcTreW7OrTwC_=8GyM=rrU2QOLPKp2Bg@mail.gmail.com>
Subject: Re: [PATCH 2/3] regulator: bindings: Add document for MT6315 regulator
To:     Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
Cc:     Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Stephen Boyd <sboyd@kernel.org>,
        lkml <linux-kernel@vger.kernel.org>,
        Devicetree List <devicetree@vger.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>,
        linux-arm-msm@vger.kernel.org,
        srv_heupstream <srv_heupstream@mediatek.com>,
        Fei Shao <fshao@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Hsin-Hsiung,

On Mon, Aug 3, 2020 at 4:57 PM Hsin-Hsiung Wang
<hsin-hsiung.wang@mediatek.com> wrote:
>
> Add device tree binding information for mt6315 regulator driver.
> Example bindings for mt6315 are added.
>
> Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
> ---
>  .../bindings/regulator/mt6315-regulator.txt        | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/mt6315-regulator.txt
>
> diff --git a/Documentation/devicetree/bindings/regulator/mt6315-regulator.txt b/Documentation/devicetree/bindings/regulator/mt6315-regulator.txt
> new file mode 100644
> index 0000000..1c14537
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mt6315-regulator.txt
> @@ -0,0 +1,45 @@
> +Mediatek MT6315 Regulator
> +
> +Required properties:
> +- compatible: Must be one of the following.
> +       "mediatek,mt6315_3-regulator"
> +       "mediatek,mt6315_6-regulator"
> +       "mediatek,mt6315_7-regulator"

As highlighted on Gerrit [1], I think this is wrong. The device tree
compatible should focus on actual hardware differences, _not_ the way
the device is used.

So I looked at the datasheet, and there are 5 variants of the MT6315.
They all have the same number of VBUCKs, just with different _default_
voltages, Imax, and sequencing. Since the regulator range is the same,
I don't think you need to care about any of this, so I'd have a single
compatible "mediatek,mt6315-regulator".

The one thing that is special here, though, is that you want to
combine regulators for BUCK1, right?

That is, for MT6315PP (id 6), you want to combine BUCK1/2/4 to power
the big cores (hence 0xb = "1011" in patch 3/3), and for MT6315SP (id
7), you want to combine BUCK1/2 (hence 0x3 = 0011).

So, instead of a table here, what I'd do is to figure out a way to
indicate, in the device tree, that bucks 1, 2, 4 need to be combined.

I think the correct way to handle this is to add a
`regulator-coupled-with` property. That is you'd have a device tree
that looks like this:

mt6315_6: mt6315@6 {
    compatible = "mediatek,mt6315-regulator";
    reg = <0x6 0 0xb 1>;
    mt6315_6_vbuck1: vbuck1 {
        regulator-compatible = "vbuck1";
        ...
        regulator-coupled-with = <&mt6315_6_vbuck2, mt6315_6_vbuck4>;
    };

    mt6315_6_vbuck2: vbuck2 {
        regulator-compatible = "vbuck2";
        ...
    };

    mt6315_6_vbuck3: vbuck3 {
        regulator-compatible = "vbuck3";
        ...
    };

    mt6315_6_vbuck4: vbuck4 {
        regulator-compatible = "vbuck4";
        ...
    };
};

Then, at probe time, you can figure out which regulators are coupled
with another one, and only provide controls for the first regulator in
the list (with the proper mask).

Another, simpler way, may look like this:

mt6315_6: mt6315@6 {
    compatible = "mediatek,mt6315-regulator";
    reg = <0x6 0 0xb 1>;
    mt6315_6_vbuck1: vbuck1 {
        regulator-compatible = "vbuck1";
        regulator-mask = <0xb>;
    };
    mt6315_6_vbuck3: vbuck3 {
        regulator-compatible = "vbuck3";
        regulator-mask = <0x8>;
    };
};

But then it's a bit weird, because 0x8 = 1 << 3, which we can already
infer from "vbuck3" compatible...

[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2229019/13/drivers/regulator/mt6315-regulator.c#283


> +- reg: SPMI slave id.
> +- regulators: List of regulators provided by this controller.
> +  The definition for each of these nodes is defined using the standard binding
> +  for regulators at Documentation/devicetree/bindings/regulator/regulator.txt.
> +
> +The valid names for regulators are:
> +BUCK:
> +  vbuck1, vbuck3, vbuck4
> +
> +Example:
> +       mt6315_3: mt6315@3 {
> +               compatible = "mediatek,mt6315_3-regulator";
> +               reg = <0x3 0 0xb 1>;
> +
> +               mt6315_3_vbuck1: vbuck1 {
> +                       regulator-compatible = "vbuck1";
> +                       regulator-min-microvolt = <300000>;
> +                       regulator-max-microvolt = <1193750>;
> +                       regulator-enable-ramp-delay = <256>;
> +                       regulator-allowed-modes = <0 1 2 4>;
> +               };
> +
> +               mt6315_3_vbuck3: vbuck3 {
> +                       regulator-compatible = "vbuck3";
> +                       regulator-min-microvolt = <300000>;
> +                       regulator-max-microvolt = <1193750>;
> +                       regulator-enable-ramp-delay = <256>;
> +                       regulator-allowed-modes = <0 1 2 4>;
> +               };
> +
> +               mt6315_3_vbuck3: vbuck3 {
> +                       regulator-compatible = "vbuck3";
> +                       regulator-min-microvolt = <300000>;
> +                       regulator-max-microvolt = <1193750>;
> +                       regulator-enable-ramp-delay = <256>;
> +                       regulator-allowed-modes = <0 1 2 4>;
> +               };
> +       };
> --
> 2.6.4