Am 30.06.2011 02:13 schrieb Stefan Tauner:
On Wed, 29 Jun 2011 15:06:00 -0700 David Hendricks dhendrix@google.com wrote:
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...
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 < 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 :)
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