Re: [PATCH 1/2] arm64: dts: qcom: sm8250: add pinctrl for SPI using GPIO as a CS

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 08 Feb 09:58 CST 2021, Doug Anderson wrote:

> Hi,
> 
> On Fri, Feb 5, 2021 at 8:48 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Fri 05 Feb 09:00 CST 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Thu, Feb 4, 2021 at 4:25 PM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > >
> > > > > > > +                             mux {
> > > > > >
> > > > > > Rather than splitting the properties in {mux, cs, config} I think it
> > > > > > makes more sense to split them in {spi, cs} or something like that.
> > > > >
> > > > > In general pinconf doesn't belong in the SoC dts file.  If there's no
> > > > > reason to change it seems like this should match what sc7180 did.
> > > > >
> > > >
> > > > Right, but I still would prefer the pinctrl state to be split by
> > > > function/pins, rather than pinmux vs pinconf. That way it's natural to
> > > > add pinconf properties to the various functional parts (i.e. bias or
> > > > drive-strength for the spi pins vs cs).
> > > >
> > > > Do you have any concerns with this?
> > >
> > > I read this a few times and I'm not exactly sure what you're
> > > proposing.  Can you provide an example of what you want it to look
> > > like in this case?
> > >
> >
> > Today in most cases we group pinctrl properties by being a "conf" of a
> > "mux" property, so we end up with:
> >
> > the_state: spi-state {
> >         all-the-mux-properties {
> >                 pins = "gpio40", gpio41", "gpio42", "gpio43";
> >                 function = qup14";
> >         };
> >
> >         repeat-pins-and-add-all-conf-properties {
> >                 pins = "gpio40", gpio41", "gpio42", "gpio43";
> >                 drive-strength = <6>;
> >                 bias-disable;
> >         };
> > };
> >
> > This made sense to me after implementing the driver, there's muxing to
> > be done and there's electrical configuration to configure.
> >
> > But what's actually trying to describe is a hardware state; i.e. that
> > miso, mosi, clk and cs should be acting in a particular fashion.
> >
> > In particular this lends itself useful when the hardware state consists
> > of different functions, a good example being the Bluetooth UART, or in
> > the SPI-with-separate-GPIO:
> >
> > the_state: spi-state {
> >         miso-mosi-clk {
> >                 pins = "gpio40", gpio41", "gpio42"
> >                 function = qup14";
> >                 drive-strength = <6>;
> >                 bias-disable;
> >         };
> >
> >         cs {
> >                 pins = "gpio43";
> >                 function = "gpio";
> >                 drive-strength = <6>;
> >                 bias-disable;
> >         };
> > };
> >
> >
> > For the case of uniform configuration across the state we've come to
> > sprinkle a few different synonyms for "pinconf" and "pinmux" in the
> > state nodes. But a few years ago someone updated the state parser to
> > handle cases either directly in the state or in subnodes. So we can
> > avoid these boilerplate nodes with a simple:
> >
> > the_state: spi-state {
> >         pins = "gpio40", gpio41", "gpio42", "gpio43";
> >         function = qup14";
> >         drive-strength = <6>;
> >         bias-disable;
> > };
> 
> OK, this makes sense to me.  I always felt like the extra "pinconf" /
> "pinmux" made things awkward.

I'm happy to hear that :)

> I guess someone should try to convert some SoC dtsi fully over so we
> can see how it looks?

Sounds good. I feel fairly confident, so let's pick SM8250 and aim to
land this patch in the "new" form.

> In this case I think you'd have something like this, right?
> 
> SoC dtsi:
> 
> tlmm: pinctrl@... {
>   qup_spi0_data_clk: qup-spi0-data-clk {
>     pins = "gpio28", "gpio29", "gpio30";
>     function = "qup0";
>   };
> 
>   qup_spi0_cs: qup-spi0-cs {
>     pins = "gpio31";
>     function = "qup0";
>   };
> 
>   qup_spi0_cs_gpio: qup-spi0-cs-gpio {
>     pins = "gpio31";
>     function = "gpio";
>   };
> };
> 
> Board dts:
> 
> &spi0 {
>   pinctrl-0 = <&qup_spi0_data_clk>, &<qup_spi0_cs_gpio>;
> };
> 
> &qup_spi0_data_clk {
>   drive-strength = <6>;
>   bias-disable;
> };
> 
> &qup_spi0_cs_gpio {
>   drive-strength = <6>;
>   bias-disable;
> };

Correct. And providing a common state for the 3 non-cs pins and using
the pinctrl-0 to select which kind of cs we want looks even better.

Regards,
Bjorn