diff mbox series

[v12,4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi pinmux properties for CRD 3.0/3.1

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

Commit Message

Srinivasa Rao Mandadapu April 27, 2022, 5:09 p.m. UTC
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>
---
 arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts | 84 +++++++++++++++++++++++
 1 file changed, 84 insertions(+)

Comments

Matthias Kaehlcke April 29, 2022, 12:02 a.m. UTC | #1
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
>
Doug Anderson April 29, 2022, 4:10 p.m. UTC | #2
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.
Srinivasa Rao Mandadapu May 2, 2022, 1:43 p.m. UTC | #3
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.
Bjorn Andersson May 4, 2022, 5:07 p.m. UTC | #4
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.
Doug Anderson May 6, 2022, 12:06 a.m. UTC | #5
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
Matthias Kaehlcke May 6, 2022, 12:46 a.m. UTC | #6
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.
Bjorn Andersson May 6, 2022, 3:06 a.m. UTC | #7
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 mbox series

Patch

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;