lkml.org 
[lkml]   [2022]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec
From

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 <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-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 = <&amp_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 = <&amp_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.

\
 
 \ /
  Last update: 2022-04-19 13:26    [W:0.096 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site