Message ID | 1644334454-16719-1-git-send-email-quic_srivasam@quicinc.com |
---|---|
Headers | show |
Series | Add lpass pin control support for audio on sc7280 based targets | expand |
Quoting Srinivasa Rao Mandadapu (2022-02-08 07:34:12) > Add AMP enable node and pinmux for primary and secondary I2S > for SC7280 based platforms. > > 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-idp.dtsi | 40 ++++++++++++++++++++++++++++++++ > arch/arm64/boot/dts/qcom/sc7280.dtsi | 40 ++++++++++++++++++++++++++++++++ > 2 files changed, 80 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > index d623d71..c7d6c46 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > @@ -436,6 +436,39 @@ > qcom,drive-strength = <3>; > }; > }; Newline here > +&pri_mi2s_data0 { > + drive-strength = <6>; > +}; > + > +&pri_mi2s_data1 { > + drive-strength = <6>; > +}; > + > +&pri_mi2s_mclk { > + drive-strength = <6>; > +}; > + > +&pri_mi2s_sclk { > + drive-strength = <6>; > +}; > + > +&pri_mi2s_ws { > + drive-strength = <6>; > +}; > + > +&sec_mi2s_data0 { > + drive-strength = <6>; > + bias-disable; > +}; > + > +&sec_mi2s_sclk { > + drive-strength = <6>; > + bias-disable; > +}; > + > +&sec_mi2s_ws { > + drive-strength = <6>; > +}; Please sort these nodes alphabetically on node name. > > &qspi_cs0 { > bias-disable; > @@ -491,6 +524,13 @@ > }; > > &tlmm { > + amp_en: amp-en { > + pins = "gpio63"; > + function = "gpio"; > + bias-disable; Is there an external pull? > + drive-strength = <2>; > + }; > + > nvme_pwren: nvme-pwren { > function = "gpio"; > }; > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 937c2e0..76e73e9 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -3461,6 +3461,46 @@ > }; > }; > > + pri_mi2s_data0: pri-mi2s-data0 { > + pins = "gpio98"; > + function = "mi2s0_data0"; > + }; > + > + pri_mi2s_data1: pri-mi2s-data1 { > + pins = "gpio99"; > + function = "mi2s0_data1"; > + }; > + > + pri_mi2s_mclk: pri-mi2s-mclk { > + pins = "gpio96"; > + function = "pri_mi2s"; > + }; > + > + pri_mi2s_sclk: pri-mi2s-sclk { > + pins = "gpio97"; > + function = "mi2s0_sck"; > + }; > + > + pri_mi2s_ws: pri-mi2s-ws { > + pins = "gpio100"; > + function = "mi2s0_ws"; > + }; > + > + sec_mi2s_data0: sec-mi2s-data0 { > + pins = "gpio107"; > + function = "mi2s1_data0"; > + }; > + > + sec_mi2s_sclk: sec-mi2s-sclk { > + pins = "gpio106"; > + function = "mi2s1_sck"; > + }; > + > + sec_mi2s_ws: sec-mi2s-ws { > + pins = "gpio108"; > + function = "mi2s1_ws"; > + }; Please sort these nodes alphabetically on node name.
Quoting Srinivasa Rao Mandadapu (2022-02-08 07:34:14) > Add pinmux to reset wcd codec, conneceted on SC7280 based platforms. > > 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-idp.dtsi | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > index 4704a93..6b38fa1 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi > @@ -594,6 +594,21 @@ > */ > bias-pull-up; > }; > + > + wcd938x_reset_active: wcd938x_reset_active { No underscore in node names. > + pins = "gpio83"; > + function = "gpio"; > + drive-strength = <16>; > + output-high; > + }; > + > + wcd938x_reset_sleep: wcd938x_reset_sleep { > + pins = "gpio83"; > + function = "gpio"; > + drive-strength = <16>; > + bias-disable; > + output-low; Why doesn't the device drive the reset gpio by requesting the gpio and asserting and deasserting it? We shouldn't need to use pinctrl settings to toggle reset gpios.
On 2/9/2022 2:38 AM, Stephen Boyd wrote: Thanks for your time Stephen!!! > Quoting Srinivasa Rao Mandadapu (2022-02-08 07:34:12) >> Add AMP enable node and pinmux for primary and secondary I2S >> for SC7280 based platforms. >> >> 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-idp.dtsi | 40 ++++++++++++++++++++++++++++++++ >> arch/arm64/boot/dts/qcom/sc7280.dtsi | 40 ++++++++++++++++++++++++++++++++ >> 2 files changed, 80 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> index d623d71..c7d6c46 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> @@ -436,6 +436,39 @@ >> qcom,drive-strength = <3>; >> }; >> }; > Newline here Okay. will remove it. > >> +&pri_mi2s_data0 { >> + drive-strength = <6>; >> +}; >> + >> +&pri_mi2s_data1 { >> + drive-strength = <6>; >> +}; >> + >> +&pri_mi2s_mclk { >> + drive-strength = <6>; >> +}; >> + >> +&pri_mi2s_sclk { >> + drive-strength = <6>; >> +}; >> + >> +&pri_mi2s_ws { >> + drive-strength = <6>; >> +}; >> + >> +&sec_mi2s_data0 { >> + drive-strength = <6>; >> + bias-disable; >> +}; >> + >> +&sec_mi2s_sclk { >> + drive-strength = <6>; >> + bias-disable; >> +}; >> + >> +&sec_mi2s_ws { >> + drive-strength = <6>; >> +}; > Please sort these nodes alphabetically on node name. Okay. > >> &qspi_cs0 { >> bias-disable; >> @@ -491,6 +524,13 @@ >> }; >> >> &tlmm { >> + amp_en: amp-en { >> + pins = "gpio63"; >> + function = "gpio"; >> + bias-disable; > Is there an external pull? I think no external pull. In trogdor mentioned bias-pull-down but you suggested to remove it. > >> + drive-strength = <2>; >> + }; >> + >> nvme_pwren: nvme-pwren { >> function = "gpio"; >> }; >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> index 937c2e0..76e73e9 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> @@ -3461,6 +3461,46 @@ >> }; >> }; >> >> + pri_mi2s_data0: pri-mi2s-data0 { >> + pins = "gpio98"; >> + function = "mi2s0_data0"; >> + }; >> + >> + pri_mi2s_data1: pri-mi2s-data1 { >> + pins = "gpio99"; >> + function = "mi2s0_data1"; >> + }; >> + >> + pri_mi2s_mclk: pri-mi2s-mclk { >> + pins = "gpio96"; >> + function = "pri_mi2s"; >> + }; >> + >> + pri_mi2s_sclk: pri-mi2s-sclk { >> + pins = "gpio97"; >> + function = "mi2s0_sck"; >> + }; >> + >> + pri_mi2s_ws: pri-mi2s-ws { >> + pins = "gpio100"; >> + function = "mi2s0_ws"; >> + }; >> + >> + sec_mi2s_data0: sec-mi2s-data0 { >> + pins = "gpio107"; >> + function = "mi2s1_data0"; >> + }; >> + >> + sec_mi2s_sclk: sec-mi2s-sclk { >> + pins = "gpio106"; >> + function = "mi2s1_sck"; >> + }; >> + >> + sec_mi2s_ws: sec-mi2s-ws { >> + pins = "gpio108"; >> + function = "mi2s1_ws"; >> + }; > Please sort these nodes alphabetically on node name. Okay.
On 2/9/2022 2:42 AM, Stephen Boyd wrote: Thanks for your time Stephen!!! > Quoting Srinivasa Rao Mandadapu (2022-02-08 07:34:14) >> Add pinmux to reset wcd codec, conneceted on SC7280 based platforms. >> >> 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-idp.dtsi | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> index 4704a93..6b38fa1 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi >> @@ -594,6 +594,21 @@ >> */ >> bias-pull-up; >> }; >> + >> + wcd938x_reset_active: wcd938x_reset_active { > No underscore in node names. Okay. will remove it. > >> + pins = "gpio83"; >> + function = "gpio"; >> + drive-strength = <16>; >> + output-high; >> + }; >> + >> + wcd938x_reset_sleep: wcd938x_reset_sleep { >> + pins = "gpio83"; >> + function = "gpio"; >> + drive-strength = <16>; >> + bias-disable; >> + output-low; > Why doesn't the device drive the reset gpio by requesting the gpio and > asserting and deasserting it? We shouldn't need to use pinctrl settings > to toggle reset gpios. Okay. Verified without these nodes and didn't see any impact. But similar way it's mentioned in sm8250-mtp.dts. Could You please suggest on it how to go ahead on this?.
Quoting Srinivasa Rao Mandadapu (2022-02-09 06:26:58) > > On 2/9/2022 2:42 AM, Stephen Boyd wrote: > > Quoting Srinivasa Rao Mandadapu (2022-02-08 07:34:14) > > > >> + pins = "gpio83"; > >> + function = "gpio"; > >> + drive-strength = <16>; > >> + output-high; > >> + }; > >> + > >> + wcd938x_reset_sleep: wcd938x_reset_sleep { > >> + pins = "gpio83"; > >> + function = "gpio"; > >> + drive-strength = <16>; > >> + bias-disable; > >> + output-low; > > Why doesn't the device drive the reset gpio by requesting the gpio and > > asserting and deasserting it? We shouldn't need to use pinctrl settings > > to toggle reset gpios. > Okay. Verified without these nodes and didn't see any impact. But > similar way it's mentioned in sm8250-mtp.dts. Could You please suggest > on it how to go ahead on this?. I'd expect the wcd938x codec device node to have a 'reset-gpios' property like reset-gpios = <&tlmm 83 GPIO_ACTIVE_LOW> and then the driver to request that gpio via reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); so it gets the gpio during driver probe. Then the gpio can be deasserted during suspend and reasserted on resume, if that's even important?
On 2/10/2022 5:37 AM, Stephen Boyd wrote: Thanks for Your time Stephen!!! > Quoting Srinivasa Rao Mandadapu (2022-02-09 05:42:40) >> On 2/9/2022 2:38 AM, Stephen Boyd wrote: >>> Quoting Srinivasa Rao Mandadapu (2022-02-08 07:34:12) >>>> &qspi_cs0 { >>>> bias-disable; >>>> @@ -491,6 +524,13 @@ >>>> }; >>>> >>>> &tlmm { >>>> + amp_en: amp-en { >>>> + pins = "gpio63"; >>>> + function = "gpio"; >>>> + bias-disable; >>> Is there an external pull? >> I think no external pull. In trogdor mentioned bias-pull-down but you >> suggested to remove it. > Maybe on trogdor there was an external pull inside the amp that this pin > is connected to? Usually we have a comment like /* Has external > pull-{up,down} */ so please add that here depending on which way the > pull goes. As per Anderson suggestion we removed bias-pull-down. Actually, it's up-streamed for same platform in sc7280-herobrine.dtsi. We will fallow the same. It contains bias-pull-down.
On 2/10/2022 5:33 AM, Stephen Boyd wrote: Thanks for your time Stephen!!! > Quoting Srinivasa Rao Mandadapu (2022-02-09 06:26:58) >> On 2/9/2022 2:42 AM, Stephen Boyd wrote: >>> Quoting Srinivasa Rao Mandadapu (2022-02-08 07:34:14) >>> >>>> + pins = "gpio83"; >>>> + function = "gpio"; >>>> + drive-strength = <16>; >>>> + output-high; >>>> + }; >>>> + >>>> + wcd938x_reset_sleep: wcd938x_reset_sleep { >>>> + pins = "gpio83"; >>>> + function = "gpio"; >>>> + drive-strength = <16>; >>>> + bias-disable; >>>> + output-low; >>> Why doesn't the device drive the reset gpio by requesting the gpio and >>> asserting and deasserting it? We shouldn't need to use pinctrl settings >>> to toggle reset gpios. >> Okay. Verified without these nodes and didn't see any impact. But >> similar way it's mentioned in sm8250-mtp.dts. Could You please suggest >> on it how to go ahead on this?. > I'd expect the wcd938x codec device node to have a 'reset-gpios' > property like > > reset-gpios = <&tlmm 83 GPIO_ACTIVE_LOW> > > and then the driver to request that gpio via > > reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); > > so it gets the gpio during driver probe. Then the gpio can be deasserted > during suspend and reasserted on resume, if that's even important? Okay will remove it. Already wcd938x node has reset gpio. It seems these are redundant.