| Message ID | 1437716236-12715-1-git-send-email-larper@axis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 24, 2015 at 07:37:16AM +0200, Lars Persson wrote: > This reverts commit 232a5adc5199 ("spi: bitbang: only toggle > bitchanges") because it breaks bitbanged SPI on our MIPS system. I > found two problems with the patch: > - oldbit must initially be computed from bit position 7, 15 or 31 of > word depending on the value of bits. This might be a real issue but fixing it does not require a revert. > - The optimization also does not eliminate consecutive ones because > the compare of 1<<31 and 1 will be false (a bool only takes values 0 > or 1). No, any non-zero value is true in C. I assume you're talking about this section of the diff: > - if ((flags & SPI_MASTER_NO_TX) == 0) { > - if ((word & (1 << 31)) != oldbit) { > - setmosi(spi, word & (1 << 31)); > - oldbit = word & (1 << 31); which looks fine - we see if the top bit is the same as the last top bit we wrote, if it isn't we update the value output and record what we just wrote.
On Fri, 2015-07-24 at 12:44 +0200, Mark Brown wrote: > On Fri, Jul 24, 2015 at 07:37:16AM +0200, Lars Persson wrote: > > This reverts commit 232a5adc5199 ("spi: bitbang: only toggle > > bitchanges") because it breaks bitbanged SPI on our MIPS system. I > > found two problems with the patch: > > - oldbit must initially be computed from bit position 7, 15 or 31 of > > word depending on the value of bits. > > This might be a real issue but fixing it does not require a revert. > > > - The optimization also does not eliminate consecutive ones because > > the compare of 1<<31 and 1 will be false (a bool only takes values 0 > > or 1). > > No, any non-zero value is true in C. I assume you're talking about > this section of the diff: > > > - if ((flags & SPI_MASTER_NO_TX) == 0) { > > - if ((word & (1 << 31)) != oldbit) { > > - setmosi(spi, word & (1 << 31)); > > - oldbit = word & (1 << 31); > > which looks fine - we see if the top bit is the same as the last top bit > we wrote, if it isn't we update the value output and record what we just > wrote. No this is a misunderstanding about the semantics of the bool type. When assigning to the bool any non-zero value becomes 1. When comparing an integer against the bool, the bool is promoted to an integer. If we previously transmitted a one, oldbit equals 1. If current bit is a one, then (word & (1 << 31)) equals 1<<31. ((1<<31) != 1) is TRUE so we call setmosi again, hence no optimization for consecutive ones. Please do not mix bool and bitwise operations. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 24, 2015 at 12:44 PM, Mark Brown <broonie@kernel.org> wrote: > On Fri, Jul 24, 2015 at 07:37:16AM +0200, Lars Persson wrote: >> This reverts commit 232a5adc5199 ("spi: bitbang: only toggle >> bitchanges") because it breaks bitbanged SPI on our MIPS system. I >> found two problems with the patch: >> - oldbit must initially be computed from bit position 7, 15 or 31 of >> word depending on the value of bits. > > This might be a real issue but fixing it does not require a revert. > >> - The optimization also does not eliminate consecutive ones because >> the compare of 1<<31 and 1 will be false (a bool only takes values 0 >> or 1). > > No, any non-zero value is true in C. I assume you're talking about > this section of the diff: While any value will be treated as true, the inverse of a value (!) can only ever be 0 or 1, so >> - bool oldbit = !(word & 1); makes oldbit to be either 0 or 1, and here >> - if ((word & (1 << 31)) != oldbit) { we compare the bool with an unsigned integer, thus the bool gets promoted to unsigned integer as well, and we compare 0 or 1 with 0 or (1 << 31), so if oldbit is 1 this condition will always evaluate to true. Feel free to verify with a compiler of your choice ;-) The fix for this is to use !!(word & (1 << 31)). Jonas -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/24/2015 01:48 PM, Lars Persson wrote: > On Fri, 2015-07-24 at 12:44 +0200, Mark Brown wrote: >> On Fri, Jul 24, 2015 at 07:37:16AM +0200, Lars Persson wrote: >>> This reverts commit 232a5adc5199 ("spi: bitbang: only toggle >>> bitchanges") because it breaks bitbanged SPI on our MIPS system. I >>> found two problems with the patch: >>> - oldbit must initially be computed from bit position 7, 15 or 31 of >>> word depending on the value of bits. >> >> This might be a real issue but fixing it does not require a revert. >> >>> - The optimization also does not eliminate consecutive ones because >>> the compare of 1<<31 and 1 will be false (a bool only takes values 0 >>> or 1). >> >> No, any non-zero value is true in C. I assume you're talking about >> this section of the diff: >> >>> - if ((flags & SPI_MASTER_NO_TX) == 0) { >>> - if ((word & (1 << 31)) != oldbit) { >>> - setmosi(spi, word & (1 << 31)); >>> - oldbit = word & (1 << 31); >> >> which looks fine - we see if the top bit is the same as the last top bit >> we wrote, if it isn't we update the value output and record what we just >> wrote. > > No this is a misunderstanding about the semantics of the bool type. > When assigning to the bool any non-zero value becomes 1. > When comparing an integer against the bool, the bool is promoted to an > integer. From the patch context alone it is not clear that oldbit is of type bool. I think this is where the confusion came from. By just looking at the path fragment itself you'd think that oldbit is a unsigned int given its usage pattern. But it becomes obvious that the code wont work if you know oldbit is a bool. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 24, 2015 at 07:37:16AM +0200, Lars Persson wrote: > This reverts commit 232a5adc5199 ("spi: bitbang: only toggle > bitchanges") because it breaks bitbanged SPI on our MIPS system. I > found two problems with the patch: > - oldbit must initially be computed from bit position 7, 15 or 31 of > word depending on the value of bits. > - The optimization also does not eliminate consecutive ones because > the compare of 1<<31 and 1 will be false (a bool only takes values 0 > or 1). > > Signed-off-by: Lars Persson <larper@axis.com> > --- > drivers/spi/spi-bitbang-txrx.h | 18 ++++-------------- > 1 file changed, 4 insertions(+), 14 deletions(-) > > diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h > index 06b34e5..c616e41 100644 > --- a/drivers/spi/spi-bitbang-txrx.h > +++ b/drivers/spi/spi-bitbang-txrx.h > @@ -49,17 +49,12 @@ bitbang_txrx_be_cpha0(struct spi_device *spi, > { > /* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */ > > - bool oldbit = !(word & 1); > /* clock starts at inactive polarity */ > for (word <<= (32 - bits); likely(bits); bits--) { > > /* setup MSB (to slave) on trailing edge */ > - if ((flags & SPI_MASTER_NO_TX) == 0) { > - if ((word & (1 << 31)) != oldbit) { This needs to become: if (!!(word & (1 << 31)) != oldbit) { so oldbit really get compared by bool and not by int. > - setmosi(spi, word & (1 << 31)); > - oldbit = word & (1 << 31); > - } > - } > + if ((flags & SPI_MASTER_NO_TX) == 0) > + setmosi(spi, word & (1 << 31)); > spidelay(nsecs); /* T(setup) */ > > setsck(spi, !cpol); > @@ -81,18 +76,13 @@ bitbang_txrx_be_cpha1(struct spi_device *spi, > { > /* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */ > > - bool oldbit = !(word & (1 << 31)); > /* clock starts at inactive polarity */ > for (word <<= (32 - bits); likely(bits); bits--) { > > /* setup MSB (to slave) on leading edge */ > setsck(spi, !cpol); > - if ((flags & SPI_MASTER_NO_TX) == 0) { > - if ((word & (1 << 31)) != oldbit) { and this needs to become: if (!!(word & (1 << 31)) != oldbit) { > - setmosi(spi, word & (1 << 31)); > - oldbit = word & (1 << 31); > - } > - } > + if ((flags & SPI_MASTER_NO_TX) == 0) > + setmosi(spi, word & (1 << 31)); > spidelay(nsecs); /* T(setup) */ > > setsck(spi, cpol); Sorry for the issue, Michael
On Fri, Jul 24, 2015 at 01:55:30PM +0200, Lars-Peter Clausen wrote: > On 07/24/2015 01:48 PM, Lars Persson wrote: > >No this is a misunderstanding about the semantics of the bool type. > >When assigning to the bool any non-zero value becomes 1. > >When comparing an integer against the bool, the bool is promoted to an > >integer. > From the patch context alone it is not clear that oldbit is of type bool. I > think this is where the confusion came from. By just looking at the path > fragment itself you'd think that oldbit is a unsigned int given its usage > pattern. But it becomes obvious that the code wont work if you know oldbit > is a bool. ...and obviously the clear intention is that it should be an unsigned integer so the appropriate thing to do is to fix that.
diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h index 06b34e5..c616e41 100644 --- a/drivers/spi/spi-bitbang-txrx.h +++ b/drivers/spi/spi-bitbang-txrx.h @@ -49,17 +49,12 @@ bitbang_txrx_be_cpha0(struct spi_device *spi, { /* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */ - bool oldbit = !(word & 1); /* clock starts at inactive polarity */ for (word <<= (32 - bits); likely(bits); bits--) { /* setup MSB (to slave) on trailing edge */ - if ((flags & SPI_MASTER_NO_TX) == 0) { - if ((word & (1 << 31)) != oldbit) { - setmosi(spi, word & (1 << 31)); - oldbit = word & (1 << 31); - } - } + if ((flags & SPI_MASTER_NO_TX) == 0) + setmosi(spi, word & (1 << 31)); spidelay(nsecs); /* T(setup) */ setsck(spi, !cpol); @@ -81,18 +76,13 @@ bitbang_txrx_be_cpha1(struct spi_device *spi, { /* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */ - bool oldbit = !(word & (1 << 31)); /* clock starts at inactive polarity */ for (word <<= (32 - bits); likely(bits); bits--) { /* setup MSB (to slave) on leading edge */ setsck(spi, !cpol); - if ((flags & SPI_MASTER_NO_TX) == 0) { - if ((word & (1 << 31)) != oldbit) { - setmosi(spi, word & (1 << 31)); - oldbit = word & (1 << 31); - } - } + if ((flags & SPI_MASTER_NO_TX) == 0) + setmosi(spi, word & (1 << 31)); spidelay(nsecs); /* T(setup) */ setsck(spi, cpol);
This reverts commit 232a5adc5199 ("spi: bitbang: only toggle bitchanges") because it breaks bitbanged SPI on our MIPS system. I found two problems with the patch: - oldbit must initially be computed from bit position 7, 15 or 31 of word depending on the value of bits. - The optimization also does not eliminate consecutive ones because the compare of 1<<31 and 1 will be false (a bool only takes values 0 or 1). Signed-off-by: Lars Persson <larper@axis.com> --- drivers/spi/spi-bitbang-txrx.h | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-)