Re: [PATCH v1] spi: fix client driver breakages when using GPIO descriptors

[Linus Walleij]

On Wed, Nov 11, 2020 at 1:33 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Wed, Nov 11, 2020 at 02:05:19AM +0100, Linus Walleij wrote:

> > I would say that anything that has:
>
> > spi->mode = ...
>
> > is essentially broken.
>
> This is not clear to me, most of these settings are things that are
> constant for the device so it's not clear that they should be being set
> by the device tree in the first place.

This was added initially with some two properties
in drivers/of/of_spi.c in 2008:
commit 284b01897340974000bcc84de87a4e1becc8a83d
"spi: Add OF binding support for SPI busses"

This was around the time ARM was first starting to migrate
to device tree, so I suppose it made sense to them/us back
then.

Some properties were the accumulated over time.

commit d57a4282d04810417c4ed2a49cbbeda8b3569b18
"spi/devicetree: Move devicetree support code into spi directory"
made this part of the SPI subsystem.

This seems as simple as nobody was there to push back and
say "wait the devices can specify that with code, don't put it
as properties in device tree". To be honest we have kind of
moved back and forward on that topic over time. :/

> The idea that the chip select
> might be being inverted like it is by this whole gpiolib/DT/new binding
> thing is breaking expectations too.

OK I think you're right, then this patch probably brings the behaviour
back to expectations and it's how I should have done it in the first
place. My bad code :/
Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

> > The core sets up vital things in .mode from e.g. device tree in
> > of_spi_parse_dt():
>
> >         /* Mode (clock phase/polarity/etc.) */
> >         if (of_property_read_bool(nc, "spi-cpha"))
> >                 spi->mode |= SPI_CPHA;
> >         if (of_property_read_bool(nc, "spi-cpol"))
> >                 spi->mode |= SPI_CPOL;
> >         if (of_property_read_bool(nc, "spi-3wire"))
> >                 spi->mode |= SPI_3WIRE;
> >         if (of_property_read_bool(nc, "spi-lsb-first"))
> >                 spi->mode |= SPI_LSB_FIRST;
> >         if (of_property_read_bool(nc, "spi-cs-high"))
> >                 spi->mode |= SPI_CS_HIGH;
>
> > All this gets overwritten and ignored when a client just assigns mode
> > like that. Not just SPI_CS_HIGH. I doubt things are different
> > with ACPI.
>
> OTOH most of these are things the device driver should just get right
> without needing any input from DT, there's a few where there's plausible
> options (eg, you can imagine pin strap configuration for 3 wire mode)

Yes I actually ran into a case where the same Samsung display support
both 4 and 3-wire mode so that needs to be configured in the device
tree depending on the layout of the electronics. Arguably we should have
just standardized the device tree bindings and let the individual SPI
drivers parse that themselves in such cases.

> so generally it's not clear how many of these make sense for anything
> other than spidev.  This binding all predates my involvement so I don't
> know the thought process here.

I dug out some details, let's see if Grant has some historical anecdotes
to add. The usage document from back then doesn't really say what
device properties should be encoded in the device tree and what
should just be assigned by code and e.g. determined from the
compatible-string. It was later that especially Rob pointed out that
random properties on device nodes was overused and that simply
knowing the compatible is often enough.

I don't know if we ever formalized it, there is nowadays a rule akin to

"if a property can be determined from the compatible-string, and if the
 compatible-string is identifying the variant of the electronic component,
 then do not add this property to the device tree description. Just
 deduce it from the compatible-string, assign it with code to the device
 model of the operating system and handle it inside the operating system."

I think this, while clear and intuitive, wasn't at all clear and intuitive in
the recent past.

Yours,
Linus Walleij