[PATCH v9 4/4] arm64: dts: qcom: sc7280: include sc7280-herobrine-audio-rt5682.dtsi in evoker

Sheng-Liang Pan posted 4 patches 3 years, 5 months ago
There is a newer version of this series
[PATCH v9 4/4] arm64: dts: qcom: sc7280: include sc7280-herobrine-audio-rt5682.dtsi in evoker
Posted by Sheng-Liang Pan 3 years, 5 months ago
Include sc7280-herobrine-audio-rt5682.dtsi in evoker
as it uses rt5682 codec.

Signed-off-by: Sheng-Liang Pan <sheng-liang.pan@quanta.corp-partner.google.com>
---

Changes in v9:
- new patch for evoker include rt5682.dtsi

 .../boot/dts/qcom/sc7280-herobrine-evoker.dts | 132 ++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts
index dcdd4eecfe670..d54c07ff35f4f 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts
@@ -8,8 +8,140 @@
 /dts-v1/;
 
 #include "sc7280-herobrine-evoker.dtsi"
+#include "sc7280-herobrine-audio-rt5682.dtsi"
+
 
 / {
 	model = "Google Evoker";
 	compatible = "google,evoker", "qcom,sc7280";
 };
+
+&sound {
+	model = "sc7280-rt5682-max98360a-3mic";
+
+	audio-routing =
+		"VA DMIC0", "vdd-micb",
+		"VA DMIC1", "vdd-micb",
+		"VA DMIC2", "vdd-micb",
+		"VA DMIC3", "vdd-micb",
+
+		"Headphone Jack", "HPOL",
+		"Headphone Jack", "HPOR";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+	dai-link@0 {
+		link-name = "MAX98360";
+		reg = <0>;
+
+		cpu {
+			sound-dai = <&lpass_cpu MI2S_SECONDARY>;
+		};
+
+		codec {
+			sound-dai = <&max98360a>;
+		};
+	};
+
+	dai-link@1 {
+		link-name = "DisplayPort";
+		reg = <1>;
+
+		cpu {
+			sound-dai = <&lpass_cpu LPASS_DP_RX>;
+		};
+
+		codec {
+			sound-dai = <&mdss_dp>;
+		};
+	};
+
+	dai-link@2 {
+		link-name = "ALC5682";
+		reg = <1>;
+
+		cpu {
+			sound-dai = <&lpass_cpu MI2S_PRIMARY>;
+		};
+
+		codec {
+			sound-dai = <&alc5682 0 /* aif1 */>;
+		};
+	};
+
+	dai-link@4 {
+		link-name = "DMIC";
+		reg = <4>;
+
+		cpu {
+			sound-dai = <&lpass_cpu LPASS_CDC_DMA_VA_TX0>;
+		};
+
+		codec {
+			sound-dai = <&lpass_va_macro 0>;
+		};
+	};
+};
+
+&lpass_cpu {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&mi2s0_data0>, <&mi2s0_data1>, <&mi2s0_mclk>, <&mi2s0_sclk>, <&mi2s0_ws>,
+			<&mi2s1_data0>, <&mi2s1_sclk>, <&mi2s1_ws>;
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	dai-link@0 {
+		reg = <MI2S_PRIMARY>;
+		qcom,playback-sd-lines = <1>;
+		qcom,capture-sd-lines = <0>;
+	};
+
+	dai-link@1 {
+		reg = <MI2S_SECONDARY>;
+		qcom,playback-sd-lines = <0>;
+	};
+
+	dai-link@5 {
+		reg = <LPASS_DP_RX>;
+	};
+
+	dai-link@25 {
+		reg = <LPASS_CDC_DMA_VA_TX0>;
+	};
+};
+
+&lpass_va_macro {
+	status = "okay";
+	vdd-micb-supply = <&pp1800_l2c>;
+};
+
+/* PINCTRL */
+
+&lpass_dmic01_clk {
+	drive-strength = <8>;
+	bias-disable;
+};
+
+&lpass_dmic01_clk_sleep {
+	drive-strength = <2>;
+};
+
+&lpass_dmic01_data {
+	bias-pull-down;
+};
+
+&lpass_dmic23_clk {
+	drive-strength = <8>;
+	bias-disable;
+};
+
+&lpass_dmic23_clk_sleep {
+	drive-strength = <2>;
+};
+
+&lpass_dmic23_data {
+	bias-pull-down;
+};
-- 
2.34.1
Re: [PATCH v9 4/4] arm64: dts: qcom: sc7280: include sc7280-herobrine-audio-rt5682.dtsi in evoker
Posted by Doug Anderson 3 years, 5 months ago
Hi,

