On Feb 26, 2013, at 8:42 PM, Scott Wood <scottw...@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
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
