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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
