linux-kernel - Re: [PATCH 3/6] arm64: dts: qcom: sm8250: add lpass lpi pin controller node

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]

Message-ID: <X8aWDj1n6KTM5ma/@builder.lan>
Date:   Tue, 1 Dec 2020 13:14:22 -0600
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     linux-arm-msm@...r.kernel.org, agross@...nel.org,
        robh+dt@...nel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] arm64: dts: qcom: sm8250: add lpass lpi pin
 controller node

On Tue 01 Dec 09:37 CST 2020, Srinivas Kandagatla wrote:

> Add LPASS LPI pinctrl node required for Audio functionality on RB5.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 95 ++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index ec5b53b8f656..4e1309b6571e 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -2607,6 +2607,101 @@
>  			clock-names = "core", "audio", "bus";
>  		};
>  
> +		lpass_tlmm: pinctrl@...0000{
> +			compatible = "qcom,sm8250-lpass-lpi-pinctrl";
> +			reg = <0 0x33c0000 0x0 0x20000>,
> +				 <0 0x3550000 0x0 0x10000>;
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +			gpio-ranges = <&lpass_tlmm 0 0 14>;
> +
> +			clocks = <&q6afecc LPASS_HW_MACRO_VOTE LPASS_CLK_ATTRIBUTE_COUPLE_NO>,
> +				<&q6afecc LPASS_HW_DCODEC_VOTE LPASS_CLK_ATTRIBUTE_COUPLE_NO>;
> +			clock-names = "core", "audio";
> +
> +			wsa_swr_clk_pin {

I prefer if you drop this outer "container" node.

> +				wsa_swr_clk_sleep: wsa_swr_clk_sleep {

Per the binding I think the name of this node should match '-pins$' and
the node name may not contain '_' characters.

> +					mux {

The pinctrl state is a collection of all pins, function and pinconf
properties of the phandle pointed to and its immediate child nodes. As
such you can flatten this inner "mux" level.


On the other hand, I'm assuming that you're always going to mux the
clock and data pin together, so instead of designing this around the
pins you could design it around the states and do:

			wsa_swr_active: wsa-swr-active-pins {
				clk {
					pins = "gpio10";
					function = "wsa_swr_clk";
					drive-strength = <2>;
					slew-rate = <1>;
					bias-disable;
				};

				data {
					pins = "gpio11";
					function = "wsa_swr_data";
					drive-strength = <2>;
					slew-rate = <1>;
					bias-bus-hold;

				};
			};

This way the state describes the whole thing and you don't end up with a
bunch of states for each part of the function.

Regards,
Bjorn

> +						pins = "gpio10";
> +						function = "wsa_swr_clk";
> +						drive-strength = <2>;
> +						input-enable;
> +						bias-pull-down;
> +					};
> +				};
> +
> +				wsa_swr_clk_active: wsa_swr_clk_active {
> +					mux {
> +						pins = "gpio10";
> +						function = "wsa_swr_clk";
> +						drive-strength = <2>;
> +						slew-rate = <1>;
> +						bias-disable;
> +					};
> +				};
> +			};
> +
> +			wsa_swr_data_pin {
> +				wsa_swr_data_sleep: wsa_swr_data_sleep {
> +					mux {
> +						pins = "gpio11";
> +						function = "wsa_swr_data";
> +						drive-strength = <2>;
> +						input-enable;
> +						bias-pull-down;
> +					};
> +				};
> +
> +				wsa_swr_data_active: wsa_swr_data_active {
> +					mux {
> +						pins = "gpio11";
> +						function = "wsa_swr_data";
> +						drive-strength = <2>;
> +						slew-rate = <1>;
> +						bias-bus-hold;
> +					};
> +				};
> +			};
> +
> +	                cdc_dmic01_data_active: dmic01_data_active {
> +	                        mux {
> +	                                pins = "gpio7";
> +					function = "dmic1_data";
> +	                                drive-strength = <8>;
> +	                                input-enable;
> +	                        };
> +	                };
> +
> +	                cdc_dmic01_data_sleep: dmic01_data_sleep {
> +	                        mux {
> +	                                pins = "gpio7";
> +					function = "dmic1_data";
> +	                                drive-strength = <2>;
> +	                                pull-down;
> +	                                input-enable;
> +	                        };
> +	                };
> +
> +	                cdc_dmic01_clk_active: dmic01_clk_active {
> +	                        mux {
> +	                                pins = "gpio6";
> +					function = "dmic1_clk";
> +	                                drive-strength = <8>;
> +	                                output-high;
> +	                        };
> +	                };
> +
> +	                cdc_dmic01_clk_sleep: dmic01_clk_sleep {
> +	                        mux {
> +	                                pins = "gpio6";
> +					function = "dmic1_clk";
> +	                                drive-strength = <2>;
> +	                                bias-disable;
> +	                                output-low;
> +	                        };
> +	                };
> +		};
> +
>  		adsp: remoteproc@...00000 {
>  			compatible = "qcom,sm8250-adsp-pas";
>  			reg = <0 0x17300000 0 0x100>;
> -- 
> 2.21.0
>