Am 30.06.2011 02:13 schrieb Stefan Tauner: > On Wed, 29 Jun 2011 15:06:00 -0700 > David Hendricks <[email protected]> wrote: > >> On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner >> <[email protected]> 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... >
We don't optimize for compilers. The ICH SPI code was donated to flashrom, and I'm happy we got it. The authors already rewrote most of it (compared to the initial submission) before I merged it, and back then the flashrom source code was not exactly a thing of beauty. Since then, quite a few cleanups have happened, but people tended to leave ICH SPI alone because the code was a bit more complex than the rest of flashrom. ICH SPI has quite a few refactoring opportunities: There are various helper functions which could be used more than once... > but changes should be in their on commit though anyway and so we can > postpone it. > Yes. However, using the better style for the new code would make it easy to just copy it over to the old code in a later commit. >> On Mon, Jun 27, 2011 at 8:33 PM, Stefan Tauner < >> [email protected]> 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 :) > Side note: Do we want a release mode Makefile switch which activates all warnings under the sun? > please see 9/10 and 10/10 for my proposed fixes of this (and other > problems like the possibility of setting SME ) > Oh well, I should probably have looked at those first. Regards, Carl-Daniel -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
