[U-Boot] [PATCH] env: add flash_read function

[Wolfgang Denk]

Dear Horatiu,

In message <1543678222-15837-1-git-send-email-horatiu.vultur at 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.vultur at 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: wd at denx.de
The optimum committee has no members.
                                                   - Norman Augustine