Skip to content

Commit

Permalink
spi: fix client driver breakages when using GPIO descriptors
Browse files Browse the repository at this point in the history
Commit f3186dd ("spi: Optionally use GPIO descriptors for CS GPIOs")
introduced the optional use of GPIO descriptors for chip selects.

A side-effect of this change: when a SPI bus uses GPIO descriptors,
all its client devices have SPI_CS_HIGH set in spi->mode. This flag is
required for the SPI bus to operate correctly.

This unfortunately breaks many client drivers, which use the following
pattern to configure their underlying SPI bus:

static int client_device_probe(struct spi_device *spi)
{
	...
	spi->mode = SPI_MODE_0;
	spi->bits_per_word = 8;
	err = spi_setup(spi);
	..
}

In short, many client drivers overwrite the SPI_CS_HIGH bit in
spi->mode, and break the underlying SPI bus driver.

This is especially true for Freescale/NXP imx ecspi, where large
numbers of spi client drivers now no longer work.

Proposed fix:
-------------
When using gpio descriptors, depend on gpiolib to handle CS polarity.
Existing quirks in gpiolib ensure that this is handled correctly.

Existing gpiolib behaviour will force the polarity of any chip-select
gpiod to active-high (if 'spi-active-high' devicetree prop present) or
active-low (if 'spi-active-high' absent). Irrespective of whether
the gpio is marked GPIO_ACTIVE_[HIGH|LOW] in the devicetree.

Loose ends:
-----------
If this fix is applied:
- is commit 138c9c3
  ("spi: spidev: Fix CS polarity if GPIO descriptors are used")
  still necessary / correct ?

Fixes: f3186dd ("spi: Optionally use GPIO descriptors for CS GPIOs")
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Link: https://lore.kernel.org/r/20201106150706.29089-1-TheSven73@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
  • Loading branch information
Sven Van Asbroeck authored and Mark Brown committed Nov 11, 2020
1 parent bc7f2cd commit 766c6b6
Showing 1 changed file with 6 additions and 17 deletions.
23 changes: 6 additions & 17 deletions drivers/spi/spi.c
Original file line number Diff line number Diff line change
Expand Up @@ -812,18 +812,16 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
enable = !enable;

if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) {
/*
* Honour the SPI_NO_CS flag and invert the enable line, as
* active low is default for SPI. Execution paths that handle
* polarity inversion in gpiolib (such as device tree) will
* enforce active high using the SPI_CS_HIGH resulting in a
* double inversion through the code above.
*/
if (!(spi->mode & SPI_NO_CS)) {
if (spi->cs_gpiod)
/* polarity handled by gpiolib */
gpiod_set_value_cansleep(spi->cs_gpiod,
!enable);
enable1);
else
/*
* invert the enable line, as active low is
* default for SPI.
*/
gpio_set_value_cansleep(spi->cs_gpio, !enable);
}
/* Some SPI masters need both GPIO CS & slave_select */
Expand Down Expand Up @@ -1992,15 +1990,6 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
}
spi->chip_select = value;

/*
* For descriptors associated with the device, polarity inversion is
* handled in the gpiolib, so all gpio chip selects are "active high"
* in the logical sense, the gpiolib will invert the line if need be.
*/
if ((ctlr->use_gpio_descriptors) && ctlr->cs_gpiods &&
ctlr->cs_gpiods[spi->chip_select])
spi->mode |= SPI_CS_HIGH;

/* Device speed */
if (!of_property_read_u32(nc, "spi-max-frequency", &value))
spi->max_speed_hz = value;
Expand Down

0 comments on commit 766c6b6

Please sign in to comment.