[U-Boot] [PATCH 2/3 v3] sandbox: new SPI flash driver

[Simon Glass]

Hi Mike,

On Wed, Mar 14, 2012 at 10:24 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Thursday 15 March 2012 00:09:59 Simon Glass wrote:
>> On Mon, Mar 12, 2012 at 8:22 AM, Mike Frysinger wrote:
>> > +/* Used to quickly bulk erase backing store */
>> > +static u8 sb_sf_0xff[0x10000];
>>
>> Ick, Does it really need to be so large?
>
> in order to do a single write() for a single sector, yes.  it also simplifies
> the code as i don't have to loop over some smaller size and keep track of how
> many bytes i've written until a sector is cleared.
>
> this is in the bss and it's 64KiB.  i'm really not worried about your
> development system running Chrome getting low on RAM :).

No, it's more that I think it is a bit grubby. But OK.

>
>> > +       sbsf->fd = os_open(file, 02);
>> > +       if (sbsf->fd == -1) {
>> > +               free(sbsf);
>> > +               goto error;
>>
>> Prints incorrect error if the file couldn't be found/open. I wonder if
>> we should have os_perror() ?
>
> probably would be useful.  we prob have to document that while os_perror()
> would work, strerror(errno) would not.  the os_* funcs update the errno that
> the C library knows about (or *__errno_location() in glibc), but u-boot
> declares a local "errno" variable.  so attempting to reference "errno" in u-
> boot code will always resolve to the local definition rather than the glibc
> one.  os_perror() would work because it calls __errno_location() internally
> just like the other os_* funcs.

Yes a bit messy, let's leave it for now then.

>
>> > +                       pos += os_read(sbsf->fd, tx + pos, cnt);
>>
>> This can fail (return -1)
>
> the ways in which read/write could fail aren't really possible for us, but i
> guess it won't hurt to assert() the values.

Are you sure? What if the file has a read error? That could happen in
the test setup. To me an assert is a bit ugly for this sort of thing.
Also for test code we don't want it dying in the bowels of the test
with an assert - better to return test failure so people can see what
went wrong.

Regards,
Simon

> -mike