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: <1591868882-16553-1-git-send-email-rbokka@codeaurora.org> <1591868882-16553-4-git-send-email-rbokka@codeaurora.org>
In-Reply-To: <1591868882-16553-4-git-send-email-rbokka@codeaurora.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Sat, 13 Jun 2020 13:23:09 -0700
Message-ID: <CAD=FV=XBbm_kGBdkhf5Hx5pDFEsYeX4o-SL=a5rPt-R_8FjNqg@mail.gmail.com>
Subject: Re: [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse
To:     Ravi Kumar Bokka <rbokka@codeaurora.org>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
        dhavalp@codeaurora.org, mturney@codeaurora.org,
        sparate@codeaurora.org, c_rbokka@codeaurora.org,
        mkurumel@codeaurora.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>
> This patch adds device tree node for qfprom-efuse controller.
>
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts |  4 ++++
>  arch/arm64/boot/dts/qcom/sc7180.dtsi    | 10 ++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index 4e9149d..2a9224e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -287,6 +287,10 @@
>         };
>  };
>
> +&qfprom {
> +       vcc-supply = <&vreg_l11a_1p8>;
> +};
> +
>  &qspi {
>         status = "okay";
>         pinctrl-names = "default";
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index 31b9217..20f3480 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -498,9 +498,15 @@
>                         #power-domain-cells = <1>;
>                 };
>
> -               qfprom@784000 {
> +               qfprom: qfprom@780000 {

The dt schema checker claims that your node should actually be called:

eeprom|efuse|nvram

So you probably want the above to actually be:

qfprom: efuse@780000

The label to the side doesn't matter and so it can stay qfprom--just
the node name itself is what the checker cares about.


>                         compatible = "qcom,qfprom";

As per my response in the bindings, this should be:

  "qcom,sc7180-qfprom", "qcom,qfprom"

...even if the driver only ever makes use of "qcom,qfprom" this
future-proofs us a bit.


> -                       reg = <0 0x00784000 0 0x8ff>;
> +                       reg = <0 0x00780000 0 0x7a0>,
> +                             <0 0x00782000 0 0x100>,
> +                             <0 0x00784000 0 0x8ff>;
> +                       reg-names = "raw", "conf", "corrected";

As per my response in the bindings, reg-names is discouraged so you
should remove and make it so that the driver doesn't need.


> +

It's hard to tell in email, but checkpatch yells above the above line:

    ERROR: trailing whitespace
    #53: FILE: arch/arm64/boot/dts/qcom/sc7180.dtsi:512:
    +^I^I^I$


> +                       clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> +                       clock-names = "secclk";

As per the binding spec, clock name shouldn't end in "clk".

For your edification, I provided a patch to fix all my review feedback at:

https://crrev.com/c/2244933

Feel free to squash it into your next version.


-Doug