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

On Friday, August 19, 2011 14:19:42 Simon Glass wrote:

+static const char *write_block(struct spi_flash *flash, u32 offset,

• size_t len, const char *buf, char *cmp_buf, size_t *skipped)
  

"spi_flash_update_block" is probably a better name. after all, you're doing more (and sometimes less) than "writing a block".

• return"read";
  

need a space in the middle there

• printf("SPI flash %s failed\n", err_oper);
  

how about: SPI flash update failed in %s step running 'sf update' on the command line would be obvious where the problem is coming from, but if you have a bunch of sf commands in a script and only see one error, it's probably confusing to quickly pick out where the error occurred.

• if (strcmp(argv[0], "read") == 0)

• if (strcmp(argv[0], "update") == 0)

• ret = spi_flash_update(flash, offset, len, buf);
  

• else if (strcmp(argv[0], "read") == 0) ret = spi_flash_read(flash, offset, len, buf);

...

• if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0)

• if (strcmp(cmd, "read") == 0 || strcmp(cmd, "write") == 0 ||

• 	strcmp(cmd, "update") == 0)
  

  ret = do_spi_flash_read_write(argc, argv); else if (strcmp(cmd, "erase") == 0) ret = do_spi_flash_erase(argc, argv);

these duplicate strcmp's make me wonder if a follow up patch should turn these into a tokenize step first and then later code works off that

rest looks fine -mike