Date:   Thu, 5 May 2022 20:06:39 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Doug Anderson <dianders@...omium.org>,
        Srinivasa Rao Mandadapu <quic_srivasam@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
        quic_rohkumar@...cinc.com,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Judy Hsiao <judyhsiao@...omium.org>,
        Venkata Prasad Potturu <quic_potturu@...cinc.com>
Subject: Re: [PATCH v12 4/4] arm64: dts: qcom: sc7280-herobrine: Add lpi
 pinmux properties for CRD 3.0/3.1

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@...aro.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@...omium.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@...cinc.com>
> > > > > > Co-developed-by: Venkata Prasad Potturu <quic_potturu@...cinc.com>
> > > > > > Signed-off-by: Venkata Prasad Potturu <quic_potturu@...cinc.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

Powered by blists - more mailing lists