On Wed, 29 Jun 2011 15:06:00 -0700 David Hendricks dhendrix@google.com wrote:
A couple quick comments in-line below...
On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
- static void ich_read_data(uint8_t *data, uint8_t len, int reg0_off)
- {
int a;uint32_t temp32 = 0;if (len > spi_programmer->max_data_read)len = spi_programmer->max_data_read;for (a = 0; a < len; a++) {if ((a % 4) == 0)temp32 = REGREAD32(reg0_off + (a));data[a] = (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))>> ((a % 4) * 8);How about "data[a] = (temp32 >> ((a % 4) * 8)) & 0xff" instead? That is clearer IMHO, and you won't even need to break the line. (80 columns really *is* enough :-P)
i just copied the existing code from swseq and did not change it at all on purpose. improvements are welcome... the code certainly is not very readable. i wonder if this is due to some compiler optimization friendly style... but changes should be in their on commit though anyway and so we can postpone it.
On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
+static uint8_t ich_fill_data(const uint8_t *data, uint8_t len, int reg0_off)
As mentioned on IRC, the caller tends to pass in a "len" value > 255, e.g. 1 block of 4096 bytes or more. Since len is handled as an int in the caller, it should probably get passed in as an int here.
inderdaad. i would have expected the compiler to warn in such cases. obviously i am too used to my usual embedded makefile with ~20 -W terms :)
please see 9/10 and 10/10 for my proposed fixes of this (and other problems like the possibility of setting SME )