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

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

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;
};

Regards,
Bjorn