Hi Tom,
Thanks for reviewing.
On Mon, Mar 28, 2016 at 11:15:43AM -0400, Tom Rini wrote:
>On Mon, Mar 28, 2016 at 05:37:16PM +0800, Peng Fan wrote:
>
>> Introduce env support for sata device.
>> 1. Implement write_env/read_env/env_relocate_spec/saveenv/sata_get_env_dev
>> 2. If want to enable this feature, define CONFIG_ENV_IS_IN_SATA, and
>> define CONFIG_SYS_SATA_ENV_DEV or implement your own sata_get_ev_dev.
>[snip]
>> +/*
>> + * TODO: Support CONFIG_ENV_SIZE_REDUND CONFIG_ENV_OFFSET_REDUND
>> + */
>
>I'd like to see #error here as well, having been bit in the past in
>cases of "Oh, redund doesn't work?".
Will fix in V2.
>
>> +#if !defined(CONFIG_ENV_OFFSET) || !defined(CONFIG_ENV_SIZE)
>> +#error CONFIG_ENV_OFFSET or CONFIG_ENV_SIZE not defined
>> +#endif
>> +
>> +char *env_name_spec = "SATA";
>> +
>> +#ifdef ENV_IS_EMBEDDED
>> +env_t *env_ptr = (env_t *)(&environment[0]);
>
>So, you're not adding (and I'm not suggesting you do) SATA to the list
>of environment types where we support embedded env. Please just remove
>these tests here and elsewhere. Thanks!
Fix in V2.
>
>[snip]
>> +static inline int read_env(struct blk_desc *sata, unsigned long size,
>> + unsigned long offset, const void *buffer)
>> +{
>> + uint blk_start, blk_cnt, n;
>> +
>> + blk_start = ALIGN(offset, sata->blksz) / sata->blksz;
>> + blk_cnt = ALIGN(size, sata->blksz) / sata->blksz;
>> +
>> + n = sata->block_read(sata, blk_start, blk_cnt, (uchar *)buffer);
>
>Here and elsewhere, please update to use blk_dread, etc, from blk.h to
>help future proof this code, thanks!
Fix in V2.
Thanks,
Peng.
>
>--
>Tom
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
