Add wcd938x and max98360a codecs for audio use case on
sc7280 based platforms.
Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
Add amp_en node for max98360a codec.
Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
Co-developed-by: Venkata Prasad Potturu <[email protected]>
Signed-off-by: Venkata Prasad Potturu <[email protected]>
---
arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts | 6 ++
arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 8 +++
arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 97 ++++++++++++++++++++++++++
3 files changed, 111 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
index 344338a..aa0bf6e2 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
@@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
pins = "gpio51";
};
+&wcd938x {
+ pinctrl-names = "default";
+ pinctrl-0 = <&us_euro_hs_sel>;
+ us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
+};
+
&tlmm {
tp_int_odl: tp-int-odl {
pins = "gpio7";
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index d58045d..f247403 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -20,6 +20,14 @@
#include "sc7280-chrome-common.dtsi"
/ {
+ max98360a: audio-codec-0 {
+ compatible = "maxim,max98360a";
+ pinctrl-names = "default";
+ pinctrl-0 = <&_en>;
+ sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
+ #sound-dai-cells = <0>;
+ };
+
chosen {
stdout-path = "serial0:115200n8";
};
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 2f863c0..8dad599 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -20,6 +20,42 @@
serial1 = &uart7;
};
+ max98360a: audio-codec-0 {
+ compatible = "maxim,max98360a";
+ pinctrl-names = "default";
+ pinctrl-0 = <&_en>;
+ sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
+ #sound-dai-cells = <0>;
+ };
+
+ wcd938x: audio-codec-1 {
+ compatible = "qcom,wcd9385-codec";
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&wcd_reset_n>;
+ pinctrl-1 = <&wcd_reset_n_sleep>;
+
+ reset-gpios = <&tlmm 83 GPIO_ACTIVE_HIGH>;
+
+ qcom,rx-device = <&wcd_rx>;
+ qcom,tx-device = <&wcd_tx>;
+
+ vdd-rxtx-supply = <&vreg_l18b_1p8>;
+ vdd-io-supply = <&vreg_l18b_1p8>;
+ vdd-buck-supply = <&vreg_l17b_1p8>;
+ vdd-mic-bias-supply = <&vreg_bob>;
+
+ qcom,micbias1-microvolt = <1800000>;
+ qcom,micbias2-microvolt = <1800000>;
+ qcom,micbias3-microvolt = <1800000>;
+ qcom,micbias4-microvolt = <1800000>;
+
+ qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000 237000 500000 500000
+ 500000 500000 500000>;
+ qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
+ qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
+ #sound-dai-cells = <1>;
+ };
+
gpio-keys {
compatible = "gpio-keys";
label = "gpio-keys";
@@ -238,6 +274,19 @@
modem-init;
};
+&lpass_rx_macro {
+ status = "okay";
+};
+
+&lpass_tx_macro {
+ status = "okay";
+};
+
+&lpass_va_macro {
+ status = "okay";
+ vdd-micb-supply = <&vreg_bob>;
+};
+
&pcie1 {
status = "okay";
perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
@@ -298,6 +347,26 @@
cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
};
+&swr0 {
+ status = "okay";
+ wcd_rx: codec@0,4 {
+ compatible = "sdw20217010d00";
+ reg = <0 4>;
+ #sound-dai-cells = <1>;
+ qcom,rx-port-mapping = <1 2 3 4 5>;
+ };
+};
+
+&swr1 {
+ status = "okay";
+ wcd_tx: codec@0,3 {
+ compatible = "sdw20217010d00";
+ reg = <0 3>;
+ #sound-dai-cells = <1>;
+ qcom,tx-port-mapping = <1 2 3 4>;
+ };
+};
+
&uart5 {
compatible = "qcom,geni-debug-uart";
status = "okay";
@@ -561,6 +630,12 @@
};
&tlmm {
+ amp_en: amp-en {
+ pins = "gpio63";
+ bias-pull-down;
+ drive-strength = <2>;
+ };
+
bt_en: bt-en {
pins = "gpio85";
function = "gpio";
@@ -643,6 +718,28 @@
function = "gpio";
bias-pull-down;
};
+
+ us_euro_hs_sel: us-euro-hs-sel {
+ pins = "gpio81";
+ function = "gpio";
+ bias-pull-down;
+ drive-strength = <2>;
+ };
+
+ wcd_reset_n: wcd-reset-n {
+ pins = "gpio83";
+ function = "gpio";
+ drive-strength = <8>;
+ output-high;
+ };
+
+ wcd_reset_n_sleep: wcd-reset-n-sleep {
+ pins = "gpio83";
+ function = "gpio";
+ drive-strength = <8>;
+ bias-disable;
+ output-low;
+ };
};
&remoteproc_wpss {
--
2.7.4
On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
> Add wcd938x and max98360a codecs for audio use case on
> sc7280 based platforms.
> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
> Add amp_en node for max98360a codec.
General note: I don't think it's a good practice to add stuff like this to
multiple boards in a single patch. Why?
First the subject of such a patch tends to be vague ("arm64: dts: qcom:
sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it gives
no hint about the boards. If someone was interested in picking changes
for a given board they can't easily identify from the subject that the
change is relevant for them.
Changes touching multiple boards are more likely to cause conflicts when
being picked (or reverted), both upstream and in downstream trees (which
unfortunately have to exist for product development). Downstream trees
might only pick changes for the board(s) they target, patches that touch
mutiple boards often cause conflicts due to context deltas in the
'irrelevant' boards.
Lastly it's usually easier to get a patch reviewed (in the sense of
getting a 'Reviewed-by' tag) and landed that does a single thing.
> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
> Co-developed-by: Venkata Prasad Potturu <[email protected]>
> Signed-off-by: Venkata Prasad Potturu <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts | 6 ++
> arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 8 +++
> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 97 ++++++++++++++++++++++++++
> 3 files changed, 111 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> index 344338a..aa0bf6e2 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
> pins = "gpio51";
> };
>
> +&wcd938x {
> + pinctrl-names = "default";
> + pinctrl-0 = <&us_euro_hs_sel>;
> + us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
> +};
Since this is added for the CRD rev3 it probably should also be added to
sc7280-herobrine-crd.dts
> +
> &tlmm {
> tp_int_odl: tp-int-odl {
> pins = "gpio7";
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> index d58045d..f247403 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> @@ -20,6 +20,14 @@
> #include "sc7280-chrome-common.dtsi"
>
> / {
> + max98360a: audio-codec-0 {
> + compatible = "maxim,max98360a";
> + pinctrl-names = "default";
> + pinctrl-0 = <&_en>;
> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> + #sound-dai-cells = <0>;
> + };
> +
> chosen {
> stdout-path = "serial0:115200n8";
> };
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index 2f863c0..8dad599 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -20,6 +20,42 @@
> serial1 = &uart7;
> };
>
> + max98360a: audio-codec-0 {
> + compatible = "maxim,max98360a";
> + pinctrl-names = "default";
> + pinctrl-0 = <&_en>;
> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> + #sound-dai-cells = <0>;
> + };
> +
> + wcd938x: audio-codec-1 {
Why 'wcd938x' and not 'wcd9385'?
> + compatible = "qcom,wcd9385-codec";
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&wcd_reset_n>;
> + pinctrl-1 = <&wcd_reset_n_sleep>;
> +
> + reset-gpios = <&tlmm 83 GPIO_ACTIVE_HIGH>;
> +
> + qcom,rx-device = <&wcd_rx>;
> + qcom,tx-device = <&wcd_tx>;
> +
> + vdd-rxtx-supply = <&vreg_l18b_1p8>;
> + vdd-io-supply = <&vreg_l18b_1p8>;
> + vdd-buck-supply = <&vreg_l17b_1p8>;
> + vdd-mic-bias-supply = <&vreg_bob>;
> +
> + qcom,micbias1-microvolt = <1800000>;
> + qcom,micbias2-microvolt = <1800000>;
> + qcom,micbias3-microvolt = <1800000>;
> + qcom,micbias4-microvolt = <1800000>;
> +
> + qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000 237000 500000 500000
> + 500000 500000 500000>;
> + qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
> + qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
> + #sound-dai-cells = <1>;
> + };
Also add this node to sc7280-herobrine-crd.dts?
> +
> gpio-keys {
> compatible = "gpio-keys";
> label = "gpio-keys";
> @@ -238,6 +274,19 @@
> modem-init;
> };
>
> +&lpass_rx_macro {
> + status = "okay";
> +};
> +
> +&lpass_tx_macro {
> + status = "okay";
> +};
> +
> +&lpass_va_macro {
> + status = "okay";
> + vdd-micb-supply = <&vreg_bob>;
> +};
Enable these also in sc7280-herobrine.dtsi if other nodes are added to
sc7280-herobrine-crd.dts.
> +
> &pcie1 {
> status = "okay";
> perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
> @@ -298,6 +347,26 @@
> cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
> };
>
> +&swr0 {
> + status = "okay";
nit: add an empty line between properties and nodes
> + wcd_rx: codec@0,4 {
> + compatible = "sdw20217010d00";
> + reg = <0 4>;
> + #sound-dai-cells = <1>;
> + qcom,rx-port-mapping = <1 2 3 4 5>;
> + };
> +};
> +
> +&swr1 {
> + status = "okay";
ditto
> + wcd_tx: codec@0,3 {
> + compatible = "sdw20217010d00";
> + reg = <0 3>;
> + #sound-dai-cells = <1>;
> + qcom,tx-port-mapping = <1 2 3 4>;
> + };
> +};
Also add these to sc7280-herobrine-crd.dts?
> +
> &uart5 {
> compatible = "qcom,geni-debug-uart";
> status = "okay";
> @@ -561,6 +630,12 @@
> };
>
> &tlmm {
> + amp_en: amp-en {
> + pins = "gpio63";
> + bias-pull-down;
> + drive-strength = <2>;
> + };
> +
> bt_en: bt-en {
> pins = "gpio85";
> function = "gpio";
> @@ -643,6 +718,28 @@
> function = "gpio";
> bias-pull-down;
> };
> +
> + us_euro_hs_sel: us-euro-hs-sel {
> + pins = "gpio81";
> + function = "gpio";
> + bias-pull-down;
> + drive-strength = <2>;
> + };
> +
> + wcd_reset_n: wcd-reset-n {
> + pins = "gpio83";
> + function = "gpio";
> + drive-strength = <8>;
> + output-high;
> + };
> +
> + wcd_reset_n_sleep: wcd-reset-n-sleep {
> + pins = "gpio83";
> + function = "gpio";
> + drive-strength = <8>;
> + bias-disable;
> + output-low;
> + };
Also add to sc7280-herobrine-crd.dts if the other nodes are added.
On 4/19/2022 4:54 PM, Srinivasa Rao Mandadapu wrote:
>
> On 4/18/2022 10:15 PM, Matthias Kaehlcke wrote:
> Thanks for your time and valuable inputs Matthias!!!
>> On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
>>> Add wcd938x and max98360a codecs for audio use case on
>>> sc7280 based platforms.
>>> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
>>> Add amp_en node for max98360a codec.
>> General note: I don't think it's a good practice to add stuff like
>> this to
>> multiple boards in a single patch. Why?
>>
>> First the subject of such a patch tends to be vague ("arm64: dts: qcom:
>> sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it
>> gives
>> no hint about the boards. If someone was interested in picking changes
>> for a given board they can't easily identify from the subject that the
>> change is relevant for them.
>>
>> Changes touching multiple boards are more likely to cause conflicts when
>> being picked (or reverted), both upstream and in downstream trees (which
>> unfortunately have to exist for product development). Downstream trees
>> might only pick changes for the board(s) they target, patches that touch
>> mutiple boards often cause conflicts due to context deltas in the
>> 'irrelevant' boards.
>>
>> Lastly it's usually easier to get a patch reviewed (in the sense of
>> getting a 'Reviewed-by' tag) and landed that does a single thing.
>
> Yes, agree to your opinion. In a nutshell, we will include board
> name(ex: herobrine)
>
> in commit message and split the patches per external codec.
>
> Actually, in Initial herobrine boards, EVT and IDP, has both maxim
> speaker and WCD codec,
>
> hence we included in same patch.
>
>>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>>> Co-developed-by: Venkata Prasad Potturu <[email protected]>
>>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>>> ---
>>> arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts | 6 ++
>>> arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 8 +++
>>> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 97
>>> ++++++++++++++++++++++++++
>>> 3 files changed, 111 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>> b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>> index 344338a..aa0bf6e2 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>> @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
>>> pins = "gpio51";
>>> };
>>> +&wcd938x {
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&us_euro_hs_sel>;
>>> + us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
>>> +};
>> Since this is added for the CRD rev3 it probably should also be added to
>> sc7280-herobrine-crd.dts
> Okay. Will add in corresponding latest herobrine CRD dts file also.
>>
>>> +
>>> &tlmm {
>>> tp_int_odl: tp-int-odl {
>>> pins = "gpio7";
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> index d58045d..f247403 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>> @@ -20,6 +20,14 @@
>>> #include "sc7280-chrome-common.dtsi"
>>> / {
>>> + max98360a: audio-codec-0 {
>>> + compatible = "maxim,max98360a";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&_en>;
>>> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>>> + #sound-dai-cells = <0>;
>>> + };
>>> +
>>> chosen {
>>> stdout-path = "serial0:115200n8";
>>> };
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> index 2f863c0..8dad599 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> @@ -20,6 +20,42 @@
>>> serial1 = &uart7;
>>> };
>>> + max98360a: audio-codec-0 {
>>> + compatible = "maxim,max98360a";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&_en>;
>>> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>>> + #sound-dai-cells = <0>;
>>> + };
>>> +
>>> + wcd938x: audio-codec-1 {
>> Why 'wcd938x' and not 'wcd9385'?
>
> Actually same driver is used for both wcd9380 and wcd9385. Here we can
> use specific name as per board.
>
> Will change accordingly.
At present, dt-bindgs also has wcd938x. So will update the name in
bindings and here post this series.
is it okay?
>
>>
>>> + compatible = "qcom,wcd9385-codec";
>>> + pinctrl-names = "default", "sleep";
>>> + pinctrl-0 = <&wcd_reset_n>;
>>> + pinctrl-1 = <&wcd_reset_n_sleep>;
>>> +
>>> + reset-gpios = <&tlmm 83 GPIO_ACTIVE_HIGH>;
>>> +
>>> + qcom,rx-device = <&wcd_rx>;
>>> + qcom,tx-device = <&wcd_tx>;
>>> +
>>> + vdd-rxtx-supply = <&vreg_l18b_1p8>;
>>> + vdd-io-supply = <&vreg_l18b_1p8>;
>>> + vdd-buck-supply = <&vreg_l17b_1p8>;
>>> + vdd-mic-bias-supply = <&vreg_bob>;
>>> +
>>> + qcom,micbias1-microvolt = <1800000>;
>>> + qcom,micbias2-microvolt = <1800000>;
>>> + qcom,micbias3-microvolt = <1800000>;
>>> + qcom,micbias4-microvolt = <1800000>;
>>> +
>>> + qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000
>>> 237000 500000 500000
>>> + 500000 500000 500000>;
>>> + qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
>>> + qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
>>> + #sound-dai-cells = <1>;
>>> + };
>> Also add this node to sc7280-herobrine-crd.dts?
> Okay. will do accordingly.
>>
>>> +
>>> gpio-keys {
>>> compatible = "gpio-keys";
>>> label = "gpio-keys";
>>> @@ -238,6 +274,19 @@
>>> modem-init;
>>> };
>>> +&lpass_rx_macro {
>>> + status = "okay";
>>> +};
>>> +
>>> +&lpass_tx_macro {
>>> + status = "okay";
>>> +};
>>> +
>>> +&lpass_va_macro {
>>> + status = "okay";
>>> + vdd-micb-supply = <&vreg_bob>;
>>> +};
>> Enable these also in sc7280-herobrine.dtsi if other nodes are added to
>> sc7280-herobrine-crd.dts.
> Okay.
>>
>>> +
>>> &pcie1 {
>>> status = "okay";
>>> perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
>>> @@ -298,6 +347,26 @@
>>> cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
>>> };
>>> +&swr0 {
>>> + status = "okay";
>> nit: add an empty line between properties and nodes
> Okay.
>>
>>> + wcd_rx: codec@0,4 {
>>> + compatible = "sdw20217010d00";
>>> + reg = <0 4>;
>>> + #sound-dai-cells = <1>;
>>> + qcom,rx-port-mapping = <1 2 3 4 5>;
>>> + };
>>> +};
>>> +
>>> +&swr1 {
>>> + status = "okay";
>> ditto
> Okay.
>>
>>> + wcd_tx: codec@0,3 {
>>> + compatible = "sdw20217010d00";
>>> + reg = <0 3>;
>>> + #sound-dai-cells = <1>;
>>> + qcom,tx-port-mapping = <1 2 3 4>;
>>> + };
>>> +};
>> Also add these to sc7280-herobrine-crd.dts?
> Okay.
>>
>>> +
>>> &uart5 {
>>> compatible = "qcom,geni-debug-uart";
>>> status = "okay";
>>> @@ -561,6 +630,12 @@
>>> };
>>> &tlmm {
>>> + amp_en: amp-en {
>>> + pins = "gpio63";
>>> + bias-pull-down;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>> bt_en: bt-en {
>>> pins = "gpio85";
>>> function = "gpio";
>>> @@ -643,6 +718,28 @@
>>> function = "gpio";
>>> bias-pull-down;
>>> };
>>> +
>>> + us_euro_hs_sel: us-euro-hs-sel {
>>> + pins = "gpio81";
>>> + function = "gpio";
>>> + bias-pull-down;
>>> + drive-strength = <2>;
>>> + };
>>> +
>>> + wcd_reset_n: wcd-reset-n {
>>> + pins = "gpio83";
>>> + function = "gpio";
>>> + drive-strength = <8>;
>>> + output-high;
>>> + };
>>> +
>>> + wcd_reset_n_sleep: wcd-reset-n-sleep {
>>> + pins = "gpio83";
>>> + function = "gpio";
>>> + drive-strength = <8>;
>>> + bias-disable;
>>> + output-low;
>>> + };
>> Also add to sc7280-herobrine-crd.dts if the other nodes are added.
> Okay.
On 4/18/2022 10:15 PM, Matthias Kaehlcke wrote:
Thanks for your time and valuable inputs Matthias!!!
> On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
>> Add wcd938x and max98360a codecs for audio use case on
>> sc7280 based platforms.
>> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
>> Add amp_en node for max98360a codec.
> General note: I don't think it's a good practice to add stuff like this to
> multiple boards in a single patch. Why?
>
> First the subject of such a patch tends to be vague ("arm64: dts: qcom:
> sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it gives
> no hint about the boards. If someone was interested in picking changes
> for a given board they can't easily identify from the subject that the
> change is relevant for them.
>
> Changes touching multiple boards are more likely to cause conflicts when
> being picked (or reverted), both upstream and in downstream trees (which
> unfortunately have to exist for product development). Downstream trees
> might only pick changes for the board(s) they target, patches that touch
> mutiple boards often cause conflicts due to context deltas in the
> 'irrelevant' boards.
>
> Lastly it's usually easier to get a patch reviewed (in the sense of
> getting a 'Reviewed-by' tag) and landed that does a single thing.
Yes, agree to your opinion. In a nutshell, we will include board
name(ex: herobrine)
in commit message and split the patches per external codec.
Actually, in Initial herobrine boards, EVT and IDP, has both maxim
speaker and WCD codec,
hence we included in same patch.
>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>> Co-developed-by: Venkata Prasad Potturu <[email protected]>
>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts | 6 ++
>> arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 8 +++
>> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 97 ++++++++++++++++++++++++++
>> 3 files changed, 111 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> index 344338a..aa0bf6e2 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>> @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
>> pins = "gpio51";
>> };
>>
>> +&wcd938x {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&us_euro_hs_sel>;
>> + us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
>> +};
> Since this is added for the CRD rev3 it probably should also be added to
> sc7280-herobrine-crd.dts
Okay. Will add in corresponding latest herobrine CRD dts file also.
>
>> +
>> &tlmm {
>> tp_int_odl: tp-int-odl {
>> pins = "gpio7";
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> index d58045d..f247403 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>> @@ -20,6 +20,14 @@
>> #include "sc7280-chrome-common.dtsi"
>>
>> / {
>> + max98360a: audio-codec-0 {
>> + compatible = "maxim,max98360a";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&_en>;
>> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>> + #sound-dai-cells = <0>;
>> + };
>> +
>> chosen {
>> stdout-path = "serial0:115200n8";
>> };
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> index 2f863c0..8dad599 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> @@ -20,6 +20,42 @@
>> serial1 = &uart7;
>> };
>>
>> + max98360a: audio-codec-0 {
>> + compatible = "maxim,max98360a";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&_en>;
>> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>> + #sound-dai-cells = <0>;
>> + };
>> +
>> + wcd938x: audio-codec-1 {
> Why 'wcd938x' and not 'wcd9385'?
Actually same driver is used for both wcd9380 and wcd9385. Here we can
use specific name as per board.
Will change accordingly.
>
>> + compatible = "qcom,wcd9385-codec";
>> + pinctrl-names = "default", "sleep";
>> + pinctrl-0 = <&wcd_reset_n>;
>> + pinctrl-1 = <&wcd_reset_n_sleep>;
>> +
>> + reset-gpios = <&tlmm 83 GPIO_ACTIVE_HIGH>;
>> +
>> + qcom,rx-device = <&wcd_rx>;
>> + qcom,tx-device = <&wcd_tx>;
>> +
>> + vdd-rxtx-supply = <&vreg_l18b_1p8>;
>> + vdd-io-supply = <&vreg_l18b_1p8>;
>> + vdd-buck-supply = <&vreg_l17b_1p8>;
>> + vdd-mic-bias-supply = <&vreg_bob>;
>> +
>> + qcom,micbias1-microvolt = <1800000>;
>> + qcom,micbias2-microvolt = <1800000>;
>> + qcom,micbias3-microvolt = <1800000>;
>> + qcom,micbias4-microvolt = <1800000>;
>> +
>> + qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000 237000 500000 500000
>> + 500000 500000 500000>;
>> + qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
>> + qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
>> + #sound-dai-cells = <1>;
>> + };
> Also add this node to sc7280-herobrine-crd.dts?
Okay. will do accordingly.
>
>> +
>> gpio-keys {
>> compatible = "gpio-keys";
>> label = "gpio-keys";
>> @@ -238,6 +274,19 @@
>> modem-init;
>> };
>>
>> +&lpass_rx_macro {
>> + status = "okay";
>> +};
>> +
>> +&lpass_tx_macro {
>> + status = "okay";
>> +};
>> +
>> +&lpass_va_macro {
>> + status = "okay";
>> + vdd-micb-supply = <&vreg_bob>;
>> +};
> Enable these also in sc7280-herobrine.dtsi if other nodes are added to
> sc7280-herobrine-crd.dts.
Okay.
>
>> +
>> &pcie1 {
>> status = "okay";
>> perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
>> @@ -298,6 +347,26 @@
>> cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
>> };
>>
>> +&swr0 {
>> + status = "okay";
> nit: add an empty line between properties and nodes
Okay.
>
>> + wcd_rx: codec@0,4 {
>> + compatible = "sdw20217010d00";
>> + reg = <0 4>;
>> + #sound-dai-cells = <1>;
>> + qcom,rx-port-mapping = <1 2 3 4 5>;
>> + };
>> +};
>> +
>> +&swr1 {
>> + status = "okay";
> ditto
Okay.
>
>> + wcd_tx: codec@0,3 {
>> + compatible = "sdw20217010d00";
>> + reg = <0 3>;
>> + #sound-dai-cells = <1>;
>> + qcom,tx-port-mapping = <1 2 3 4>;
>> + };
>> +};
> Also add these to sc7280-herobrine-crd.dts?
Okay.
>
>> +
>> &uart5 {
>> compatible = "qcom,geni-debug-uart";
>> status = "okay";
>> @@ -561,6 +630,12 @@
>> };
>>
>> &tlmm {
>> + amp_en: amp-en {
>> + pins = "gpio63";
>> + bias-pull-down;
>> + drive-strength = <2>;
>> + };
>> +
>> bt_en: bt-en {
>> pins = "gpio85";
>> function = "gpio";
>> @@ -643,6 +718,28 @@
>> function = "gpio";
>> bias-pull-down;
>> };
>> +
>> + us_euro_hs_sel: us-euro-hs-sel {
>> + pins = "gpio81";
>> + function = "gpio";
>> + bias-pull-down;
>> + drive-strength = <2>;
>> + };
>> +
>> + wcd_reset_n: wcd-reset-n {
>> + pins = "gpio83";
>> + function = "gpio";
>> + drive-strength = <8>;
>> + output-high;
>> + };
>> +
>> + wcd_reset_n_sleep: wcd-reset-n-sleep {
>> + pins = "gpio83";
>> + function = "gpio";
>> + drive-strength = <8>;
>> + bias-disable;
>> + output-low;
>> + };
> Also add to sc7280-herobrine-crd.dts if the other nodes are added.
Okay.
On 4/21/2022 9:12 PM, Matthias Kaehlcke wrote:
Thanks for your time Matthias!!
> On Thu, Apr 21, 2022 at 12:31:52PM +0530, Srinivasa Rao Mandadapu wrote:
>> On 4/19/2022 4:54 PM, Srinivasa Rao Mandadapu wrote:
>>> On 4/18/2022 10:15 PM, Matthias Kaehlcke wrote:
>>> Thanks for your time and valuable inputs Matthias!!!
>>>> On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
>>>>> Add wcd938x and max98360a codecs for audio use case on
>>>>> sc7280 based platforms.
>>>>> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
>>>>> Add amp_en node for max98360a codec.
>>>> General note: I don't think it's a good practice to add stuff like
>>>> this to
>>>> multiple boards in a single patch. Why?
>>>>
>>>> First the subject of such a patch tends to be vague ("arm64: dts: qcom:
>>>> sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it
>>>> gives
>>>> no hint about the boards. If someone was interested in picking changes
>>>> for a given board they can't easily identify from the subject that the
>>>> change is relevant for them.
>>>>
>>>> Changes touching multiple boards are more likely to cause conflicts when
>>>> being picked (or reverted), both upstream and in downstream trees (which
>>>> unfortunately have to exist for product development). Downstream trees
>>>> might only pick changes for the board(s) they target, patches that touch
>>>> mutiple boards often cause conflicts due to context deltas in the
>>>> 'irrelevant' boards.
>>>>
>>>> Lastly it's usually easier to get a patch reviewed (in the sense of
>>>> getting a 'Reviewed-by' tag) and landed that does a single thing.
>>> Yes, agree to your opinion. In a nutshell, we will include board
>>> name(ex: herobrine)
>>>
>>> in commit message and split the patches per external codec.
>>>
>>> Actually, in Initial herobrine boards, EVT and IDP, has both maxim
>>> speaker and WCD codec,
>>>
>>> hence we included in same patch.
>>>
>>>>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>>>>> Co-developed-by: Venkata Prasad Potturu <[email protected]>
>>>>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>>>>> ---
>>>>> arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts | 6 ++
>>>>> arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 8 +++
>>>>> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 97
>>>>> ++++++++++++++++++++++++++
>>>>> 3 files changed, 111 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>>>> b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>>>> index 344338a..aa0bf6e2 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
>>>>> @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
>>>>> pins = "gpio51";
>>>>> };
>>>>> +&wcd938x {
>>>>> + pinctrl-names = "default";
>>>>> + pinctrl-0 = <&us_euro_hs_sel>;
>>>>> + us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
>>>>> +};
>>>> Since this is added for the CRD rev3 it probably should also be added to
>>>> sc7280-herobrine-crd.dts
>>> Okay. Will add in corresponding latest herobrine CRD dts file also.
>>>>> +
>>>>> &tlmm {
>>>>> tp_int_odl: tp-int-odl {
>>>>> pins = "gpio7";
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> index d58045d..f247403 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
>>>>> @@ -20,6 +20,14 @@
>>>>> #include "sc7280-chrome-common.dtsi"
>>>>> / {
>>>>> + max98360a: audio-codec-0 {
>>>>> + compatible = "maxim,max98360a";
>>>>> + pinctrl-names = "default";
>>>>> + pinctrl-0 = <&_en>;
>>>>> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>>>>> + #sound-dai-cells = <0>;
>>>>> + };
>>>>> +
>>>>> chosen {
>>>>> stdout-path = "serial0:115200n8";
>>>>> };
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> index 2f863c0..8dad599 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>>>> @@ -20,6 +20,42 @@
>>>>> serial1 = &uart7;
>>>>> };
>>>>> + max98360a: audio-codec-0 {
>>>>> + compatible = "maxim,max98360a";
>>>>> + pinctrl-names = "default";
>>>>> + pinctrl-0 = <&_en>;
>>>>> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
>>>>> + #sound-dai-cells = <0>;
>>>>> + };
>>>>> +
>>>>> + wcd938x: audio-codec-1 {
>>>> Why 'wcd938x' and not 'wcd9385'?
>>> Actually same driver is used for both wcd9380 and wcd9385. Here we can
>>> use specific name as per board.
>>>
>>> Will change accordingly.
>> At present, dt-bindgs also has wcd938x. So will update the name in bindings
>> and here post this series.
>>
>> is it okay?
> I don't think it's strictly necessary to update the binding, as
> 'wcd938x' is only used in the example, and also it's not really
> wrong. Then again, if the example in the binding uses a
> specific wcd version it might make it less likely that the
> wildcard name is used in future board DTs. Up to you :)
Okay. Will update accordingly.
On Thu, Apr 21, 2022 at 12:31:52PM +0530, Srinivasa Rao Mandadapu wrote:
>
> On 4/19/2022 4:54 PM, Srinivasa Rao Mandadapu wrote:
> >
> > On 4/18/2022 10:15 PM, Matthias Kaehlcke wrote:
> > Thanks for your time and valuable inputs Matthias!!!
> > > On Mon, Apr 18, 2022 at 07:44:10PM +0530, Srinivasa Rao Mandadapu wrote:
> > > > Add wcd938x and max98360a codecs for audio use case on
> > > > sc7280 based platforms.
> > > > Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
> > > > Add amp_en node for max98360a codec.
> > > General note: I don't think it's a good practice to add stuff like
> > > this to
> > > multiple boards in a single patch. Why?
> > >
> > > First the subject of such a patch tends to be vague ("arm64: dts: qcom:
> > > sc7280: Add nodes for wcd9385 and max98360a codec"), in this case it
> > > gives
> > > no hint about the boards. If someone was interested in picking changes
> > > for a given board they can't easily identify from the subject that the
> > > change is relevant for them.
> > >
> > > Changes touching multiple boards are more likely to cause conflicts when
> > > being picked (or reverted), both upstream and in downstream trees (which
> > > unfortunately have to exist for product development). Downstream trees
> > > might only pick changes for the board(s) they target, patches that touch
> > > mutiple boards often cause conflicts due to context deltas in the
> > > 'irrelevant' boards.
> > >
> > > Lastly it's usually easier to get a patch reviewed (in the sense of
> > > getting a 'Reviewed-by' tag) and landed that does a single thing.
> >
> > Yes, agree to your opinion. In a nutshell, we will include board
> > name(ex: herobrine)
> >
> > in commit message and split the patches per external codec.
> >
> > Actually, in Initial herobrine boards, EVT and IDP, has both maxim
> > speaker and WCD codec,
> >
> > hence we included in same patch.
> >
> > > > Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
> > > > Co-developed-by: Venkata Prasad Potturu <[email protected]>
> > > > Signed-off-by: Venkata Prasad Potturu <[email protected]>
> > > > ---
> > > > arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts | 6 ++
> > > > arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 8 +++
> > > > arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 97
> > > > ++++++++++++++++++++++++++
> > > > 3 files changed, 111 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> > > > b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> > > > index 344338a..aa0bf6e2 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd-r3.dts
> > > > @@ -87,6 +87,12 @@ ap_ts_pen_1v8: &i2c13 {
> > > > pins = "gpio51";
> > > > };
> > > > +&wcd938x {
> > > > + pinctrl-names = "default";
> > > > + pinctrl-0 = <&us_euro_hs_sel>;
> > > > + us-euro-gpios = <&tlmm 81 GPIO_ACTIVE_HIGH>;
> > > > +};
> > > Since this is added for the CRD rev3 it probably should also be added to
> > > sc7280-herobrine-crd.dts
> > Okay. Will add in corresponding latest herobrine CRD dts file also.
> > >
> > > > +
> > > > &tlmm {
> > > > tp_int_odl: tp-int-odl {
> > > > pins = "gpio7";
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > > > b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > > > index d58045d..f247403 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > > > @@ -20,6 +20,14 @@
> > > > #include "sc7280-chrome-common.dtsi"
> > > > / {
> > > > + max98360a: audio-codec-0 {
> > > > + compatible = "maxim,max98360a";
> > > > + pinctrl-names = "default";
> > > > + pinctrl-0 = <&_en>;
> > > > + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> > > > + #sound-dai-cells = <0>;
> > > > + };
> > > > +
> > > > chosen {
> > > > stdout-path = "serial0:115200n8";
> > > > };
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > index 2f863c0..8dad599 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> > > > @@ -20,6 +20,42 @@
> > > > serial1 = &uart7;
> > > > };
> > > > + max98360a: audio-codec-0 {
> > > > + compatible = "maxim,max98360a";
> > > > + pinctrl-names = "default";
> > > > + pinctrl-0 = <&_en>;
> > > > + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> > > > + #sound-dai-cells = <0>;
> > > > + };
> > > > +
> > > > + wcd938x: audio-codec-1 {
> > > Why 'wcd938x' and not 'wcd9385'?
> >
> > Actually same driver is used for both wcd9380 and wcd9385. Here we can
> > use specific name as per board.
> >
> > Will change accordingly.
>
> At present, dt-bindgs also has wcd938x. So will update the name in bindings
> and here post this series.
>
> is it okay?
I don't think it's strictly necessary to update the binding, as
'wcd938x' is only used in the example, and also it's not really
wrong. Then again, if the example in the binding uses a
specific wcd version it might make it less likely that the
wildcard name is used in future board DTs. Up to you :)