Am 01.07.2011 05:38 schrieb Stefan Tauner:
- add ich_fill_data to fill the chipset registers from an array
- add ich_read_data to copy the data from the chipset register into an array
- replace the existing code with calls to those functions
NB: this changes the semantic of the *run_opcode functions a bit: previously they could trash the chipset registers following the data registers because there was no (explicit) length check (it was and is checked by the dispatching run_opcode method).
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
I know you're just moving code, but this is a chance to clean up the code and improve readability.
diff --git a/ichspi.c b/ichspi.c index 19e52d2..905ed70 100644 --- a/ichspi.c +++ b/ichspi.c @@ -592,6 +592,57 @@ static void ich_set_bbar(uint32_t min_addr) msg_perr("Setting BBAR failed!\n"); }
+/* Reads up to len byte from the fdata/spid register into the data array.
- The amount actually read is limited by the maximum read size of the
- chipset. */
Can you put the terminating */ on its own line like the multiline comments in other files?
- static void ich_read_data(uint8_t *data, int len, int reg0_off)
- {
- int a;
int i would be a nicer name... although you just moved that code, so it's not your fault.
- uint32_t temp32 = 0;
- if (len > spi_programmer->max_data_read)
len = spi_programmer->max_data_read;
Can this really happen? If yes, does it depend on bugs elsewhere in the code? This needs at least a msg_perr("bug in blabla, please report to flashrom@"), we might even want to have the function return int to handle errors.
- for (a = 0; a < len; a++) {
if ((a % 4) == 0)temp32 = REGREAD32(reg0_off + (a));
Are the extra parentheses around a really needed?
data[a] = (temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))>> ((a % 4) * 8);
I think David had a suggestion on how to simplify this formula a bit.
- }
+}
+/* Fills up to len bytes from the data array into the fdata/spid registers.
- The amount actually written is limited by the maximum write size of the
- chipset and is returned by the function. */
The result is discarded anyway, and we probably shouldn't call this function with a too big len in the first place, so a return type of void may be the best choice. That said, if you decide to care about the result of this function, please make sure that ich_fill_data and ich_read_data have identical return types and check that the comments above the functions match the prototypes.
+static uint8_t ich_fill_data(const uint8_t *data, int len, int reg0_off) +{
- uint32_t temp32 = 0;
- int a;
See ich_read_data.
- if (len > spi_programmer->max_data_write)
len = spi_programmer->max_data_write;
See ich_read_data.
- if (len <= 0)
return 0;
Superfluous check? len<0 should not happen, and len==0 won't execute the for loop below.
- for (a = 0; a < len; a++) {
if ((a % 4) == 0) {temp32 = 0;}
Please kill superfluous {}.
temp32 |= ((uint32_t) data[a]) << ((a % 4) * 8);if ((a % 4) == 3) {REGWRITE32(reg0_off + (a - (a % 4)), temp32);}
Please kill superfluous {}.
- }
- if (((a - 1) % 4) != 3) {
Yeargh! AFAIK modulo for negative integers is implementation-defined. So your len<=0 check a few lines above may have been a good idea. That said, it is a good idea to add a comment so others won't fall into that trap. And all this (a-1) stuff in the line above and below would be a lot more readable if you inserted a--; before the if.
REGWRITE32(reg0_off + ((a - 1) - ((a - 1) % 4)), temp32);- }
Please kill superfluous {}.
- return len;
+}
/* This function generates OPCODES from or programs OPCODES to ICH according to
- the chipset's SPI configuration lock.
@@ -638,9 +689,8 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset, { int write_cmd = 0; int timeout;
- uint32_t temp32 = 0;
- uint32_t temp32; uint16_t temp16;
- uint32_t a; uint64_t opmenu; int opcode_index;
@@ -664,26 +714,8 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset, REGWRITE32(ICH7_REG_SPIA, (offset & 0x00FFFFFF) | temp32);
/* Program data into SPID0 to N */
- if (write_cmd && (datalength != 0)) {
temp32 = 0;for (a = 0; a < datalength; a++) {if ((a % 4) == 0) {temp32 = 0;}temp32 |= ((uint32_t) data[a]) << ((a % 4) * 8);if ((a % 4) == 3) {REGWRITE32(ICH7_REG_SPID0 + (a - (a % 4)),temp32);}}if (((a - 1) % 4) != 3) {REGWRITE32(ICH7_REG_SPID0 +((a - 1) - ((a - 1) % 4)), temp32);}- }
- if (write_cmd && (datalength != 0))
ich_fill_data(data, datalength, ICH7_REG_SPID0);
Nice! Killed superfluous {}.
/* Assemble SPIS */ temp16 = REGREAD16(ICH7_REG_SPIS); @@ -765,15 +797,7 @@ static int ich7_run_opcode(OPCODE op, uint32_t offset, }
if ((!write_cmd) && (datalength != 0)) {
for (a = 0; a < datalength; a++) {if ((a % 4) == 0) {temp32 = REGREAD32(ICH7_REG_SPID0 + (a));}data[a] =(temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))>> ((a % 4) * 8);}
ich_read_data(data, datalength, ICH7_REG_SPID0);
Can you kill {} here as well?
}
return 0; @@ -785,7 +809,6 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, int write_cmd = 0; int timeout; uint32_t temp32;
- uint32_t a; uint64_t opmenu; int opcode_index;
@@ -810,25 +833,8 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, REGWRITE32(ICH9_REG_FADDR, (offset & 0x00FFFFFF) | temp32);
/* Program data into FDATA0 to N */
- if (write_cmd && (datalength != 0)) {
temp32 = 0;for (a = 0; a < datalength; a++) {if ((a % 4) == 0) {temp32 = 0;}temp32 |= ((uint32_t) data[a]) << ((a % 4) * 8);if ((a % 4) == 3) {REGWRITE32(ICH9_REG_FDATA0 + (a - (a % 4)),temp32);}}if (((a - 1) % 4) != 3) {REGWRITE32(ICH9_REG_FDATA0 +((a - 1) - ((a - 1) % 4)), temp32);}- }
if (write_cmd && (datalength != 0))
ich_fill_data(data, datalength, ICH9_REG_FDATA0);/* Assemble SSFS + SSFC */ temp32 = REGREAD32(ICH9_REG_SSFS);
@@ -917,15 +923,7 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, }
if ((!write_cmd) && (datalength != 0)) {
for (a = 0; a < datalength; a++) {if ((a % 4) == 0) {temp32 = REGREAD32(ICH9_REG_FDATA0 + (a));}data[a] =(temp32 & (((uint32_t) 0xff) << ((a % 4) * 8)))>> ((a % 4) * 8);}
ich_read_data(data, datalength, ICH9_REG_FDATA0);
Kill superfluous {}
}
return 0;
Regards, Carl-Daniel