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

[Simon Glass]

Hi Mike,

On Fri, Aug 19, 2011 at 1:03 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> 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

Done

>
>> +             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.

Done
>
>> -     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

Yes I agree. Still the sf code is quite a bit cleaner than March.

>
> rest looks fine
> -mike
>

Thanks,
Simon