Messages in this thread | | | From | Linus Walleij <> | Date | Wed, 11 Nov 2020 02:05:19 +0100 | Subject | Re: [PATCH v1] spi: fix client driver breakages when using GPIO descriptors |
| |
On Mon, Nov 9, 2020 at 3:41 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > On Mon, Nov 9, 2020 at 9:24 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > Sounds like "many SPI drivers have to be fixed". > > 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.
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.
> One the one hand: the fact that many spi client drivers just overwrite > flags and values in their parent bus structure, doesn't sound idiomatic. > I guess those spi->... values should really be opaque, and we should > be using accessor functions, eg.: > > static int acme_probe(struct spi_device *spi) > { > ... > // won't touch SPI_CS_HIGH flag > spi_set_mode_clock(spi, SPI_MODE_0); > ... > }
I would just make sure to affect the flags that matters to my driver, it's just bits.
spi->mode &= ~FOO; spi->mode |= BAR;
> On the other hand, it sounds very confusing to set SPI_CS_HIGH on > all spi buses that use gpio descriptors: especially because gpiolib > already handles absolutely everything related to polarity.
As long as gpiolib gets a 1 for asserted and a 0 for deasserted it will be happy.
I'm not against your patch, it makes the codepath cleaner so in a way it is good.
Yours, Linus Walleij
| |