Re: [PATCH v8 2/4] arm64: dts: qcom: sc7280: Add nodes for wcd9385 and max98360a codec

[Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>]

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.