[U-Boot] [PATCH 1/3] Bad block support for environment variables partition in OneNAND

[prakash t <prakash.t@samsung.com>]

Hi Rohit / Moorthy,

Do we have coding guidelines from open source? Let us follow that.
It is very good that they are reviewing in depth this also means that they 
are much interested in our products.

Regards,
Prakash Talawar

----- Original Message ----- 
From: "Scott Wood" <scottwood@freescale.com>
To: "Rohit Hagargundgi" <h.rohit@samsung.com>
Cc: <u-boot@lists.denx.de>; <k.rajesh@samsung.com>; <prakash.t@samsung.com>; 
<amitsharma.9@samsung.com>
Sent: Friday, January 16, 2009 2:42 AM
Subject: Re: [U-Boot] [PATCH 1/3] Bad block support for environment 
variables partition in OneNAND


> On Thu, Dec 11, 2008 at 07:58:26PM +0530, Rohit Hagargundgi wrote:
>> + env_addr = FLEXONENAND(this) ? CONFIG_ENV_ADDR_FLEX :
>> + CONFIG_ENV_ADDR;
>> +
>> + env_len = FLEXONENAND(this) ? CONFIG_ENV_SIZE_FLEX :
>> + CONFIG_ENV_SIZE;
>
> Is there any reason why this has to be done at runtime, rather than just
> using one set of CONFIG_ parameters regardless of whether it's flex?
>
>>
>>  /* Check OneNAND exist */
>> - if (onenand_mtd.writesize)
>> - /* Ignore read fail */
>> - onenand_read(&onenand_mtd, env_addr, onenand_mtd.writesize,
>> -      &retlen, (u_char *) env_ptr);
>> - else
>> + if (onenand_mtd.writesize) {
>> + while (env_addr < env_addr + env_len) {
>
> This is a tautology, unless you're checking for wraparound of unsigned
> long.
>
>> + onenand_get_block(this, ofs, &slc);
>> + if (slc)
>> + instr.len += 1 << (this->erase_shift -1);
>
> this->erase_shift - 1
>
>> + if (mtd->erase(mtd, &instr)) {
>> + printf("writeenv: erase failed at 0x%08lx\n", ofs);
>> + if(mtd->block_markbad(mtd, ofs))
>> + printf("writeenv: Mark bad failed at 0x%08lx\n", ofs);
>
> Space after "if".
>
> -Scott