Re: [PATCH] spi: bitbang: Fix lsb-first Rx

[Robin Murphy]

On 2022-08-03 16:02, Mark Brown wrote:
> On Wed, Aug 03, 2022 at 03:58:57PM +0100, Robin Murphy wrote:
> > Shifting the recieved bit by "bits" inserts it at the top of the
> > *currently remaining* Tx data, so we end up accumulating the whole
> > transfer into bit 0 of the output word. Oops.
> >
> > For the algorithm to work as intended, we need to remember where the
> > top of the *original* word was, and shift Rx to there.
>
> So if this never worked we presumably have some systems out there which
> somehow rely on the old behaviour that we need to fix somehow - copying
> in everyone from the original change...

Hmm, I can't imagine anyone's relying too critically on data transfer 
corruption :/

Apparently these TM16x8 chips are the only thing so far to even want 
lsb-first bitbang support at all, so I strongly suspect I'm the first 
person to try the receive side of it (since Rockchip's SPI hardware 
doesn't seem to support 3-wire half-duplex). It actually took me quite a 
while to figure out that this wasn't my bug in trying to hook up the 
matrix-keymap stuff, since half the bytes I was expecting to get back do 
only have bit 0 set anyway.

Cheers,
Robin.

> > Fixes: 1847e3046c52 ("spi: gpio: Implement LSB First bitbang support")
> > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > ---
> >   drivers/spi/spi-bitbang-txrx.h | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
> > index 267342dfa738..2dcbe166df63 100644
> > --- a/drivers/spi/spi-bitbang-txrx.h
> > +++ b/drivers/spi/spi-bitbang-txrx.h
> > @@ -116,6 +116,7 @@ bitbang_txrx_le_cpha0(struct spi_device *spi,
> >   {
> >   	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
> >   
> > +	u8 rxbit = bits - 1;
> >   	u32 oldbit = !(word & 1);
> >   	/* clock starts at inactive polarity */
> >   	for (; likely(bits); bits--) {
> > @@ -135,7 +136,7 @@ bitbang_txrx_le_cpha0(struct spi_device *spi,
> >   		/* sample LSB (from slave) on leading edge */
> >   		word >>= 1;
> >   		if ((flags & SPI_MASTER_NO_RX) == 0)
> > -			word |= getmiso(spi) << (bits - 1);
> > +			word |= getmiso(spi) << rxbit;
> >   		setsck(spi, cpol);
> >   	}
> >   	return word;
> > @@ -148,6 +149,7 @@ bitbang_txrx_le_cpha1(struct spi_device *spi,
> >   {
> >   	/* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */
> >   
> > +	u8 rxbit = bits - 1;
> >   	u32 oldbit = !(word & 1);
> >   	/* clock starts at inactive polarity */
> >   	for (; likely(bits); bits--) {
> > @@ -168,7 +170,7 @@ bitbang_txrx_le_cpha1(struct spi_device *spi,
> >   		/* sample LSB (from slave) on trailing edge */
> >   		word >>= 1;
> >   		if ((flags & SPI_MASTER_NO_RX) == 0)
> > -			word |= getmiso(spi) << (bits - 1);
> > +			word |= getmiso(spi) << rxbit;
> >   	}
> >   	return word;
> >   }
> > --
> > 2.36.1.dirty