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:
See below.
You could pull this line out of the loop, but you don't have to.
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.
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:
Yes, that code is a leftover from previous versions. Killed.
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:
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:
OK.
Thanks for the review, committed in r1077 (region walking) and r1078 (line wrap).
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Michael Karcher