Message-ID: <426ca0fd-5119-7e3c-89ce-27590b11f63f@linaro.org>
Date:   Tue, 14 Mar 2023 11:09:41 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v2] arm64: dts: qcom: sm6115: Move SDHC node(s)'s
 'pinctrl' properties to dts
Content-Language: en-US
To:     Bhupesh Sharma <bhupesh.sharma@linaro.org>,
        linux-arm-msm@vger.kernel.org
Cc:     agross@kernel.org, andersson@kernel.org,
        linux-kernel@vger.kernel.org, bhupesh.linux@gmail.com,
        robh+dt@kernel.org, devicetree@vger.kernel.org,
        Marijn Suijten <marijn.suijten@somainline.org>
References: <20230314074001.1873781-1-bhupesh.sharma@linaro.org>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <20230314074001.1873781-1-bhupesh.sharma@linaro.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 14.03.2023 08:40, Bhupesh Sharma wrote:
> Normally the 'pinctrl' properties of a SDHC controller and the
> chip detect pin settings are dependent on the type of the slots
> (for e.g uSD card slot), regulators and GPIO(s) available on the
> board(s).
> 
> So, move the same from the sm6115 dtsi file to the respective
> board file(s).
So, file or files? :D

> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
> Changes since v1:
> - v1 can be seen here: https://lore.kernel.org/linux-arm-msm/20221220113616.1556097-1-bhupesh.sharma@linaro.org/
> - Colleted the R-B from Marijn.
> - Rebased on linux-next/master
> 
>  .../boot/dts/qcom/sm4250-oneplus-billie2.dts  | 10 +++++++++
>  arch/arm64/boot/dts/qcom/sm6115.dtsi          | 22 -------------------
>  2 files changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts
> index a3f1c7c41fd73..329eb496bbc5f 100644
> --- a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts
> +++ b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts
> @@ -202,12 +202,22 @@ &sdhc_2 {
>  	vqmmc-supply = <&vreg_l5a>;
>  
>  	cd-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>;
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&sdc2_state_on &sdc2_card_det_n>;
> +	pinctrl-1 = <&sdc2_state_off &sdc2_card_det_n>;
This should have been

pinctrl-n
pinctrl-names

I made a mistake in my lenovo dts if that was your reference..

You should also mention that the implicit removal of sdhci1's
gpio properties from the lenovo j606f and oneplus billie2 is intentional
as they both use UFS instead of eMMC.

And one more thing, you missed bringing the CD pin back into pinctrl-0/1
in the tab dts. I'd really appreciate if you could fix up that ordering
mess I mentioned above while at it.

Konrad
>  
>  	status = "okay";
>  };
>  
>  &tlmm {
>  	gpio-reserved-ranges = <14 4>;
> +
> +	sdc2_card_det_n: sd-card-det-n-state {
> +		pins = "gpio88";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
>  };
>  
>  &ufs_mem_hc {
> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> index fbd67d2c8d781..e8e5f2cafebb9 100644
> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> @@ -595,13 +595,6 @@ data-pins {
>  					bias-pull-up;
>  					drive-strength = <10>;
>  				};
> -
> -				sd-cd-pins {
> -					pins = "gpio88";
> -					function = "gpio";
> -					bias-pull-up;
> -					drive-strength = <2>;
> -				};
>  			};
>  
>  			sdc2_state_off: sdc2-off-state {
> @@ -622,13 +615,6 @@ data-pins {
>  					bias-pull-up;
>  					drive-strength = <2>;
>  				};
> -
> -				sd-cd-pins {
> -					pins = "gpio88";
> -					function = "gpio";
> -					bias-disable;
> -					drive-strength = <2>;
> -				};
>  			};
>  		};
>  
> @@ -731,10 +717,6 @@ sdhc_1: mmc@4744000 {
>  				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
>  			clock-names = "iface", "core", "xo", "ice";
>  
> -			pinctrl-0 = <&sdc1_state_on>;
> -			pinctrl-1 = <&sdc1_state_off>;
> -			pinctrl-names = "default", "sleep";
> -
>  			bus-width = <8>;
>  			status = "disabled";
>  		};
> @@ -753,10 +735,6 @@ sdhc_2: mmc@4784000 {
>  				 <&rpmcc RPM_SMD_XO_CLK_SRC>;
>  			clock-names = "iface", "core", "xo";
>  
> -			pinctrl-0 = <&sdc2_state_on>;
> -			pinctrl-1 = <&sdc2_state_off>;
> -			pinctrl-names = "default", "sleep";
> -
>  			power-domains = <&rpmpd SM6115_VDDCX>;
>  			operating-points-v2 = <&sdhc2_opp_table>;
>  			iommus = <&apps_smmu 0x00a0 0x0>;