Re: [PATCH] Revert "spi: bitbang: only toggle bitchanges"

[Lars Persson <lars.persson@xxxxxxxx>]

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