[U-Boot] [PATCH] Add 'sf update' command to do smart SPI flash update

[Mike Frysinger]

On Friday, August 19, 2011 09:47:04 Simon Glass wrote:
> This adds a new SPI flash command which only rewrites blocks if the
> contents need to change. This can speed up SPI flash programming when much
> of the data is unchanged from what is already there.

sounds good

> +static int spi_flash_update(struct spi_flash *flash, u32 offset,
> +		size_t len, const char *buf)
> +{
> +	const char *oper;

i wonder if there'd be any code size difference if we had:
	static const char * const opers[] = {
		"malloc",
		"read",
		...
	};
	const char *oper;
	...
		oper = 0;
	...
		++oper;
	...
		++oper;
	...
	printf("...%s...", ..., opers[oper]);

i hope it'd be slightly smaller as you would be loading up a small int in the 
for loop and not an entire pointer ...

> +	int err = 0;
> +	char *cmp_buf = NULL;
> +	size_t cmp_size = 0;
> +	size_t todo;	/* number of bytes to do in this pass */
> +	size_t skipped, written;		/* statistics */
> +
> +	for (skipped = written = 0; !err && len > 0;
> +			buf += todo, offset += todo, len -= todo) {
> +		todo = min(len, flash->sector_size);
> +		debug("offset=%#x, sector_size=%#x, todo=%#x\n",
> +		      offset, flash->sector_size, todo);
> +		if (!err) {

why do you need to check err here ?  you already have "!err" in the for loop 
constraint, err starts off at 0, and there is no setting of err between the 
top of the for loop and this point.

> +		if (!err) {
> +			oper = "read";
> +			err = spi_flash_read(flash, offset, todo, cmp_buf);
> +		}

all this error checking makes my head spin.  i wonder if it'd look any better 
using the (more normal?) style:
err = ...;
if (err)
	break;

err = ...;
if (err)
	break;

you could then also drop "!err" from the for loop constraint, and the initial 
"err = 0".  not that big of a deal if the code size is the same ...

> +		if (!err) {
> +			if (0 == memcmp(cmp_buf, buf, todo)) {

please reverse the sides of this compare.  this is unusual style for the 
linux/u-boot projects.

> +	if (cmp_buf)
> +		free(cmp_buf);

the if() isn't necessary ... free(NULL) works fine and would add extra 
overhead only in the (uncommon) failure case.  same goes for the malloc() 
logic earlier in this func.

> +	"sf update addr offset len	- erase and write `len' bytes from "
> +		"memory\n"

please don't split the string constant on non-\n boundaries
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110819/9ec1af5f/attachment.pgp