Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 18 Nov 2022 15:28:38 +0000
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Rajendra Nayak <quic_rjendra@quicinc.com>
Cc:     agross@kernel.org, andersson@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, konrad.dybcio@linaro.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, dianders@chromium.org
Subject: Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add a new herobrine Pro SKU
Message-ID: <Y3ekpgpRxgFwEeo0@google.com>
References: <20221118073017.26128-1-quic_rjendra@quicinc.com>
 <20221118073017.26128-2-quic_rjendra@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20221118073017.26128-2-quic_rjendra@quicinc.com>
Precedence: bulk

Hi Rajendra,

On Fri, Nov 18, 2022 at 01:00:17PM +0530, Rajendra Nayak wrote:

> Subject: arm64: dts: qcom: sc7280: Add a new herobrine Pro SKU

nit: this adds the herobrine *CRD* Pro SKU (though other Pro SKUs
might follow), so 'CRD' should be part of the subject

uber-nit: 'new' is redundant in this context

> Some of the qualcomm qcard based herobrine devices can come with
> a Pro variant of the chipset with some qcard level changes like
> the smps9 from pm8350c which is ganged up with smps7 and smps8,
> so we just end up removing smps9 from the herobrine pro sku dtsi.

This is a very long sentence :)

> We then use it to create a new dts for the Pro variant of the
> herobrine CRD.

Using 'we' is a a bit colloquial for a commit message, how a about
something like this: 'Add a .dtsi for pro skus that deletes the
smps9 node and include it from the new dts for the CRD Pro'.

> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/Makefile             |  1 +
>  .../dts/qcom/sc7280-herobrine-crd-pro.dts     | 35 +++++++++++++++++++
>  .../dts/qcom/sc7280-herobrine-pro-sku.dtsi    |  8 +++++
>  3 files changed, 44 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7280-herobrine-crd-pro.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7280-herobrine-pro-sku.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index afe496a93f94..c5ac51c3a383 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -108,6 +108,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-wormdingler-rev1-boe-rt5682s.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-r1.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7180-trogdor-r1-lte.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-herobrine-crd.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-herobrine-crd-pro.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-herobrine-evoker.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-herobrine-evoker-lte.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-herobrine-herobrine-r1.dtb
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd-pro.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd-pro.dts
> new file mode 100644
> index 000000000000..fe6b228e9e4b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd-pro.dts
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * sc7280 CRD 3+ Pro board device tree source
> + *
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include "sc7280-herobrine-crd.dts"
> +#include "sc7280-herobrine-pro-sku.dtsi"
> +
> +/ {
> +	model = "Qualcomm Technologies, Inc. sc7280 CRD Pro platform (rev5+)";
> +	compatible = "google,hoglin-sku1536", "qcom,sc7280";
> +
> +	/* FIXED REGULATORS */
> +
> +	/*
> +	 * On most herobrine boards PPVAR_SYS directly provides VREG_EDP_BL.
> +	 * However, on CRD there's an extra regulator in the way. Since this
> +	 * is expected to be uncommon, we'll leave the "vreg_edp_bl" label
> +	 * in the baseboard herobrine.dtsi point at "ppvar_sys" and then
> +	 * make a "_crd" specific version here.
> +	 */
> +	vreg_edp_bl_crd: vreg-edp-bl-crd-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vreg_edp_bl_crd";
> +
> +		gpio = <&pm8350c_gpios 6 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&edp_bl_reg_en>;
> +
> +		vin-supply = <&ppvar_sys>;
> +	};
> +};

Why is this node needed here, doesn't it already exist by including
'sc7280-herobrine-crd.dts'?

> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-pro-sku.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine-pro-sku.dtsi
> new file mode 100644
> index 000000000000..fb4bbe8aeda0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-pro-sku.dtsi
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Google Herobrine dts fragment for PRO SKUs
> + *
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +/delete-node/ &vreg_s9c_0p676;
> -- 
> 2.17.1
>