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:   Wed, 6 Apr 2022 13:45:19 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Cc:     agross@kernel.org, bjorn.andersson@linaro.org, robh+dt@kernel.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, quic_rohitkr@quicinc.com,
        srinivas.kandagatla@linaro.org, dianders@chromium.org,
        swboyd@chromium.org, judyhsiao@chromium.org,
        Venkata Prasad Potturu <quic_potturu@quicinc.com>
Subject: Re: [PATCH v6 3/3] arm64: dts: qcom: sc7280: Add dt nodes for sound
 card
Message-ID: <Yk3736Av338XoLH/@google.com>
References: <1649157220-29304-1-git-send-email-quic_srivasam@quicinc.com>
 <1649157220-29304-4-git-send-email-quic_srivasam@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <1649157220-29304-4-git-send-email-quic_srivasam@quicinc.com>
Precedence: bulk

On Tue, Apr 05, 2022 at 04:43:40PM +0530, Srinivasa Rao Mandadapu wrote:
> Add dt nodes for sound card support, which is using WCD938x headset
> playback, capture, I2S speaker playback and DMICs via VA macro.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280-crd.dts  |  8 +++
>  arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 93 ++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> index 224a82d..b1b968a 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> @@ -90,6 +90,14 @@ ap_ts_pen_1v8: &i2c13 {
>  	us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
>  };
>  
> +&sound {
> +	audio-routing =
> +		"VA DMIC0", "MIC BIAS1",
> +		"VA DMIC1", "MIC BIAS1",
> +		"VA DMIC2", "MIC BIAS3",
> +		"VA DMIC3", "MIC BIAS3";
> +};
> +
>  &tlmm {
>  	tp_int_odl: tp-int-odl {
>  		pins = "gpio7";
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index e3d8cbf..45e1d82 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -84,6 +84,99 @@
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&nvme_pwren>;
>  	};
> +
> +	sound: sound {
> +		compatible = "google,sc7280-herobrine";
> +		model = "sc7280-wcd938x-max98360a-1mic";
> +
> +		audio-routing =
> +				"IN1_HPHL", "HPHL_OUT",
> +				"IN2_HPHR", "HPHR_OUT",
> +				"AMIC1", "MIC BIAS1",
> +				"AMIC2", "MIC BIAS2",
> +				"VA DMIC0", "MIC BIAS3",
> +				"VA DMIC1", "MIC BIAS3",
> +				"VA DMIC2", "MIC BIAS1",
> +				"VA DMIC3", "MIC BIAS1",
> +				"TX SWR_ADC0", "ADC1_OUTPUT",
> +				"TX SWR_ADC1", "ADC2_OUTPUT",
> +				"TX SWR_ADC2", "ADC3_OUTPUT",
> +				"TX SWR_DMIC0", "DMIC1_OUTPUT",
> +				"TX SWR_DMIC1", "DMIC2_OUTPUT",
> +				"TX SWR_DMIC2", "DMIC3_OUTPUT",
> +				"TX SWR_DMIC3", "DMIC4_OUTPUT",
> +				"TX SWR_DMIC4", "DMIC5_OUTPUT",
> +				"TX SWR_DMIC5", "DMIC6_OUTPUT",
> +				"TX SWR_DMIC6", "DMIC7_OUTPUT",
> +				"TX SWR_DMIC7", "DMIC8_OUTPUT";
> +
> +		qcom,msm-mbhc-hphl-swh = <1>;
> +		qcom,msm-mbhc-gnd-swh = <1>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		#sound-dai-cells = <0>;
> +
> +		dai-link@1 {
> +			link-name = "Secondary MI2S Playback";

The other link names provide information about the other end
of the link (DP, WCD, DMIC), while this one describes the SoC side.
Shouldn't this be "MAX98360A"? Not sure about the 'Playback' part,
it seems 'link-name' is used as stream name, judging from a few
samples of peeking into '/proc/asound/pcm' on different devices
it seems that 'Playback' or 'Capture' is ususally not part of the
stream name.

> +			reg = <MI2S_SECONDARY>;
> +			cpu {
> +				sound-dai = <&lpass_cpu MI2S_SECONDARY>;
> +			};
> +
> +			codec {
> +				sound-dai = <&max98360a>;
> +			};
> +		};
> +
> +		dai-link@5 {
> +			link-name = "DP Playback";

See comment above about 'Playback'. Just 'DP' is maybe a bit short, how
about 'DisplayPort'?

> +			reg = <LPASS_DP_RX>;
> +			cpu {
> +				sound-dai = <&lpass_cpu LPASS_DP_RX>;
> +			};
> +
> +			codec {
> +				sound-dai = <&mdss_dp>;
> +			};
> +		};
> +
> +		dai-link@6 {
> +			link-name = "WCD Playback";

Most instances I found spell out the codec name. It seems here we need
the 'Playback'/'Capture' info (or something else) to not end up with
duplicate link names. So my suggestion here would be "WCD9385 Playback".

> +			reg = <LPASS_CDC_DMA_RX0>;
> +			cpu {
> +				sound-dai = <&lpass_cpu LPASS_CDC_DMA_RX0>;
> +			};
> +
> +			codec {
> +				sound-dai = <&wcd938x 0>, <&swr0 0>, <&rxmacro 0>;
> +			};
> +		};
> +
> +		dai-link@19 {
> +			link-name = "WCD Capture";

"WCD9385 Capture"?

> +			reg = <LPASS_CDC_DMA_TX3>;
> +			cpu {
> +				sound-dai = <&lpass_cpu LPASS_CDC_DMA_TX3>;
> +			};
> +
> +			codec {
> +				sound-dai = <&wcd938x 1>, <&swr1 0>, <&txmacro 0>;
> +			};
> +		};
> +
> +		dai-link@25 {
> +			link-name = "DMIC Capture";


just "DMIC"?