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

[Mark Brown]

On Wed, Nov 11, 2020 at 02:05:19AM +0100, Linus Walleij wrote:
> On Mon, Nov 9, 2020 at 3:41 PM Sven Van Asbroeck <thesven73@xxxxxxxxx> wrote:

> > I don't disagree. Fact is that after the imx cspi bus driver was converted
> > to gpio descriptors, most spi client drivers broke. It would be great if this
> > could be fixed. Any method that the community can find a consensus on,
> > would be great :)

> I think your patch is the quick fix.

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

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

Attachment: signature.asc
Description: PGP signature