On Mon, Oct 31, 2022 at 3:20 AM Sheng-Liang Pan
<sheng-liang.pan@quanta.corp-partner.google.com> wrote:
>
> Include sc7280-herobrine-audio-rt5682.dtsi in evoker
> as it uses rt5682 codec.
>
> Signed-off-by: Sheng-Liang Pan <sheng-liang.pan@quanta.corp-partner.google.com>
> ---
>
> Changes in v9:
> - new patch for evoker include rt5682.dtsi
>
>  .../boot/dts/qcom/sc7280-herobrine-evoker.dts | 132 ++++++++++++++++++
>  1 file changed, 132 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts
> index dcdd4eecfe670..d54c07ff35f4f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts

Why are you modifying the "evoker.dts" file and not the "evoker.dtsi"
file? Does the LTE SKU not have rt5682 audio?


> @@ -8,8 +8,140 @@
>  /dts-v1/;
>
>  #include "sc7280-herobrine-evoker.dtsi"
> +#include "sc7280-herobrine-audio-rt5682.dtsi"
> +
>
>  / {
>         model = "Google Evoker";
>         compatible = "google,evoker", "qcom,sc7280";
>  };
> +
> +&sound {

This is sorted incorrectly. "&sound" sorts under "&lpass".

...though looking closer at everything, I suspect that you're going to
need to reorganize things anyway. See below.


> +       model = "sc7280-rt5682-max98360a-3mic";
> +
> +       audio-routing =
> +               "VA DMIC0", "vdd-micb",
> +               "VA DMIC1", "vdd-micb",
> +               "VA DMIC2", "vdd-micb",
> +               "VA DMIC3", "vdd-micb",
> +
> +               "Headphone Jack", "HPOL",
> +               "Headphone Jack", "HPOR";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +       dai-link@0 {
> +               link-name = "MAX98360";
> +               reg = <0>;
> +
> +               cpu {
> +                       sound-dai = <&lpass_cpu MI2S_SECONDARY>;
> +               };
> +
> +               codec {
> +                       sound-dai = <&max98360a>;
> +               };
> +       };

The way you have things organized right now, technically the entire
"dai-link@0" here should be removed. Why? You're already getting it
from "sc7280-herobrine-audio-rt5682.dtsi". ...so I would say just to
remove it, but... (see below for more).


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

So dai-link@1 is confusing. You're fully overriding all of the
properties that you inherited by including
"sc7280-herobrine-audio-rt5682.dtsi". It happens to work because you
override _all_ of the properties, but it's a sign that something isn't
quite right. It feels like you are diverging _too much_ from
"sc7280-herobrine-audio-rt5682.dtsi"

My suggestion is that, instead of doing it this way, you:

1. Fully copy "sc7280-herobrine-audio-rt5682.dtsi" to a new file
called "sc7280-herobrine-audio-rt5682-3mic.dtsi".

2. Accept that there will be some duplication between the normal and
the 3mic version. I think there are enough differences that the
duplication is better than the spaghetti of overrides.

3. Try to make it so that diffs between the normal and "3mic" version
are as clean as possible so someone comparing the files can see the
exact differences.


> +       dai-link@2 {
> +               link-name = "ALC5682";
> +               reg = <1>;

Something is wrong with the above node. Your unit address (dai-link@2)
doesn't match your reg (reg = <1>;).