On Mon, 8 Feb 2021 at 21:04, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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.
OK. As a starting bit I will convert SPI pinctrl for now with the rest
of pins to follow.
>
> > 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
--
With best wishes
Dmitry
