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

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

[Robin Murphy]

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.

Fixes: 1847e3046c52 ("spi: gpio: Implement LSB First bitbang support")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 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

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

[Mark Brown]

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

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...

> Fixes: 1847e3046c52 ("spi: gpio: Implement LSB First bitbang support")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  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
> 

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

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@arm.com>
>> ---
>>   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
>>

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

[Mark Brown]

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

On Wed, Aug 03, 2022 at 04:22:13PM +0100, Robin Murphy wrote:
> On 2022-08-03 16:02, Mark Brown wrote:

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

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

Unless it happens to get corrupted into what is actually wanted - it
wouldn't be the first time I've seen driver code bodge around some issue
in the core.  Doesn't seem super likely here given the nature of the bug
but it's alway worth checking.

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

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

[Mark Brown]

On Wed, 3 Aug 2022 15:58:57 +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.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: bitbang: Fix lsb-first Rx
      commit: 46f7ac3d7892e808c9ba01c39da6bb85cda26ecd

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark