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

[Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>]

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