Wolfgang Denk wrote:
Dear Matthias,
in message 200711071529.49762.matthias.fuchs@esd-electronics.com you wrote:
when copying val from 'data[bytecount++]' it is common practice that val if of the same type as the array elements. So val should be unsigned char.
Maybe. But what's the difference?
I changed the sign check into a 'val & 0x80', which I think is fine an clean.
If it is indeed intended to be a test for a negative number, then this is neither nice nor clean.
Best regards,
Wolfgang Denk
Hi Wolfgang,
You are missing the point of signed vs. unsigned because you got sucked into the wrong argument. The problematic code is:
512 val = data [bytecount ++]; 513 i = 8; 514 do { 515 /* Deassert the clock */ 516 (*fn->clk) (FALSE, TRUE, cookie); 517 CONFIG_FPGA_DELAY (); 518 /* Write data */ 519 (*fn->wr) ((val < 0), TRUE, cookie); <-------- BAD 520 CONFIG_FPGA_DELAY (); 521 /* Assert the clock */ 522 (*fn->clk) (TRUE, TRUE, cookie); 523 CONFIG_FPGA_DELAY (); 524 val <<= 1; 525 i --; 526 } while (i > 0);
As you can see, the code is looking at the MSB of the 8 bit value to see if it must program a '1' or a '0' and it is shifting the byte left by 1 bit each time.
The problem is that it is using a *signed character test* (val < 0) where it *should be* using an bit ANDing operation to isolate the MSBit of the 8 bit "char" (and I would leave the char as a char, since it is immaterial whether it is signed or unsigned *if the correct test were used*). This is what Angelos recommended: http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/33116
Obviously, I agree with him and recommend that the (val < 0) be changed to (val & 0x80) rather than the original fix (which also works, but is fixing the problem in the wrong way IMHO).
Best regards, gvb