diff mbox series

[v2,4/6] arm64: dts: qcom: msm8976: Declare and use SDC1 pins

Message ID 20240121-msm8976-dt-v2-4-7b186a02dc72@somainline.org
State New
Headers
Series arm64: dts: qcom: msm8956-loire: SDCard and USB support |

Commit Message

Marijn Suijten Jan. 21, 2024, 10:33 p.m. UTC 
  Add the pinctrl states for SDC1 and use them on sdhc_1.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 arch/arm64/boot/dts/qcom/msm8976.dtsi | 55 +++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

Comments

Konrad Dybcio Feb. 6, 2024, 6:58 p.m. UTC | #1 
On 21.01.2024 23:33, Marijn Suijten wrote:
> Add the pinctrl states for SDC1 and use them on sdhc_1.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---

[...]


> @@ -840,6 +890,11 @@ sdhc_1: mmc@7824900 {
>  				 <&gcc GCC_SDCC1_APPS_CLK>,
>  				 <&rpmcc RPM_SMD_XO_CLK_SRC>;
>  			clock-names = "iface", "core", "xo";
> +
> +			pinctrl-names = "default", "sleep";
> +			pinctrl-0 = <&sdc1_on_state>;
> +			pinctrl-1 = <&sdc1_off_state>;

-names should go last

Konrad
Marijn Suijten Feb. 6, 2024, 10:36 p.m. UTC | #2 
On 2024-02-06 19:58:50, Konrad Dybcio wrote:
> On 21.01.2024 23:33, Marijn Suijten wrote:
> > Add the pinctrl states for SDC1 and use them on sdhc_1.
> > 
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> 
> [...]
> 
> 
> > @@ -840,6 +890,11 @@ sdhc_1: mmc@7824900 {
> >  				 <&gcc GCC_SDCC1_APPS_CLK>,
> >  				 <&rpmcc RPM_SMD_XO_CLK_SRC>;
> >  			clock-names = "iface", "core", "xo";
> > +
> > +			pinctrl-names = "default", "sleep";
> > +			pinctrl-0 = <&sdc1_on_state>;
> > +			pinctrl-1 = <&sdc1_off_state>;
> 
> -names should go last

Per the cover letter:

    - Moved pinctrl-names before pinctrl-N (Konrad);

You explicitly requested this in https://lore.kernel.org/linux-arm-msm/60a40ace-d4e9-df74-88f9-4354d80efaac@linaro.org/#t
but we also backtracked on it somewhat for consistency with other -names, and the general state of this file.

Maybe we should apply v1 of this specific patch?

- Marijn
AngeloGioacchino Del Regno Feb. 7, 2024, 12:15 p.m. UTC | #3 
Il 06/02/24 23:36, Marijn Suijten ha scritto:
> On 2024-02-06 19:58:50, Konrad Dybcio wrote:
>> On 21.01.2024 23:33, Marijn Suijten wrote:
>>> Add the pinctrl states for SDC1 and use them on sdhc_1.
>>>
>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>> ---
>>
>> [...]
>>
>>
>>> @@ -840,6 +890,11 @@ sdhc_1: mmc@7824900 {
>>>   				 <&gcc GCC_SDCC1_APPS_CLK>,
>>>   				 <&rpmcc RPM_SMD_XO_CLK_SRC>;
>>>   			clock-names = "iface", "core", "xo";
>>> +
>>> +			pinctrl-names = "default", "sleep";
>>> +			pinctrl-0 = <&sdc1_on_state>;
>>> +			pinctrl-1 = <&sdc1_off_state>;
>>
>> -names should go last
> 

Konrad is technically correct as per [1] but, for consistency, since all of the
other nodes in msm8976.dtsi are following that order for pinctrl, this version of
the patch is correct.

That, unless anyone wants to reorder everything in this file as per [1], which
may or may not be worth the noise.

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/dts-coding-style.rst

Regards,
Angelo

> Per the cover letter:
> 
>      - Moved pinctrl-names before pinctrl-N (Konrad);
> 
> You explicitly requested this in https://lore.kernel.org/linux-arm-msm/60a40ace-d4e9-df74-88f9-4354d80efaac@linaro.org/#t
> but we also backtracked on it somewhat for consistency with other -names, and the general state of this file.
> 
> Maybe we should apply v1 of this specific patch?
> 
> - Marijn
Konrad Dybcio Feb. 9, 2024, 4:59 p.m. UTC | #4 
On 6.02.2024 23:36, Marijn Suijten wrote:
> On 2024-02-06 19:58:50, Konrad Dybcio wrote:
>> On 21.01.2024 23:33, Marijn Suijten wrote:
>>> Add the pinctrl states for SDC1 and use them on sdhc_1.
>>>
>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>> ---
>>
>> [...]
>>
>>
>>> @@ -840,6 +890,11 @@ sdhc_1: mmc@7824900 {
>>>  				 <&gcc GCC_SDCC1_APPS_CLK>,
>>>  				 <&rpmcc RPM_SMD_XO_CLK_SRC>;
>>>  			clock-names = "iface", "core", "xo";
>>> +
>>> +			pinctrl-names = "default", "sleep";
>>> +			pinctrl-0 = <&sdc1_on_state>;
>>> +			pinctrl-1 = <&sdc1_off_state>;
>>
>> -names should go last
> 
> Per the cover letter:
> 
>     - Moved pinctrl-names before pinctrl-N (Konrad);
> 
> You explicitly requested this in https://lore.kernel.org/linux-arm-msm/60a40ace-d4e9-df74-88f9-4354d80efaac@linaro.org/#t
> but we also backtracked on it somewhat for consistency with other -names, and the general state of this file.

Agreed, but we made up our minds since 2022-12-15 ;)

Konrad
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/msm8976.dtsi b/arch/arm64/boot/dts/qcom/msm8976.dtsi
index d2bb1ada361a..1d06f9b8a0f1 100644
--- a/arch/arm64/boot/dts/qcom/msm8976.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8976.dtsi
@@ -669,6 +669,56 @@  tlmm: pinctrl@1000000 {
 			interrupt-controller;
 			#interrupt-cells = <2>;
 
+			sdc1_off_state: sdc1-off-state {
+				clk-pins {
+					pins = "sdc1_clk";
+					drive-strength = <2>;
+					bias-disable;
+				};
+
+				cmd-pins {
+					pins = "sdc1_cmd";
+					drive-strength = <2>;
+					bias-pull-up;
+				};
+
+				data-pins {
+					pins = "sdc1_data";
+					drive-strength = <2>;
+					bias-pull-up;
+				};
+
+				rclk-pins {
+					pins = "sdc1_rclk";
+					bias-pull-down;
+				};
+			};
+
+			sdc1_on_state: sdc1-on-state {
+				clk-pins {
+					pins = "sdc1_clk";
+					drive-strength = <16>;
+					bias-disable;
+				};
+
+				cmd-pins {
+					pins = "sdc1_cmd";
+					drive-strength = <10>;
+					bias-pull-up;
+				};
+
+				data-pins {
+					pins = "sdc1_data";
+					drive-strength = <10>;
+					bias-pull-up;
+				};
+
+				rclk-pins {
+					pins = "sdc1_rclk";
+					bias-pull-down;
+				};
+			};
+
 			spi1_default: spi0-default-state {
 				spi-pins {
 					pins = "gpio0", "gpio1", "gpio3";
@@ -840,6 +890,11 @@  sdhc_1: mmc@7824900 {
 				 <&gcc GCC_SDCC1_APPS_CLK>,
 				 <&rpmcc RPM_SMD_XO_CLK_SRC>;
 			clock-names = "iface", "core", "xo";
+
+			pinctrl-names = "default", "sleep";
+			pinctrl-0 = <&sdc1_on_state>;
+			pinctrl-1 = <&sdc1_off_state>;
+
 			status = "disabled";
 		};