| Message ID | fca3ba7cdc930cd36854666ceac4fbcf01b89028.1582027457.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 138c9c32f090894614899eca15e0bb7279f59865 |
| Headers | show |
| Series | spi: spidev: Fix CS polarity if GPIO descriptors are used | expand |
On Tue, Feb 18, 2020 at 1:08 PM Lukas Wunner <lukas@wunner.de> wrote: > Commit f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs") > amended of_spi_parse_dt() to always set SPI_CS_HIGH for SPI slaves whose > Chip Select is defined by a "cs-gpios" devicetree property. > > This change broke userspace applications which issue an SPI_IOC_WR_MODE > ioctl() to an spidev: Chip Select polarity will be incorrect unless the > application is changed to set SPI_CS_HIGH. And once changed, it will be > incompatible with kernels not containing the commit. > > Fix by setting SPI_CS_HIGH in spidev_ioctl() (under the same conditions > as in of_spi_parse_dt()). > > Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs") > Reported-by: Simon Han <z.han@kunbus.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: stable@vger.kernel.org # v5.1+ > Cc: Linus Walleij <linus.walleij@linaro.org> (...) Nit: I would also insert a comment in the code to tell what is going on. > + if (ctlr->use_gpio_descriptors && ctlr->cs_gpiods && > + ctlr->cs_gpiods[spi->chip_select]) > + tmp |= SPI_CS_HIGH; Should this be tmp ^= SPI_CS_HIGH? If the device tree node for cs-gpios is actually active high, which happens, then you probably want the opposite of what was requested, right? Yours, Linus Walleij
On Wed, Feb 19, 2020 at 04:47:50PM +0100, Linus Walleij wrote: > On Tue, Feb 18, 2020 at 1:08 PM Lukas Wunner <lukas@wunner.de> wrote: > > Commit f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs") > > amended of_spi_parse_dt() to always set SPI_CS_HIGH for SPI slaves whose > > Chip Select is defined by a "cs-gpios" devicetree property. > > > > This change broke userspace applications which issue an SPI_IOC_WR_MODE > > ioctl() to an spidev: Chip Select polarity will be incorrect unless the > > application is changed to set SPI_CS_HIGH. And once changed, it will be > > incompatible with kernels not containing the commit. > > > > Fix by setting SPI_CS_HIGH in spidev_ioctl() (under the same conditions > > as in of_spi_parse_dt()). > > Nit: I would also insert a comment in the code to tell what is going on. Fair enough, but the below should be clarified before I respin: > > + if (ctlr->use_gpio_descriptors && ctlr->cs_gpiods && > > + ctlr->cs_gpiods[spi->chip_select]) > > + tmp |= SPI_CS_HIGH; > > Should this be tmp ^= SPI_CS_HIGH? > > If the device tree node for cs-gpios is actually active high, which > happens, then you probably want the opposite of what was > requested, right? I don't quite follow. Using an XOR here would seem to be inconsistent with what you added to of_spi_parse_dt(): In that function, you *always* set SPI_CS_HIGH if gpio_descs are used. So if the polarity is specified in the cs-gpios property, anything else is considered irrelevant and ignored. Applying the same logic to spidev_ioctl() means that if the user specified SPI_CS_HIGH, it is likewise ignored because the polarity in the cs-gpios property takes precedence. Am I missing something? TBH the way commit f3186dd87669 abuses SPI_CS_HIGH seems clumsy to me. Would it not have been possible to just amend spi_set_cs() like this: - if (spi->mode & SPI_CS_HIGH) + if (spi->mode & SPI_CS_HIGH && !(ctlr->use_gpio_descriptors && + ctlr->cs_gpiods && + ctlr->cs_gpiods[spi->chip_select])) enable = !enable; This would have avoided the regression fixed by my patch. Also note that spi_setup() emits a dev_dbg() which now incorrectly reports "cs_high" if a cs-gpios property is present. :-( Thanks, Lukas
On Thu, Feb 20, 2020 at 07:11:22AM +0100, Lukas Wunner wrote: > - if (spi->mode & SPI_CS_HIGH) > + if (spi->mode & SPI_CS_HIGH && !(ctlr->use_gpio_descriptors && > + ctlr->cs_gpiods && > + ctlr->cs_gpiods[spi->chip_select])) > enable = !enable; Sorry, I meant (spi->mode & SPI_CS_HIGH || (ctlr->...) ^^^^
On Thu, Feb 20, 2020 at 7:11 AM Lukas Wunner <lukas@wunner.de> wrote: > On Wed, Feb 19, 2020 at 04:47:50PM +0100, Linus Walleij wrote: > > > + if (ctlr->use_gpio_descriptors && ctlr->cs_gpiods && > > > + ctlr->cs_gpiods[spi->chip_select]) > > > + tmp |= SPI_CS_HIGH; > > > > Should this be tmp ^= SPI_CS_HIGH? > > > > If the device tree node for cs-gpios is actually active high, which > > happens, then you probably want the opposite of what was > > requested, right? > > I don't quite follow. Using an XOR here would seem to be inconsistent > with what you added to of_spi_parse_dt(): In that function, you > *always* set SPI_CS_HIGH if gpio_descs are used. So if the polarity > is specified in the cs-gpios property, anything else is considered > irrelevant and ignored. It's not just cs-gpios though: if you look in drivers/gpio/gpiolib-of.c in the function of_gpio_flags_quirks() you see that the bool property spi-cs-high in the consumer node will take precedence over the cs-gpios property and that will also be handled transparently by gpiolib. (commit 6953c57ab1721 with fixes on top). But I guess that is what you mean. Yes as far as I can see this solves the problem of double-specifying (both in device tree and from userspace) that we want CS high, so after some thinking: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > TBH the way commit f3186dd87669 abuses SPI_CS_HIGH seems clumsy to me. Clumsy programmer, that's me, yeah sometimes :/ > Would it not have been possible to just amend spi_set_cs() like this: > > - if (spi->mode & SPI_CS_HIGH) > + if (spi->mode & SPI_CS_HIGH || (ctlr->use_gpio_descriptors && > + ctlr->cs_gpiods && > + ctlr->cs_gpiods[spi->chip_select])) > enable = !enable; > > This would have avoided the regression fixed by my patch. There is way too much double inversion going on for sure (some of it dating back before any attempts to use gpiolib for polarity handling and I just feel partially responsible) but I just haven't figured out how to properly fix the problem. Maybe it was just stupid of me to try to move polarity inversion semantics over to gpiolib before fixing that though. Patches like the above are also welcome! Yours, Linus Walleij
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 1e217e3e9486..2ab6e782f14c 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -396,6 +396,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) else retval = get_user(tmp, (u32 __user *)arg); if (retval == 0) { + struct spi_controller *ctlr = spi->controller; u32 save = spi->mode; if (tmp & ~SPI_MODE_MASK) { @@ -403,6 +404,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) break; } + if (ctlr->use_gpio_descriptors && ctlr->cs_gpiods && + ctlr->cs_gpiods[spi->chip_select]) + tmp |= SPI_CS_HIGH; + tmp |= spi->mode & ~SPI_MODE_MASK; spi->mode = (u16)tmp; retval = spi_setup(spi);
Commit f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs") amended of_spi_parse_dt() to always set SPI_CS_HIGH for SPI slaves whose Chip Select is defined by a "cs-gpios" devicetree property. This change broke userspace applications which issue an SPI_IOC_WR_MODE ioctl() to an spidev: Chip Select polarity will be incorrect unless the application is changed to set SPI_CS_HIGH. And once changed, it will be incompatible with kernels not containing the commit. Fix by setting SPI_CS_HIGH in spidev_ioctl() (under the same conditions as in of_spi_parse_dt()). Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs") Reported-by: Simon Han <z.han@kunbus.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: stable@vger.kernel.org # v5.1+ Cc: Linus Walleij <linus.walleij@linaro.org> --- drivers/spi/spidev.c | 5 +++++ 1 file changed, 5 insertions(+)