Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5d4b7c44-62e1-aaa6-3116-f58e1a1b437b@quicinc.com>
Date:   Thu, 21 Apr 2022 12:31:52 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385
 and max98360a codec
Content-Language: en-US
From:   Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
To:     Matthias Kaehlcke <mka@chromium.org>
CC:     <agross@kernel.org>, <bjorn.andersson@linaro.org>,
        <robh+dt@kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_rohkumar@quicinc.com>, <srinivas.kandagatla@linaro.org>,
        <dianders@chromium.org>, <swboyd@chromium.org>,
        <judyhsiao@chromium.org>,
        Venkata Prasad Potturu <quic_potturu@quicinc.com>
References: <1650291252-30398-1-git-send-email-quic_srivasam@quicinc.com>
 <1650291252-30398-3-git-send-email-quic_srivasam@quicinc.com>
 <Yl2VmW18QAJl9v+m@google.com>
 <5ac149ac-4862-e8c9-185c-524c4b111961@quicinc.com>
Organization: Qualcomm
In-Reply-To: <5ac149ac-4862-e8c9-185c-524c4b111961@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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@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.

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.