| Message ID | 20221207230853.6174-1-kris@embeddedTS.com |
|---|---|
| State | Accepted |
| Commit | 3a6f994f848a69deb2bf3cd9d130dd0c09730e55 |
| Headers | show |
| Series | spi: spi-gpio: Don't set MOSI as an input if not 3WIRE mode | expand |
On Wed, Dec 07, 2022 at 03:08:53PM -0800, Kris Bahnsen wrote: > The addition of 3WIRE support would affect MOSI direction even > when still in standard (4 wire) mode. This can lead to MOSI being > at an invalid logic level when a device driver sets an SPI > message with a NULL tx_buf. > > spi.h states that if tx_buf is NULL then "zeros will be shifted > out ... " If MOSI is tristated then the data shifted out is subject > to pull resistors, keepers, or in the absence of those, noise. > > This issue came to light when using spi-gpio connected to an > ADS7843 touchscreen controller. MOSI pulled high when clocking > MISO data in caused the SPI device to interpret this as a command > which would put the device in an unexpected and non-functional > state. A cleaner fix which is probably marginally more performant would be to make the setting of spi_gpio_set_direction() conditional on SPI_3WIRE - then we won't call into the function at all when not doing 3 wire, avoiding the issue entirely. > As an aside, I wasn't sure how to best put down the Fixes: tags. > 4b859db2c606 ("spi: spi-gpio: add SPI_3WIRE support") introduced the > actual bug, but 5132b3d28371 ("spi: gpio: Support 3WIRE high-impedance turn-around") > modified that commit slightly and is what this patch actually applies > to. Let me know if marking both as fixes is incorrect and I can > create another patch. That's fine, it doesn't really matter either way.
On Wed, Dec 07, 2022 at 04:36:41PM -0800, Kris Bahnsen wrote: > On Wed, 2022-12-07 at 23:44 +0000, Mark Brown wrote: > > A cleaner fix which is probably marginally more performant would be to > > make the setting of spi_gpio_set_direction() conditional on SPI_3WIRE - > > then we won't call into the function at all when not doing 3 wire, > > avoiding the issue entirely. > That makes sense to me. I was operating under the assumption that 3WIRE > mode could be switched in to at a later time via ioctl(), but with the > death of spidev that is presumably no longer a concern. Ugh, right, spidev. Really even with spidev devices should probably have the mode configured beforehand (I'm not sure pinmux will do the right thing on most platforms...) but now I check it's part of the ABI so we can't get rid of it and therefore your current patch probably is what we need. No need to reroll, sorry for the noise :/ I'm not sure why you think spidev is dying, it does still exist and devices use it?
diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c index 4b12c4964a66..9c8c7948044e 100644 --- a/drivers/spi/spi-gpio.c +++ b/drivers/spi/spi-gpio.c @@ -268,9 +268,19 @@ static int spi_gpio_set_direction(struct spi_device *spi, bool output) if (output) return gpiod_direction_output(spi_gpio->mosi, 1); - ret = gpiod_direction_input(spi_gpio->mosi); - if (ret) - return ret; + /* + * Only change MOSI to an input if using 3WIRE mode. + * Otherwise, MOSI could be left floating if there is + * no pull resistor connected to the I/O pin, or could + * be left logic high if there is a pull-up. Transmitting + * logic high when only clocking MISO data in can put some + * SPI devices in to a bad state. + */ + if (spi->mode & SPI_3WIRE) { + ret = gpiod_direction_input(spi_gpio->mosi); + if (ret) + return ret; + } /* * Send a turnaround high impedance cycle when switching * from output to input. Theoretically there should be
The addition of 3WIRE support would affect MOSI direction even when still in standard (4 wire) mode. This can lead to MOSI being at an invalid logic level when a device driver sets an SPI message with a NULL tx_buf. spi.h states that if tx_buf is NULL then "zeros will be shifted out ... " If MOSI is tristated then the data shifted out is subject to pull resistors, keepers, or in the absence of those, noise. This issue came to light when using spi-gpio connected to an ADS7843 touchscreen controller. MOSI pulled high when clocking MISO data in caused the SPI device to interpret this as a command which would put the device in an unexpected and non-functional state. Fixes: 4b859db2c606 ("spi: spi-gpio: add SPI_3WIRE support") Fixes: 5132b3d28371 ("spi: gpio: Support 3WIRE high-impedance turn-around") Signed-off-by: Kris Bahnsen <kris@embeddedTS.com> --- As an aside, I wasn't sure how to best put down the Fixes: tags. 4b859db2c606 ("spi: spi-gpio: add SPI_3WIRE support") introduced the actual bug, but 5132b3d28371 ("spi: gpio: Support 3WIRE high-impedance turn-around") modified that commit slightly and is what this patch actually applies to. Let me know if marking both as fixes is incorrect and I can create another patch. drivers/spi/spi-gpio.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)