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

[Scott Wood]

On Mon, Mar 23, 2009 at 12:36:32PM +0530, Amul Kumar Saha wrote:
> diff --git a/common/cmd_onenand.c b/common/cmd_onenand.c
> index 5832ff8..ed6681a 100644
> --- a/common/cmd_onenand.c
> +++ b/common/cmd_onenand.c
> @@ -65,36 +65,49 @@ static int arg_off_size(int argc, char *argv[], ulong 
> *off, size_t *size)
>  return 0;
> }
> 
> +static inline int onenand_blocksize(loff_t ofs)
> +{
> + struct onenand_chip *this = mtd->priv;
> + int i;

Patch is whitespace-damaged.  Please send the patch separately (not as a
reply; it's easy to miss patches at the end of a reply, and I have to
manually strip the discussion from the changelog) using something that
will preserve the patch text intact (not Outlook Express).

> + int blocks = (int) onenand_block(this, from + len)
> +    - onenand_block(this, from);

Why the (int) cast?  onenand_block() already returns int.

> - 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.

>  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.

> @@ -359,9 +378,11 @@ int do_onenand(cmd_tbl_t * cmdtp, int flag, int argc, 
> char *argv[])
>   if (strcmp(cmd, "bad") == 0) {
>    /* Currently only one OneNAND device is supported */
>    printf("\nDevice %d bad blocks:\n", 0);
> -   for (ofs = 0; ofs < mtd->size; ofs += mtd->erasesize) {
> +   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.

> +    die = (int) simple_strtoul(argv[2], NULL, 0);
> +    bdry = (int) simple_strtoul(argv[3], NULL, 0);

Casts should not be necessary.

> +    if (argc == 5 && strncmp(argv[4], "LOCK", 4) == 0)
> +     lock = 1;

Please have commands be lower-case, just like all the other ones.

-Scott