The SPI configuration state includes an SPI_NO_CS flag that disables
all CS line manipulation, for applications that want to manage their
own chip selects. However, this flag is ignored by the GPIO CS code
in the SPI framework.
Correct this omission with a trivial patch.
Signed-off-by: Phil Elwell <[email protected]>
---
drivers/spi/spi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 84dfef4..b1d88fe 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -729,7 +729,9 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
enable = !enable;
if (gpio_is_valid(spi->cs_gpio)) {
- gpio_set_value(spi->cs_gpio, !enable);
+ /* Honour the SPI_NO_CS flag */
+ if (!(spi->mode & SPI_NO_CS))
+ gpio_set_value(spi->cs_gpio, !enable);
/* Some SPI masters need both GPIO CS & slave_select */
if ((spi->controller->flags & SPI_MASTER_GPIO_SS) &&
spi->controller->set_cs)
--
2.7.4
The patch
spi: Make GPIO CSs honour the SPI_NO_CS flag
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 25972d0c33deb5c49708baa48e7796c8d0348e98 Mon Sep 17 00:00:00 2001
From: Phil Elwell <[email protected]>
Date: Fri, 12 Oct 2018 10:23:18 +0100
Subject: [PATCH] spi: Make GPIO CSs honour the SPI_NO_CS flag
The SPI configuration state includes an SPI_NO_CS flag that disables
all CS line manipulation, for applications that want to manage their
own chip selects. However, this flag is ignored by the GPIO CS code
in the SPI framework.
Correct this omission with a trivial patch.
Signed-off-by: Phil Elwell <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index fcbd4cfd2818..ea3e5e146764 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -775,7 +775,9 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
enable = !enable;
if (gpio_is_valid(spi->cs_gpio)) {
- gpio_set_value(spi->cs_gpio, !enable);
+ /* Honour the SPI_NO_CS flag */
+ if (!(spi->mode & SPI_NO_CS))
+ gpio_set_value(spi->cs_gpio, !enable);
/* Some SPI masters need both GPIO CS & slave_select */
if ((spi->controller->flags & SPI_MASTER_GPIO_SS) &&
spi->controller->set_cs)
--
2.19.0
On Fri, 2018-10-12 at 10:23 +0100, Phil Elwell wrote:
> The SPI configuration state includes an SPI_NO_CS flag that disables
> all CS line manipulation, for applications that want to manage their
> own chip selects. However, this flag is ignored by the GPIO CS code
> in the SPI framework.
> @@ -729,7 +729,9 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
> enable = !enable;
>
> if (gpio_is_valid(spi->cs_gpio)) {
> - gpio_set_value(spi->cs_gpio, !enable);
> + /* Honour the SPI_NO_CS flag */
> + if (!(spi->mode & SPI_NO_CS))
> + gpio_set_value(spi->cs_gpio, !enable);
> /* Some SPI masters need both GPIO CS & slave_select */
> if ((spi->controller->flags & SPI_MASTER_GPIO_SS) &&
> spi->controller->set_cs)
What about the calls to spi->controller->set_cs() after this? Should a
driver provided set_cs method be responsible for checking SPI_NO_CS?
Or should it not be called in the first place?
I imagine it depends on what set_cs needs to do, which might not be
solely related to changing the CS line.
On Mon, Oct 15, 2018 at 06:34:18PM +0000, Trent Piepho wrote:
> What about the calls to spi->controller->set_cs() after this? Should a
> driver provided set_cs method be responsible for checking SPI_NO_CS?
> Or should it not be called in the first place?
This seems like something that should be done entirely in the framework,
no point in every single driver having to open code the same thing.
> I imagine it depends on what set_cs needs to do, which might not be
> solely related to changing the CS line.
It should be. If something is hanging other work on set_cs() then it's
going to break.
On Tue, 2018-10-16 at 10:03 +0100, Mark Brown wrote:
> On Mon, Oct 15, 2018 at 06:34:18PM +0000, Trent Piepho wrote:
>
> > What about the calls to spi->controller->set_cs() after this? Should a
> > driver provided set_cs method be responsible for checking SPI_NO_CS?
> > Or should it not be called in the first place?
>
> This seems like something that should be done entirely in the framework,
> no point in every single driver having to open code the same thing.
>
> > I imagine it depends on what set_cs needs to do, which might not be
> > solely related to changing the CS line.
>
> It should be. If something is hanging other work on set_cs() then it's
> going to break.
IIRC, for spi-dw setting CS is the only way to trigger the master to do
anything. I think orion is the same way. Even if you don't want a CS
line the driver still needs to assert one. Which CS to use as the
dummy CS is a challenge that has come up before.
bcm2835_spi_set_cs() does check SPI_NO_CS, but it still does a lot of
other stuff even if that is set, likely because of the above issue.
On Tue, Oct 16, 2018 at 07:29:21PM +0000, Trent Piepho wrote:
> On Tue, 2018-10-16 at 10:03 +0100, Mark Brown wrote:
> > On Mon, Oct 15, 2018 at 06:34:18PM +0000, Trent Piepho wrote:
> > > I imagine it depends on what set_cs needs to do, which might not be
> > > solely related to changing the CS line.
> > It should be. If something is hanging other work on set_cs() then it's
> > going to break.
> IIRC, for spi-dw setting CS is the only way to trigger the master to do
> anything. I think orion is the same way. Even if you don't want a CS
> line the driver still needs to assert one. Which CS to use as the
> dummy CS is a challenge that has come up before.
> bcm2835_spi_set_cs() does check SPI_NO_CS, but it still does a lot of
> other stuff even if that is set, likely because of the above issue.
For hardware that's that broken I'd recommend deferring the actual CS
setting operation to the transfer operation instead; that way we can
guarantee it happens for any pattern of access to the chip select (or
error out if we can't represent it properly, though obviously a lot of
such systems use a GPIO for the chip select to work around the broken
hardware).