| Message ID | 1394639617-26917-2-git-send-email-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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; }
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(-)