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.
> 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
