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

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

Commit Message

Michael Grzeschik March 12, 2014, 3:53 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

Mark Brown March 12, 2014, 4:24 p.m. UTC | #1
On Wed, Mar 12, 2014 at 04:53:35PM +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.

This isn't exactly new code...  do we understand why nobody has noticed
this before?

> @@ -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;
>  	}

Just looking at the context here it's not obvious to me that this is
helping - it means that the last bit we read is going to be shifted
which seems wrong, we ought to be reading into LSB.
Gerhard Sittig March 12, 2014, 9:45 p.m. UTC | #2
On Wed, Mar 12, 2014 at 16:24 +0000, Mark Brown wrote:
> 
> On Wed, Mar 12, 2014 at 04:53:35PM +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.
> 
> This isn't exactly new code...  do we understand why nobody has noticed
> this before?

I too suspect that the bug (if there is any) is elsewhere.

> > @@ -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;
> >  	}
> 
> Just looking at the context here it's not obvious to me that this is
> helping - it means that the last bit we read is going to be shifted
> which seems wrong, we ought to be reading into LSB.

It might be more robust to 

  if (!(flags & SPI_MASTER_NO_RX) && getmiso(spi))
    word |= 1;
    
instead.  This decouples the construction of the received bits
buffer from whatever the getmiso() implementation might look
like.  That's just a thought after the recent GPIO discussion
about whether 1/0 is given or should not be assumed (and I still
suspect that "!!x" need not result in exactly 1/0 values).

And I agree with Mark that the "late shift" is most probably
wrong.  Just noticed the resend too late and responded in the
other thread.


virtually yours
Gerhard Sittig

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;
 }