| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5d4b7c44-62e1-aaa6-3116-f58e1a1b437b@quicinc.com>
Date: Thu, 21 Apr 2022 12:31:52 +0530
From: Srinivasa Rao Mandadapu <quic_srivasam@...cinc.com>
To: Matthias Kaehlcke <mka@...omium.org>
CC: <agross@...nel.org>, <bjorn.andersson@...aro.org>,
<robh+dt@...nel.org>, <linux-arm-msm@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<quic_rohkumar@...cinc.com>, <srinivas.kandagatla@...aro.org>,
<dianders@...omium.org>, <swboyd@...omium.org>,
<judyhsiao@...omium.org>,
Venkata Prasad Potturu <quic_potturu@...cinc.com>
Subject: Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385
and max98360a codec
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 <quic_srivasam@...cinc.com>
>>> Co-developed-by: Venkata Prasad Potturu <quic_potturu@...cinc.com>
>>> Signed-off-by: Venkata Prasad Potturu <quic_potturu@...cinc.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.
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.
Powered by blists - more mailing lists