On Feb 26, 2013, at 8:42 PM, Scott Wood scottwood@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