Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <fb53452a-0898-407e-5e86-897aeab446bc@quicinc.com>
Date:   Fri, 22 Apr 2022 18:56:41 +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
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>
 <5d4b7c44-62e1-aaa6-3116-f58e1a1b437b@quicinc.com>
 <YmF7TRlxUWWQ394e@google.com>
From:   Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Organization: Qualcomm
In-Reply-To: <YmF7TRlxUWWQ394e@google.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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