Patchworkβ Split out erase region walking

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2010-07-10 13:56:35
Message ID <4C387C13.60304@gmx.net>
Download mbox | patch
Permalink /patch/1604/
State Superseded
Headers show

Comments

Carl-Daniel Hailfinger - 2010-07-10 13:56:35
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>
Michael Karcher - 2010-07-10 18:39:21
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

Patch

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)