[U-Boot] [PATCH] nand: adjust erase/read/write partition/chip size for bad blocks

[Harvey Chapman]

On Feb 26, 2013, at 8:42 PM, Scott Wood <scottwood at freescale.com> wrote:

> On 02/25/2013 11:43:48 PM, Harvey Chapman wrote:
> 
> Looks OK except for style issues:

Will do.

>> +	/* We grab the nand info object here fresh because this is usually
>> +	 * called after arg_off_size() which can change the value of dev.
>> +	 */
> 
> /*
> * U-Boot multiline
> * brace style is like this
> */
> 
> ...in general, U-Boot follows Linux code style.

I was trying to match the file. Every other multi-line comment in that file has the asterisks aligned.

>> +	nand_info_t *nand = &nand_info[dev];
>> +	loff_t original_size = *size;
>> +	loff_t maxoffset = offset + *size;
>> +	int badblocks = 0;
>> +
>> +	/* count badblocks in NAND from offset to offset + size */
>> +	for (; offset < maxoffset; offset += nand->erasesize)
>> +		if (nand_block_isbad(nand, offset)) {
>> +			badblocks++;
>> +		}
> 
> Need braces around multi-line statements.

I misunderstood you before. I thought you wanted them around the if body. Got it.

>> +	/* adjust size if any bad blocks found */
>> +	if (badblocks) {
>> +		*size -= badblocks * nand->erasesize;
>> +		printf("size adjusted to 0x%llx (%d bad blocks)\n",
>> +		       (unsigned long long)*size, badblocks);
>> +	}
>> +	/* return size adjusted as a positive value so callers
>> +	 * can use the return code to determine if anything happened
>> +	 */
>> +	return (original_size - *size);
> 
> Unnecessary parens.
> 
> Do we have any callers that care about the return code?  If not, don't bother with it.  This is an internal static function, not a public API.  It's easy to change later if we need to.

Ok.

>> 		nand = &nand_info[dev];
>> 		memset(&opts, 0, sizeof(opts));
>> @@ -644,6 +682,19 @@ static int do_nand(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> 						&off, &size) != 0)
>> 				return 1;
>> +			/* If no size was given, it has been calculated for us as
>> +			 * the remainder of the chip/partition from offset. Adjust
>> +			 * down for bad blocks, if necessary, so we don't
>> +			 * read/write past the end of the partition by accident.
>> +			 *
>> +			 * nand read addr part size     "size" is arg 5
>> +			 */
>> +			if (argc < 5) {
>> +				/* Don't try to use rwsize here, it's not the
>> +				 * right type
>> +				 */
>> +				adjust_size_for_badblocks(&size, off, dev);
>> +			}
> 
> No need to be quite so verbose in the comments.  If someone tries to change "size" to "rwsize" the compiler will complain about the type mismatch.

Actually, it does give a warning that flies by and is buried inside the compile log. Then, it'll let you run and walk on memory. Ask me how I know. :)

I don't have a special love for that comment; I'm ok to remove it.

> As for the other comment, the function name "adjust_size_for_badblocks" explains what's going on well enough IMHO.  At most a single comment of /* size is unspecified */ to describe the if block.  At least put the explanation on the adjust_size_for_badblocks() function rather than repeat it for read/write and erase.

will do.

Thanks,
Harvey
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 1089 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130226/2b592bfa/attachment.bin>