From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerry Van Baren Date: Wed, 07 Nov 2007 16:07:29 -0500 Subject: [U-Boot-Users] Spartan FPGA patch In-Reply-To: <20071107194809.D7CE4248D3@gemini.denx.de> References: <20071107194809.D7CE4248D3@gemini.denx.de> Message-ID: <47322911.5050902@ge.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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: 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