Messages in this thread | | | Date | Mon, 11 Apr 2022 12:53:33 -0700 | From | Matthias Kaehlcke <> | Subject | Re: [PATCH v6 1/3] arm64: dts: qcom: sc7280: Add nodes for va tx and rx macros and external codecs |
| |
On Mon, Apr 11, 2022 at 07:32:33PM +0530, Srinivasa Rao Mandadapu wrote: > > On 4/6/2022 3:05 AM, Matthias Kaehlcke wrote: > Thanks for your time Matthias!!! > > On Tue, Apr 05, 2022 at 04:43:38PM +0530, Srinivasa Rao Mandadapu wrote: > > > SC7280 has VA, TX and RX macros with SoundWire Controllers to attach with > > > codecs like WCD938x, max98360a using soundwire masters and i2s bus. > > > Add these nodes for sc7280 based platforms audio use case. > > > Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset. > > > > > > 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> > > > --- > > A change log would be helpful for reviewers > Change log added in cover letter.
From my perspective as a reviewer I find it more practical to have a change log per patch.
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi > > > index de646d9..c6a04c3 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>; > > > + }; > > > + > > This node shouldn't be at top but with the other device nodes, in > > alphabetical order, i.e. before 'pwmleds'. > > Actually we are sorting as per node name. Hence kept it here. As per > previous reviewer comments, sorted accordingly. > > Please suggest better approach.
True, I forgot the node names are used for sorting, not the labels.
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > > > index db74fc3..78ec84c 100644 > > > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > > > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > > > @@ -822,6 +822,127 @@ > > > #power-domain-cells = <1>; > > > }; > > > + rxmacro: codec@3200000 { > > These node are not at the correct position. They should be sorted by > > address and hence be inserted between 'lpasscc@3000000' and > > 'interconnect@3c40000'. > > Actually we are sorting as per node(codec) name. Hence kept it here. As per > previous reviewer comments, sorted accordingly.
Could you provide a pointer to those comments?
My understanding is that we are sorting by node name when nodes don't have addresses or for overrides in board files/snippets, however the nodes under 'soc@0' are sorted by address.
Another nit: 'rx_macro, tx_macro, va_macro' instead of the labels without underscore? Not really important, but maybe slightly more readable and would match the 'spelling' of the compatible strings.
| |