linux-kernel - Re: [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 13 Jun 2020 13:23:09 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Ravi Kumar Bokka <rbokka@...eaurora.org>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
        dhavalp@...eaurora.org, mturney@...eaurora.org,
        sparate@...eaurora.org, c_rbokka@...eaurora.org,
        mkurumel@...eaurora.org
Subject: Re: [RFC v2 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse

Hi,

On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@...eaurora.org> wrote:
>
> This patch adds device tree node for qfprom-efuse controller.
>
> Signed-off-by: Ravi Kumar Bokka <rbokka@...eaurora.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@...000 {
> +               qfprom: qfprom@...000 {

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@...000

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