Split erase region walking out of erase_flash. That allows us to use erase region walking for a combined erase/write action, and is a prerequisite for partial flashing.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-walk_eraseregions/flashrom.c =================================================================== --- flashrom-walk_eraseregions/flashrom.c (Revision 1072) +++ flashrom-walk_eraseregions/flashrom.c (Arbeitskopie) @@ -1164,14 +1164,36 @@ return ret; }
-int erase_flash(struct flashchip *flash) +static int walk_eraseregions(struct flashchip *flash, int erasefunction, int (*do_something) (struct flashchip *flash, unsigned int addr, unsigned int len)) { - int i, j, k, ret = 0, found = 0; + int i, j; + unsigned int done = 0; unsigned int start, len; + struct block_eraser eraser = flash->block_erasers[erasefunction]; + for (i = 0; i < NUM_ERASEREGIONS; i++) { + /* count==0 for all automatically initialized array + * members so the loop below won't be executed for them. + */ + for (j = 0; j < eraser.eraseblocks[i].count; j++) { + start = done + eraser.eraseblocks[i].size * j; + len = eraser.eraseblocks[i].size; + msg_cdbg("0x%06x-0x%06x, ", start, + start + len - 1); + if (do_something(flash, start, len)) + return 1; + } + done += eraser.eraseblocks[i].count * + eraser.eraseblocks[i].size; + } + return 0; +}
+int erase_flash(struct flashchip *flash) +{ + int k, ret = 0, found = 0; + msg_cinfo("Erasing flash chip... "); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { - unsigned int done = 0; struct block_eraser eraser = flash->block_erasers[k];
msg_cdbg("Looking at blockwise erase function %i... ", k); @@ -1194,24 +1216,7 @@ } found = 1; msg_cdbg("trying... "); - for (i = 0; i < NUM_ERASEREGIONS; i++) { - /* count==0 for all automatically initialized array - * members so the loop below won't be executed for them. - */ - for (j = 0; j < eraser.eraseblocks[i].count; j++) { - start = done + eraser.eraseblocks[i].size * j; - len = eraser.eraseblocks[i].size; - msg_cdbg("0x%06x-0x%06x, ", start, - start + len - 1); - ret = eraser.block_erase(flash, start, len); - if (ret) - break; - } - if (ret) - break; - done += eraser.eraseblocks[i].count * - eraser.eraseblocks[i].size; - } + ret = walk_eraseregions(flash, k, eraser.block_erase); msg_cdbg("\n"); /* If everything is OK, don't try another erase function. */ if (!ret)
Am Samstag, den 10.07.2010, 15:56 +0200 schrieb Carl-Daniel Hailfinger:
-int erase_flash(struct flashchip *flash) +static int walk_eraseregions(struct flashchip *flash, int erasefunction, int (*do_something) (struct flashchip *flash, unsigned int addr, unsigned int len)) {
- int i, j, k, ret = 0, found = 0;
- int i, j;
- unsigned int done = 0; unsigned int start, len;
- struct block_eraser eraser = flash->block_erasers[erasefunction];
- for (i = 0; i < NUM_ERASEREGIONS; i++) {
/* count==0 for all automatically initialized array* members so the loop below won't be executed for them.*/for (j = 0; j < eraser.eraseblocks[i].count; j++) {start = done + eraser.eraseblocks[i].size * j;
See below.
len = eraser.eraseblocks[i].size;
You could pull this line out of the loop, but you don't have to.
msg_cdbg("0x%06x-0x%06x, ", start,start + len - 1);if (do_something(flash, start, len))return 1;
You are just moving this code, but I still have a comment on it: Why don't you increase "done" here? You don't need the "start" variable any more and you don't have to write the explicit multiplication below.
}done += eraser.eraseblocks[i].count *eraser.eraseblocks[i].size;- }
- return 0;
+}
The patch as-is just refactors without any functionality change and thus is Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
On the other hand, if you change the function as indicated above (and think a second about whether my suggestions really make sense), this is also Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Regards, Michael Karcher
On 10.07.2010 20:39, Michael Karcher wrote:
You are just moving this code, but I still have a comment on it: Why don't you increase "done" here? You don't need the "start" variable any more and you don't have to write the explicit multiplication below.
Yes, that code is a leftover from previous versions. Killed.
The patch as-is just refactors without any functionality change and thus is Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
On the other hand, if you change the function as indicated above (and think a second about whether my suggestions really make sense), this is also Acked-by: Michael Karcher flashrom@mkarcher.dialup.fu-berlin.de
Thanks! Your suggestions indeed make sense, and the code is now a lot simpler. New version follows. I'm not sure if I should move start+=len into the counting expression of the for loop. Comments welcome.
Split erase region walking out of erase_flash. That allows us to use erase region walking for a combined erase/write action, and is a prerequisite for partial flashing
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-walk_eraseregions/flashrom.c =================================================================== --- flashrom-walk_eraseregions/flashrom.c (Revision 1074) +++ flashrom-walk_eraseregions/flashrom.c (Arbeitskopie) @@ -1164,14 +1164,34 @@ return ret; }
+static int walk_eraseregions(struct flashchip *flash, int erasefunction, int (*do_something) (struct flashchip *flash, unsigned int addr, unsigned int len)) +{ + int i, j; + unsigned int start = 0; + unsigned int len; + struct block_eraser eraser = flash->block_erasers[erasefunction]; + for (i = 0; i < NUM_ERASEREGIONS; i++) { + /* count==0 for all automatically initialized array + * members so the loop below won't be executed for them. + */ + len = eraser.eraseblocks[i].size; + for (j = 0; j < eraser.eraseblocks[i].count; j++) { + msg_cdbg("0x%06x-0x%06x, ", start, + start + len - 1); + if (do_something(flash, start, len)) + return 1; + start += len; + } + } + return 0; +} + int erase_flash(struct flashchip *flash) { - int i, j, k, ret = 0, found = 0; - unsigned int start, len; + int k, ret = 0, found = 0;
msg_cinfo("Erasing flash chip... "); for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { - unsigned int done = 0; struct block_eraser eraser = flash->block_erasers[k];
msg_cdbg("Looking at blockwise erase function %i... ", k); @@ -1194,24 +1214,7 @@ } found = 1; msg_cdbg("trying... "); - for (i = 0; i < NUM_ERASEREGIONS; i++) { - /* count==0 for all automatically initialized array - * members so the loop below won't be executed for them. - */ - for (j = 0; j < eraser.eraseblocks[i].count; j++) { - start = done + eraser.eraseblocks[i].size * j; - len = eraser.eraseblocks[i].size; - msg_cdbg("0x%06x-0x%06x, ", start, - start + len - 1); - ret = eraser.block_erase(flash, start, len); - if (ret) - break; - } - if (ret) - break; - done += eraser.eraseblocks[i].count * - eraser.eraseblocks[i].size; - } + ret = walk_eraseregions(flash, k, eraser.block_erase); msg_cdbg("\n"); /* If everything is OK, don't try another erase function. */ if (!ret)
Am Montag, den 12.07.2010, 03:23 +0200 schrieb Carl-Daniel Hailfinger:
Thanks! Your suggestions indeed make sense, and the code is now a lot simpler. New version follows. I'm not sure if I should move start+=len into the counting expression of the for loop. Comments welcome.
My personal preference: Keep it outside. I expect the counting expression to only affect the variable(s) initialized in the init expression and/or tested in the loop condition. Logically, the increase of the start address seems (to me) more as part of the "do_something" than as part of the loop.
+static int walk_eraseregions(struct flashchip *flash, int erasefunction, int (*do_something) (struct flashchip *flash, unsigned int addr, unsigned int len))
This is an overly long line, please wrap it.
My Ack still stands.
Regards, Michael Karcher
On 12.07.2010 08:40, Michael Karcher wrote:
Am Montag, den 12.07.2010, 03:23 +0200 schrieb Carl-Daniel Hailfinger:
Thanks! Your suggestions indeed make sense, and the code is now a lot simpler. New version follows. I'm not sure if I should move start+=len into the counting expression of the for loop. Comments welcome.
My personal preference: Keep it outside. I expect the counting expression to only affect the variable(s) initialized in the init expression and/or tested in the loop condition. Logically, the increase of the start address seems (to me) more as part of the "do_something" than as part of the loop.
OK.
+static int walk_eraseregions(struct flashchip *flash, int erasefunction, int (*do_something) (struct flashchip *flash, unsigned int addr, unsigned int len))
This is an overly long line, please wrap it.
My Ack still stands.
Thanks for the review, committed in r1077 (region walking) and r1078 (line wrap).
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Michael Karcher