[U-Boot] [PATCH v2 4/4] Flex-OneNAND boundary setting command

[Amul Kumar Saha]

Hi Scott,

> 
>> + int blocks = (int) onenand_block(this, from + len)
>> +    - onenand_block(this, from);
> 
> Why the (int) cast?  onenand_block() already returns int.
> 

Resolved

>> - if (end_block > (mtd->size >> this->erase_shift))
>> -  end_block = mtd->size >> this->erase_shift;
>> + if (end_block > onenand_block(this, mtd->size))
>> +  end_block = onenand_block(this, mtd->size);
> 
> This should probably be:
> 
> if (end_block >= onenand_block(this, mtd->size - 1))
> 
> ...to avoid asking for a block that doesn't exist.
> 

Changed from mtd->size to (mtd->size) - 1,
In the OP, end_block was understood to accomodate (last_block_no + 1).
But, as you have pointed it may be more conventional to have the exact last_block_no

made necessary changes in some other places as well.

>>  blocks = start_block;
>>  ofs = start;
>>  while (blocks < end_block) {
>> -  printf("\rTesting block %d at 0x%x", (u32)(ofs >> this->erase_shift), 
>> (u32)ofs);
>> +  printf("\rTesting block %d at 0x%x", (u32) onenand_block(this, ofs), 
>> (u32)ofs);
> 
> No (u32) cast on return from onenand_block().  Likewise elsewhere.
> 

Accepted and changed.

>> +   for (ofs = 0; ofs < mtd->size; ofs += blocksize) {
>>     if (mtd->block_isbad(mtd, ofs))
>>      printf("  %08x\n", (u32)ofs);
>> +
>> +    blocksize = onenand_blocksize(ofs);
>>    }
> 
> Hmm, not sure I like blocksize being used in the for loop when it's first
> initialized in the body of the loop.  It's valid code, but it might be
> more readable as a while loop.
> 

Modified it to a while loop. Thanks :)

ofs = 0;
while (ofs < mtd->size) {
 if (mtd->block_isbad(mtd, ofs))
     printf("  %08x\n", (u32)ofs);
 ofs += onenand_blocksize(ofs);
}

>> +    die = (int) simple_strtoul(argv[2], NULL, 0);
>> +    bdry = (int) simple_strtoul(argv[3], NULL, 0);
> 
> Casts should not be necessary.
> 

I believe that typecasting a UL to an int, is OK.
Do let me know.

>> +    if (argc == 5 && strncmp(argv[4], "LOCK", 4) == 0)
>> +     lock = 1;
> 
> Please have commands be lower-case, just like all the other ones.
> 
Changed

Do let me know, your feedback on the changes made.
I'll send the updated patch, in the next post.

With regards,
Amul