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 guess someone should try to convert
some SoC dtsi fully over so we can see how it looks? 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;
};
