| Message ID | 1651079383-7665-5-git-send-email-quic_srivasam@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add lpass pin control support for audio on sc7280 based targets | expand |
On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote: > Add LPASS LPI pinctrl properties, which are required for Audio > functionality on herobrine based platforms of rev5+ > (aka CRD 3.0/3.1) boards. > > 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> I'm not super firm in pinctrl territory, a few maybe silly questions below. > arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++ > 1 file changed, 84 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > index deaea3a..dfc42df 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 { > * - If a pin is not hooked up on Qcard, it gets no name. > */ > > +&lpass_dmic01 { > + clk { > + drive-strength = <8>; > + }; > +}; > + > +&lpass_dmic01_sleep { > + clk { > + drive-strength = <2>; Does the drive strength really matter in the sleep state, is the SoC actively driving the pin? > + bias-disable; What should this be in active/default state? If I understand correctly after a transition from 'sleep' to 'default' this setting will remain, since the default config doesn't specify a setting for bias. > + }; > + > + data { > + pull-down; Same here, I think the pull-down will still be enabled after a switch from 'sleep' to 'default'. If I'm not mistaken then the rest of the pins also need to be reviewed. > + }; > +}; > + > +&lpass_dmic23 { > + clk { > + drive-strength = <8>; > + }; > +}; > + > +&lpass_dmic23_sleep { > + clk { > + drive-strength = <2>; > + bias-disable; > + }; > + > + data { > + pull-down; > + }; > +}; > + > +&lpass_rx_swr { > + clk { > + drive-strength = <2>; > + slew-rate = <1>; > + bias-disable; > + }; > + > + data { > + drive-strength = <2>; > + slew-rate = <1>; > + bias-bus-hold; > + }; > +}; > + > +&lpass_rx_swr_sleep { > + clk { > + drive-strength = <2>; > + bias-pull-down; > + }; > + > + data { > + drive-strength = <2>; > + bias-pull-down; > + }; > +}; > + > +&lpass_tx_swr { > + clk { > + drive-strength = <2>; > + slew-rate = <1>; > + bias-disable; > + }; > + > + data { > + slew-rate = <1>; > + bias-bus-hold; > + }; > +}; > + > +&lpass_tx_swr_sleep { > + clk { > + drive-strength = <2>; > + bias-pull-down; > + }; > + > + data { > + bias-bus-hold; > + }; > +}; > + > &mi2s1_data0 { > drive-strength = <6>; > bias-disable; > -- > 2.7.4 >
Hi, On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote: > > Add LPASS LPI pinctrl properties, which are required for Audio > > functionality on herobrine based platforms of rev5+ > > (aka CRD 3.0/3.1) boards. > > > > 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> > > I'm not super firm in pinctrl territory, a few maybe silly questions > below. > > > arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++ > > 1 file changed, 84 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > index deaea3a..dfc42df 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 { > > * - If a pin is not hooked up on Qcard, it gets no name. > > */ > > > > +&lpass_dmic01 { > > + clk { > > + drive-strength = <8>; > > + }; Ugh, I've been distracted and I hadn't realized we were back to the two-level syntax. Definitely not my favorite for all the reasons I talked about [1]. I guess you took Bjorn's silence to my response to mean that you should switch back to this way? :( Bjorn: can you clarify? [1] https://lore.kernel.org/r/CAD=FV=VicFiX6QkBksZs1KLwJ5x4eCte6j5RWOBPN+WwiXm2Cw@mail.gmail.com/ > > +}; > > + > > +&lpass_dmic01_sleep { > > + clk { > > + drive-strength = <2>; > > Does the drive strength really matter in the sleep state, is the SoC actively > driving the pin? My understanding is that if a pin is left as an output in sleep state that there is a slight benefit to switching it to drive-strength 2. > > + bias-disable; > > What should this be in active/default state? If I understand correctly > after a transition from 'sleep' to 'default' this setting will remain, > since the default config doesn't specify a setting for bias. Your understanding matches mine but I haven't tested it and I remember sometimes being surprised in this corner of pinmux before. I think it's better to put the bias in the default state if it should be that way all the time, or have a bias in both the default and sleep state if they need to be different.
On 4/29/2022 9:40 PM, Doug Anderson wrote: Thanks for your time Doug Anderson!!! > Hi, > > On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote: >> On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote: >>> Add LPASS LPI pinctrl properties, which are required for Audio >>> functionality on herobrine based platforms of rev5+ >>> (aka CRD 3.0/3.1) boards. >>> >>> 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> >> I'm not super firm in pinctrl territory, a few maybe silly questions >> below. >> >>> arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++ >>> 1 file changed, 84 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts >>> index deaea3a..dfc42df 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts >>> @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 { >>> * - If a pin is not hooked up on Qcard, it gets no name. >>> */ >>> >>> +&lpass_dmic01 { >>> + clk { >>> + drive-strength = <8>; >>> + }; > Ugh, I've been distracted and I hadn't realized we were back to the > two-level syntax. Definitely not my favorite for all the reasons I > talked about [1]. I guess you took Bjorn's silence to my response to > mean that you should switch back to this way? :( > > Bjorn: can you clarify? > > [1] https://lore.kernel.org/r/CAD=FV=VicFiX6QkBksZs1KLwJ5x4eCte6j5RWOBPN+WwiXm2Cw@mail.gmail.com/ Actually Your comment addressed for MI2S pin control nodes, but missed here. Will address same here. >>> +}; >>> + >>> +&lpass_dmic01_sleep { >>> + clk { >>> + drive-strength = <2>; >> Does the drive strength really matter in the sleep state, is the SoC actively >> driving the pin? > My understanding is that if a pin is left as an output in sleep state > that there is a slight benefit to switching it to drive-strength 2. Okay. Will keep this setting as it is. Please correct me if my understanding is wrong. > > >>> + bias-disable; >> What should this be in active/default state? If I understand correctly >> after a transition from 'sleep' to 'default' this setting will remain, >> since the default config doesn't specify a setting for bias. > Your understanding matches mine but I haven't tested it and I remember > sometimes being surprised in this corner of pinmux before. I think > it's better to put the bias in the default state if it should be that > way all the time, or have a bias in both the default and sleep state > if they need to be different. Okay. Will update accordingly.
On Fri 29 Apr 11:10 CDT 2022, Doug Anderson wrote: > Hi, > > On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote: > > > Add LPASS LPI pinctrl properties, which are required for Audio > > > functionality on herobrine based platforms of rev5+ > > > (aka CRD 3.0/3.1) boards. > > > > > > 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> > > > > I'm not super firm in pinctrl territory, a few maybe silly questions > > below. > > > > > arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++ > > > 1 file changed, 84 insertions(+) > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > index deaea3a..dfc42df 100644 > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 { > > > * - If a pin is not hooked up on Qcard, it gets no name. > > > */ > > > > > > +&lpass_dmic01 { > > > + clk { > > > + drive-strength = <8>; > > > + }; > > Ugh, I've been distracted and I hadn't realized we were back to the > two-level syntax. Definitely not my favorite for all the reasons I > talked about [1]. I guess you took Bjorn's silence to my response to > mean that you should switch back to this way? :( > > Bjorn: can you clarify? > I didn't think through the fact that &mi2s0_state was specified in the .dtsi and as such will be partially be overridden by the baord dts. I do prefer the two level style and describing full "states", but as you say whenever we provide something that will have to be overwritten it's suboptimal. As such, I think your flattened model is preferred in this case - but it makes me dislike the partial definition between the dtsi and dts even more (but I don't have any better suggestion). Regards, Bjorn > [1] https://lore.kernel.org/r/CAD=FV=VicFiX6QkBksZs1KLwJ5x4eCte6j5RWOBPN+WwiXm2Cw@mail.gmail.com/ > > > > +}; > > > + > > > +&lpass_dmic01_sleep { > > > + clk { > > > + drive-strength = <2>; > > > > Does the drive strength really matter in the sleep state, is the SoC actively > > driving the pin? > > My understanding is that if a pin is left as an output in sleep state > that there is a slight benefit to switching it to drive-strength 2. > > > > > + bias-disable; > > > > What should this be in active/default state? If I understand correctly > > after a transition from 'sleep' to 'default' this setting will remain, > > since the default config doesn't specify a setting for bias. > > Your understanding matches mine but I haven't tested it and I remember > sometimes being surprised in this corner of pinmux before. I think > it's better to put the bias in the default state if it should be that > way all the time, or have a bias in both the default and sleep state > if they need to be different.
Hi, On Wed, May 4, 2022 at 10:07 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Fri 29 Apr 11:10 CDT 2022, Doug Anderson wrote: > > > Hi, > > > > On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote: > > > > Add LPASS LPI pinctrl properties, which are required for Audio > > > > functionality on herobrine based platforms of rev5+ > > > > (aka CRD 3.0/3.1) boards. > > > > > > > > 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> > > > > > > I'm not super firm in pinctrl territory, a few maybe silly questions > > > below. > > > > > > > arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++ > > > > 1 file changed, 84 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > index deaea3a..dfc42df 100644 > > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 { > > > > * - If a pin is not hooked up on Qcard, it gets no name. > > > > */ > > > > > > > > +&lpass_dmic01 { > > > > + clk { > > > > + drive-strength = <8>; > > > > + }; > > > > Ugh, I've been distracted and I hadn't realized we were back to the > > two-level syntax. Definitely not my favorite for all the reasons I > > talked about [1]. I guess you took Bjorn's silence to my response to > > mean that you should switch back to this way? :( > > > > Bjorn: can you clarify? > > > > I didn't think through the fact that &mi2s0_state was specified in the > .dtsi and as such will be partially be overridden by the baord dts. > > > I do prefer the two level style and describing full "states", but as you > say whenever we provide something that will have to be overwritten it's > suboptimal. > > As such, I think your flattened model is preferred in this case How about for future patches we just provided labels at both levels (I'm not suggesting we churn this patch series more): lpass_dmic01_sleep: dmic01-sleep { lpass_dmic01_sleep_clk: clk { pins = "gpio6"; function = "dmic1_clk"; }; lpass_dmic01_sleep_data: data { pins = "gpio7"; function = "dmic1_data"; }; }; Then you can in your pinctrl reference you can just reference the top-level node but boards can override without having to replicate hierarchy... > but it > makes me dislike the partial definition between the dtsi and dts even > more (but I don't have any better suggestion). One other proposal I'd make is that maybe we should change the rules about never putting drive strength in the soc.dtsi file. While it should still be OK for boards to override the drive strength, it seems like a whole lot of biolerplate code to have every board override every pin and say that its drive strength is 2. Similarly, if there's a high speed interface (like eMMC) where a drive strength of 2 is nonsense for any board, it doesn't seem ridiculous to specify a default drive strength of something higher in the soc.dtsi file. I would like to say the same thing goes for for pulls, but it's unfortunately uglier for pulls. :( For instance, nearly everyone has an external pullup for i2c busses. The strength of the pullup needs to be tuned for the i2c bus speed and the impedance of the line. Thus, it would ideally make sense to specify this in the soc.dtsi file. Unfortunately, if we do that and some board _wants_ to use the internal pulls (maybe they're running at a really low speed and/or forgot to add external pulls) then they have to do an ugly "/delete-property/ bias-disable" because adding the "bias-pull-up" doesn't delete the other property and you end up with both. :( That seems bad, so I guess I'd vote to keep banning bias definitions in the soc.dtsi file. Anyway, I'd love your opinion on this. -Doug
On Thu, May 05, 2022 at 05:06:08PM -0700, Doug Anderson wrote: > Hi, > > On Wed, May 4, 2022 at 10:07 AM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Fri 29 Apr 11:10 CDT 2022, Doug Anderson wrote: > > > > > Hi, > > > > > > On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote: > > > > > Add LPASS LPI pinctrl properties, which are required for Audio > > > > > functionality on herobrine based platforms of rev5+ > > > > > (aka CRD 3.0/3.1) boards. > > > > > > > > > > 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> > > > > > > > > I'm not super firm in pinctrl territory, a few maybe silly questions > > > > below. > > > > > > > > > arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++ > > > > > 1 file changed, 84 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > > index deaea3a..dfc42df 100644 > > > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 { > > > > > * - If a pin is not hooked up on Qcard, it gets no name. > > > > > */ > > > > > > > > > > +&lpass_dmic01 { > > > > > + clk { > > > > > + drive-strength = <8>; > > > > > + }; > > > > > > Ugh, I've been distracted and I hadn't realized we were back to the > > > two-level syntax. Definitely not my favorite for all the reasons I > > > talked about [1]. I guess you took Bjorn's silence to my response to > > > mean that you should switch back to this way? :( > > > > > > Bjorn: can you clarify? > > > > > > > I didn't think through the fact that &mi2s0_state was specified in the > > .dtsi and as such will be partially be overridden by the baord dts. > > > > > > I do prefer the two level style and describing full "states", but as you > > say whenever we provide something that will have to be overwritten it's > > suboptimal. > > > > As such, I think your flattened model is preferred in this case > > How about for future patches we just provided labels at both levels > (I'm not suggesting we churn this patch series more): > > lpass_dmic01_sleep: dmic01-sleep { is the outer label ('lpass_dmic01_sleep') actually needed if we don't intend to replicate the hierarchy? > lpass_dmic01_sleep_clk: clk { > pins = "gpio6"; > function = "dmic1_clk"; > }; > > lpass_dmic01_sleep_data: data { > pins = "gpio7"; > function = "dmic1_data"; > }; > }; > > Then you can in your pinctrl reference you can just reference the > top-level node but boards can override without having to replicate > hierarchy... > > > but it > > makes me dislike the partial definition between the dtsi and dts even > > more (but I don't have any better suggestion). > > One other proposal I'd make is that maybe we should change the rules > about never putting drive strength in the soc.dtsi file. While it > should still be OK for boards to override the drive strength, it seems > like a whole lot of biolerplate code to have every board override > every pin and say that its drive strength is 2. Similarly, if there's > a high speed interface (like eMMC) where a drive strength of 2 is > nonsense for any board, it doesn't seem ridiculous to specify a > default drive strength of something higher in the soc.dtsi file. Indeed, that could make sense. > I would like to say the same thing goes for for pulls, but it's > unfortunately uglier for pulls. :( For instance, nearly everyone has > an external pullup for i2c busses. The strength of the pullup needs to > be tuned for the i2c bus speed and the impedance of the line. Thus, it > would ideally make sense to specify this in the soc.dtsi file. > Unfortunately, if we do that and some board _wants_ to use the > internal pulls (maybe they're running at a really low speed and/or > forgot to add external pulls) then they have to do an ugly > "/delete-property/ bias-disable" because adding the "bias-pull-up" > doesn't delete the other property and you end up with both. :( That > seems bad, so I guess I'd vote to keep banning bias definitions in the > soc.dtsi file. I agree, having to use 'delete-property' to change a pull setting doesn't seem a good idea.
On Thu 05 May 17:46 PDT 2022, Matthias Kaehlcke wrote: > On Thu, May 05, 2022 at 05:06:08PM -0700, Doug Anderson wrote: > > Hi, > > > > On Wed, May 4, 2022 at 10:07 AM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > On Fri 29 Apr 11:10 CDT 2022, Doug Anderson wrote: > > > > > > > Hi, > > > > > > > > On Thu, Apr 28, 2022 at 5:02 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > > > > > On Wed, Apr 27, 2022 at 10:39:43PM +0530, Srinivasa Rao Mandadapu wrote: > > > > > > Add LPASS LPI pinctrl properties, which are required for Audio > > > > > > functionality on herobrine based platforms of rev5+ > > > > > > (aka CRD 3.0/3.1) boards. > > > > > > > > > > > > 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> > > > > > > > > > > I'm not super firm in pinctrl territory, a few maybe silly questions > > > > > below. > > > > > > > > > > > arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++ > > > > > > 1 file changed, 84 insertions(+) > > > > > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > > > index deaea3a..dfc42df 100644 > > > > > > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > > > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts > > > > > > @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 { > > > > > > * - If a pin is not hooked up on Qcard, it gets no name. > > > > > > */ > > > > > > > > > > > > +&lpass_dmic01 { > > > > > > + clk { > > > > > > + drive-strength = <8>; > > > > > > + }; > > > > > > > > Ugh, I've been distracted and I hadn't realized we were back to the > > > > two-level syntax. Definitely not my favorite for all the reasons I > > > > talked about [1]. I guess you took Bjorn's silence to my response to > > > > mean that you should switch back to this way? :( > > > > > > > > Bjorn: can you clarify? > > > > > > > > > > I didn't think through the fact that &mi2s0_state was specified in the > > > .dtsi and as such will be partially be overridden by the baord dts. > > > > > > > > > I do prefer the two level style and describing full "states", but as you > > > say whenever we provide something that will have to be overwritten it's > > > suboptimal. > > > > > > As such, I think your flattened model is preferred in this case > > > > How about for future patches we just provided labels at both levels > > (I'm not suggesting we churn this patch series more): > > > > lpass_dmic01_sleep: dmic01-sleep { > > is the outer label ('lpass_dmic01_sleep') actually needed if we don't > intend to replicate the hierarchy? > Yes, that's what we put in the pinctrl-N reference from the device node. > > lpass_dmic01_sleep_clk: clk { > > pins = "gpio6"; > > function = "dmic1_clk"; > > }; > > > > lpass_dmic01_sleep_data: data { > > pins = "gpio7"; > > function = "dmic1_data"; > > }; > > }; > > I like this suggestion. > > Then you can in your pinctrl reference you can just reference the > > top-level node but boards can override without having to replicate > > hierarchy... > > > > > but it > > > makes me dislike the partial definition between the dtsi and dts even > > > more (but I don't have any better suggestion). > > > > One other proposal I'd make is that maybe we should change the rules > > about never putting drive strength in the soc.dtsi file. While it > > should still be OK for boards to override the drive strength, it seems > > like a whole lot of biolerplate code to have every board override > > every pin and say that its drive strength is 2. Similarly, if there's > > a high speed interface (like eMMC) where a drive strength of 2 is > > nonsense for any board, it doesn't seem ridiculous to specify a > > default drive strength of something higher in the soc.dtsi file. > > Indeed, that could make sense. > Sounds good to me. > > I would like to say the same thing goes for for pulls, but it's > > unfortunately uglier for pulls. :( For instance, nearly everyone has > > an external pullup for i2c busses. The strength of the pullup needs to > > be tuned for the i2c bus speed and the impedance of the line. Thus, it > > would ideally make sense to specify this in the soc.dtsi file. > > Unfortunately, if we do that and some board _wants_ to use the > > internal pulls (maybe they're running at a really low speed and/or > > forgot to add external pulls) then they have to do an ugly > > "/delete-property/ bias-disable" because adding the "bias-pull-up" > > doesn't delete the other property and you end up with both. :( That > > seems bad, so I guess I'd vote to keep banning bias definitions in the > > soc.dtsi file. > > I agree, having to use 'delete-property' to change a pull setting > doesn't seem a good idea. Same. Regards, Bjorn
diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts index deaea3a..dfc42df 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts @@ -111,6 +111,90 @@ ap_ts_pen_1v8: &i2c13 { * - If a pin is not hooked up on Qcard, it gets no name. */ +&lpass_dmic01 { + clk { + drive-strength = <8>; + }; +}; + +&lpass_dmic01_sleep { + clk { + drive-strength = <2>; + bias-disable; + }; + + data { + pull-down; + }; +}; + +&lpass_dmic23 { + clk { + drive-strength = <8>; + }; +}; + +&lpass_dmic23_sleep { + clk { + drive-strength = <2>; + bias-disable; + }; + + data { + pull-down; + }; +}; + +&lpass_rx_swr { + clk { + drive-strength = <2>; + slew-rate = <1>; + bias-disable; + }; + + data { + drive-strength = <2>; + slew-rate = <1>; + bias-bus-hold; + }; +}; + +&lpass_rx_swr_sleep { + clk { + drive-strength = <2>; + bias-pull-down; + }; + + data { + drive-strength = <2>; + bias-pull-down; + }; +}; + +&lpass_tx_swr { + clk { + drive-strength = <2>; + slew-rate = <1>; + bias-disable; + }; + + data { + slew-rate = <1>; + bias-bus-hold; + }; +}; + +&lpass_tx_swr_sleep { + clk { + drive-strength = <2>; + bias-pull-down; + }; + + data { + bias-bus-hold; + }; +}; + &mi2s1_data0 { drive-strength = <6>; bias-disable;