Re: [PATCH v6 1/2] arm64: dts: qcom: sc7180-trogdor: Add lpass dai link for I2S driver

[Srinivasa Rao Mandadapu]

Hi Stephen,

Thanks for Your Time and Inputs!!!

On 3/14/2021 2:26 AM, Stephen Boyd wrote:
> Quoting Srinivasa Rao Mandadapu (2021-03-12 21:46:53)
> > From: Ajit Pandey <ajitp@xxxxxxxxxxxxxx>
> >
> > Add dai link for supporting lpass I2S driver, which is used
> > for audio capture and playback.
> > Add lpass-cpu node with  pin controls and i2s primary
> Why two spaces before 'pin'?
>
> > and secondary dai-links
> Please end sentence with a period.
>
> > Signed-off-by: Ajit Pandey <ajitp@xxxxxxxxxxxxxx>
> > Signed-off-by: V Sujith Kumar Reddy <vsujithk@xxxxxxxxxxxxxx>
> > Signed-off-by: Srinivasa Rao Mandadapu <srivasam@xxxxxxxxxxxxxx>
> > ---
> >   arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 58 ++++++++++++++++++++
> >   1 file changed, 58 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > index 436582279dad..3a24383247db 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > @@ -283,6 +284,42 @@ keyboard_backlight: keyboard-backlight {
> >                          max-brightness = <1023>;
> >                  };
> >          };
> > +
> > +       sound: sound {
> > +               compatible = "google,sc7180-trogdor";
> > +               model = "sc7180-rt5682-max98357a-1mic";
> > +
> > +               audio-routing =
> > +                       "Headphone Jack", "HPOL",
> > +                       "Headphone Jack", "HPOR";
> > +
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               dai-link@0 {
> > +                       link-name = "MultiMedia0";
> > +                       reg = <MI2S_PRIMARY>;
> > +                       cpu {
> > +                               sound-dai = <&lpass_cpu MI2S_PRIMARY>;
> > +                       };
> > +
> > +                       sound_multimedia0_codec: codec {
> > +                               sound-dai = <&alc5682 0 /*aif1*/>;
> Nitpick, add a space for comment
>
>                                 sound-dai = <&alc5682 0 /* aif1 */>;
Okay. Will re post with Fix.
> > +                       };
> > +               };
> > +
> > +               dai-link@1 {
> > +                       link-name = "MultiMedia1";
> > +                       reg = <MI2S_SECONDARY>;
> > +                       cpu {
> > +                               sound-dai = <&lpass_cpu MI2S_SECONDARY>;
> > +                       };
> > +
> > +                       sound_multimedia1_codec: codec {
> > +                               sound-dai = <&max98357a>;
> > +                       };
> > +               };
> > +       };
> >   };
> >   
> >   &qfprom {
> > @@ -720,6 +757,27 @@ &ipa {
> >          modem-init;
> >   };
> >   
> > +&lpass_cpu {
> > +       status = "okay";
> > +
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&sec_mi2s_active &pri_mi2s_active &pri_mi2s_mclk_active>;
> Super nitpick: I prefer this style
>
> 	pinctrl-0 = <&sec_mi2s_active>, <&pri_mi2s_active>, <&pri_mi2s_mclk_active>;
>
> It's effectively the same but the brackets help us see that these are
> the end of the phandle specifier instead of having to figure out that
> the first phandle isn't specifying the second phandle as an argument.
Okay. Will change accordingly.
> > +
> > +       #address-cells = <1>;
> > +       #size-cells = <0>;
> > +
> > +       mi2s-primary@0 {
> Should the node name just be mi2s instead of mi2s-primary? We have reg
> property so I think 'mi2s' should be sufficient to differentiate.
Okay. I will change it as mi2s@0 instead of mi2s-primary@0
> > +               reg = <MI2S_PRIMARY>;
> > +               qcom,playback-sd-lines = <1>;
> > +               qcom,capture-sd-lines = <0>;
> > +       };
> > +
> > +       mi2s-secondary@1 {
> > +               reg = <MI2S_SECONDARY>;
> > +               qcom,playback-sd-lines = <0>;
> > +       };
> > +};
> > +
> >   &mdp {
> >          status = "okay";
> >   };

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.