Hi Wolfgang,
The 12/03/2018 17:08, Wolfgang Denk wrote:
> Dear Horatiu,
>
> In message <1543678222-15837-1-git-send-email-horatiu.vul...@microchip.com>
> you wrote:
> > The flash_read function is a wrapper over spi_flash_read, which enables
> > the env to read multiple flash page size from flash until '\0\0' is read
> > or the end of env partition is reached. Instead of reading the entire env
> > size. When it reads '\0\0', it stops reading further the env and assumes
> > that the rest of env is '\0'.
> >
> > This is an optimization for large environments that contain few bytes of
> > environment variables. In this case it doesn't need to read the entire
> > environment and only few pages.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vul...@microchip.com>
> > ---
> > env/sf.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 43 insertions(+), 11 deletions(-)
> >
> > diff --git a/env/sf.c b/env/sf.c
> > index 2e3c600..e1e1a94 100644
> > --- a/env/sf.c
> > +++ b/env/sf.c
> > @@ -81,6 +81,39 @@ static int setup_flash_device(void)
> > return 0;
> > }
> >
> > +static int is_end(char *addr, int size)
>
> That should be
>
> (const char *addr, size_t size)
>
> instead.
I will change this in the next version.
>
> > +static int flash_read(struct spi_flash *flash, u32 offset, int size, void
> > *buf)
>
> I think this should be
>
> (const struct spi_flash *flash, u32 offset, size_t len, void *buf)
>
> [Yes, for reasons of consistency with spi_flash_read() I would also
> rename "size" into "len".]
>
The same here.
>
> Also I recommend to chose another name for the function instead of
> "flash_read". At first glance this looks like a generalization over
> spi_flash_read() which supports other flash types than SPI flash as
> well - but it does not. Instead, it is a specialized version of
> spi_flash_read() tailored for environment reading only.
>
> Maybe spi_flash_read_env() would be a more reasonable name?
>
Yes spi_flash_read_env is a better name. I will update it in the next
version.
>
> > +static int flash_read(struct spi_flash *flash, u32 offset, int size, void
> > *buf)
> > +{
> > + u32 addr = 0;
> > + u32 page_size = flash->page_size;
> > +
> > + memset(buf, 0x0, size);
> > + for (int i = 0; i < size / page_size; ++i) {
> > + int ret = spi_flash_read(flash, offset, page_size,
> > + &((char *)buf)[addr]);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (is_end(&((char *)buf)[addr], page_size))
> > + return 0;
> > +
> > + addr += page_size;
> > + offset += page_size;
> > + }
> > + return 0;
>
> Is this correct? Is the read() function not supposed to return the
> number of read bytes?
It is true that C read function returns the number of bytes read, but
the intention of the function flash_read was to mimic the function
spi_flash_read, which returns 0 if OK otherwise an error code. And
that's the reason why the function flash_read returns 0 and not number
of bytes read.
It would not be a problem for me to change the function flash_read to
return the number of bytes read.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
> The optimum committee has no members.
> - Norman Augustine
--
Thanks,
/Horatiu
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
