Messages in this thread | | | Date | Tue, 19 Apr 2022 16:54:49 +0530 | Subject | Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec | From | Srinivasa Rao Mandadapu <> |
| |
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 = <&_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.
| |