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.
> +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".]
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?
> +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?
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
