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