Hi Mike,
On Fri, Aug 19, 2011 at 9:14 AM, Mike Frysinger vapier@gentoo.org wrote:
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 ...
It's exactly the same code/data size on ARM, but perhaps smaller on a different architecture. I like that approach anyway so will change it.
- 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.
It's a hangover from previous code - will remove it.
- 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 ...
Ick. I will send a new patch with the code in a function and see what you think about that. It is 0x88 bytes smaller on ARM than this patch.
- 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.
ok
- 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.
OK.
- "sf update addr offset len - erase and write `len' bytes from "
- "memory\n"
please don't split the string constant on non-\n boundaries
ok, will go over 80cols.
-mike