[PATCH 09/10] fixup! ichspi.c: refactor filling and reading the fdata/spid registers (was Re: [PATCH 5/8] ichspi.c: refactor filling and reading the fdata/spid registers)
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 )
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
On Fri, 01 Jul 2011 00:00:58 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
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:
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...
working on it if you have something in mind specifically, just drop it off in my corner. ;)
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.
parser exception. the current patch does move the fill/read loops used equivalently in ich7_run_opcode and ich9_run_opcode into two functions which are called instead. much easier for review and testing. it can then be optimized/de-obfuscated later.
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?
i would call it a developing makefile and would love it. the only danger of such a thing is that you begin to rely on it and stop thinking for yourself - i am all for it therefore! :)
please see 9/10 and 10/10 for my proposed fixes of this (and other problems like the possibility of setting SME )
Am 30.06.2011 02:13 schrieb Stefan Tauner:
From: Stefan Tauner stefan.tauner@student.tuwien.ac.at Date: Thu, 30 Jun 2011 02:02:32 +0200 Subject: [PATCH 09/10] fixup! ichspi.c: refactor filling and reading the fdata/spid registers
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Note to self: Need to review that patch
Am 01.07.2011 00:03 schrieb Carl-Daniel Hailfinger:
Am 30.06.2011 02:13 schrieb Stefan Tauner:
From: Stefan Tauner stefan.tauner@student.tuwien.ac.at Date: Thu, 30 Jun 2011 02:02:32 +0200 Subject: [PATCH 09/10] fixup! ichspi.c: refactor filling and reading the fdata/spid registers
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Once you have committed the acked patches, can you repost the rest of this series with all the squash patches merged into their base patches? Thanks.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Stefan Tauner