[1/3] spi: bitbang: fix shift for getmosi
diff mbox

Message ID 1394637636-29042-2-git-send-email-m.grzeschik@pengutronix.de
State New, archived
Headers show

Commit Message

Michael Grzeschik March 12, 2014, 3:20 p.m. UTC
The driver needs to shift the word bit after reading the mosi bit.
Otherwise the return word will have an Off-by-one bit value.

Cc: <stable@vger.kernel.org>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/spi/spi-bitbang-txrx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Gerhard Sittig March 12, 2014, 9:23 p.m. UTC | #1
On Wed, Mar 12, 2014 at 16:20 +0100, Michael Grzeschik wrote:
> 
> The driver needs to shift the word bit after reading the mosi bit.
> Otherwise the return word will have an Off-by-one bit value.

The MISO gets read (is incoming), MOSI is outgoing.

> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Is the Cc: to stable@ appropriate without Fixes: or a version
that's supposed to be affected?  Has a bug been verified and are
you certain that the fix is correct?

> ---
>  drivers/spi/spi-bitbang-txrx.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
> index c616e41..b6e348d 100644
> --- a/drivers/spi/spi-bitbang-txrx.h
> +++ b/drivers/spi/spi-bitbang-txrx.h
> @@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
>  		spidelay(nsecs);
>  
>  		/* sample MSB (from slave) on leading edge */
> -		word <<= 1;
>  		if ((flags & SPI_MASTER_NO_RX) == 0)
>  			word |= getmiso(spi);
>  		setsck(spi, cpol);
> +		word <<= 1;
>  	}
>  	return word;
>  }

This seems wrong.  You might have observed an off by one error.
But this code change is suspicious.

Shifting first and ORing then makes the sample end up in the
lowest bit.  That's expected.

ORing the sample and shifting afterwards actually would be
creating an off by one error. :-O


virtually yours
Gerhard Sittig
Mark Brown March 12, 2014, 9:37 p.m. UTC | #2
On Wed, Mar 12, 2014 at 10:23:34PM +0100, Gerhard Sittig wrote:
> On Wed, Mar 12, 2014 at 16:20 +0100, Michael Grzeschik wrote:

> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

> Is the Cc: to stable@ appropriate without Fixes: or a version
> that's supposed to be affected?  Has a bug been verified and are
> you certain that the fix is correct?

Like I said in my reply I'm somewhat dubious that this is actually an
issue but if it is then it's been present since at least 2.6.35 (when
the header file was split out of the driver).

Patch
diff mbox

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index c616e41..b6e348d 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -61,10 +61,10 @@  bitbang_txrx_be_cpha0(struct spi_device *spi,
 		spidelay(nsecs);
 
 		/* sample MSB (from slave) on leading edge */
-		word <<= 1;
 		if ((flags & SPI_MASTER_NO_RX) == 0)
 			word |= getmiso(spi);
 		setsck(spi, cpol);
+		word <<= 1;
 	}
 	return word;
 }
@@ -89,9 +89,9 @@  bitbang_txrx_be_cpha1(struct spi_device *spi,
 		spidelay(nsecs);
 
 		/* sample MSB (from slave) on trailing edge */
-		word <<= 1;
 		if ((flags & SPI_MASTER_NO_RX) == 0)
 			word |= getmiso(spi);
+		word <<= 1;
 	}
 	return word;
 }