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 <[email protected]> > > 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 <[email protected]> > 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 <[email protected]> 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) -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
