[PATCH] spi: Fix incorrect cs_setup delay handling

[PATCH] spi: Fix incorrect cs_setup delay handling

[Hector Martin]

We need to wait *after* asserting CS and before the transfer, not before
asserting CS which isn't very useful.

Fixes: 25093bdeb6bc ("spi: implement SW control for CS times")
Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/spi/spi.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index b23e675953e1..cfb708d928b5 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -947,12 +947,9 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
 	spi->controller->last_cs_enable = enable;
 	spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH;
 
-	if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio) ||
-	    !spi->controller->set_cs_timing) {
-		if (activate)
-			spi_delay_exec(&spi->cs_setup, NULL);
-		else
-			spi_delay_exec(&spi->cs_hold, NULL);
+	if ((spi->cs_gpiod || gpio_is_valid(spi->cs_gpio) ||
+	    !spi->controller->set_cs_timing) && !activate) {
+		spi_delay_exec(&spi->cs_hold, NULL);
 	}
 
 	if (spi->mode & SPI_CS_HIGH)
@@ -994,7 +991,9 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
 
 	if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio) ||
 	    !spi->controller->set_cs_timing) {
-		if (!activate)
+		if (activate)
+			spi_delay_exec(&spi->cs_setup, NULL);
+		else
 			spi_delay_exec(&spi->cs_inactive, NULL);
 	}
 }
-- 
2.33.0

Re: [PATCH] spi: Fix incorrect cs_setup delay handling

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

On Sat, Dec 11, 2021 at 12:36:34AM +0900, Hector Martin wrote:

> We need to wait *after* asserting CS and before the transfer, not before
> asserting CS which isn't very useful.

This needs a better changelog, there's multiple delays being handled
here and it's not clear from this which are affected here or what the
problem is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: Fix incorrect cs_setup delay handling

[Hector Martin]

On 11/12/2021 01.21, Mark Brown wrote:
> On Sat, Dec 11, 2021 at 12:36:34AM +0900, Hector Martin wrote:
> 
>> We need to wait *after* asserting CS and before the transfer, not before
>> asserting CS which isn't very useful.
> 
> This needs a better changelog, there's multiple delays being handled
> here and it's not clear from this which are affected here or what the
> problem is.

cs_setup is affected, I thought at least that was clear from the patch 
summary :-)

 From spi.h:

* @cs_setup: delay to be introduced by the controller after CS is asserted

So clearly that delay has to happen at the end of spi_set_cs, *after* 
the assertion, not at the beginning before it.

If you prefer, I can resend it with a reference to the spi.h comment.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH] spi: Fix incorrect cs_setup delay handling

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Sat, Dec 11, 2021 at 01:32:06AM +0900, Hector Martin wrote:
> On 11/12/2021 01.21, Mark Brown wrote:

> > This needs a better changelog, there's multiple delays being handled
> > here and it's not clear from this which are affected here or what the
> > problem is.

> cs_setup is affected, I thought at least that was clear from the patch
> summary :-)

This should be in the commit message, the subject line of the e-mail
isn't super visible when people are reviewing - basically, the body of
the commit message should make sense standalone.

> If you prefer, I can resend it with a reference to the spi.h comment.

Yes, please resend with a clear commit message.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]