spi: Make GPIO CSs honour the SPI_NO_CS flag
diff mbox series

Message ID 1539336198-84179-1-git-send-email-phil@raspberrypi.org
State New
Headers show
Series
  • spi: Make GPIO CSs honour the SPI_NO_CS flag
Related show

Commit Message

Phil Elwell Oct. 12, 2018, 9:23 a.m. UTC
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 <phil@raspberrypi.org>
---
 drivers/spi/spi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Trent Piepho Oct. 15, 2018, 6:34 p.m. UTC | #1
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.
Mark Brown Oct. 16, 2018, 9:03 a.m. UTC | #2
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.
Trent Piepho Oct. 16, 2018, 7:29 p.m. UTC | #3
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.
Mark Brown Oct. 17, 2018, 9:42 a.m. UTC | #4
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).

Patch
diff mbox series

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)