[U-Boot] [PATCH v4 2/4] mtdparts: show net size in mtdparts list

[Ben Gardiner]

Hi Scott,

Thank you for reviewing patches 2-4.

On Thu, Aug 26, 2010 at 2:57 PM, Scott Wood <scottwood at freescale.com> wrote:
> On Mon, Aug 09, 2010 at 04:43:58PM -0400, Ben Gardiner wrote:
>> diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c
>> index 772ad54..500a38e 100644
>> --- a/common/cmd_mtdparts.c
>> +++ b/common/cmd_mtdparts.c
>> @@ -1215,18 +1215,65 @@ static int generate_mtdparts_save(char *buf, u32 buflen)
>>       return ret;
>>  }
>>
>> +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
>>  /**
>> - * Format and print out a partition list for each device from global device
>> - * list.
>> + * Get the net size (w/o bad blocks) of the given partition.
>> + *
>> + * @param mtd the mtd info
>> + * @param part the partition
>> + * @return the calculated net size of this partition
>>   */
>> -static void list_partitions(void)
>> +static u32 net_part_size(struct mtd_info *mtd, struct part_info *part)
>
> Don't assume partition size fits in 32 bits.  part->size is uint64_t.

My mistake.

>> +{
>> +     if (mtd->block_isbad) {
>> +             u32 i, bb_delta = 0;
>> +
>> +             for (i = 0; i < part->size; i += mtd->erasesize) {
>> +                     if (mtd->block_isbad(mtd, part->offset + i))
>> +                             bb_delta += mtd->erasesize;
>> +             }
>> +
>> +             return part->size - bb_delta;
>
> Seems like it'd be slightly simpler to just count up the good blocks,
> rather than count the bad blocks and subtract.

Will do.

>> +     } else {
>> +             return part->size;
>> +     }
>
> It's usually more readable to do this:
>
> if (can't do this)
>        return;
>
> do this;
>
> than this
>
> if (can do this)
>        do this;
> else
>        don't;
>
> When "do this" is more than a line or two, and there's nothing else to be
> done in the function afterward.

Right. I think you told me this in the env.oob review also. I'll keep
this in mind for future submissions.

>> +}
>> +#endif
>> +
>> +static void print_partition_table(void)
>>  {
>>       struct list_head *dentry, *pentry;
>>       struct part_info *part;
>>       struct mtd_device *dev;
>>       int part_num;
>>
>> -     debug("\n---list_partitions---\n");
>> +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
>> +     list_for_each(dentry, &devices) {
>> +             struct mtd_info *mtd;
>> +
>> +             dev = list_entry(dentry, struct mtd_device, link);
>> +             printf("\ndevice %s%d <%s>, # parts = %d\n",
>> +                             MTD_DEV_TYPE(dev->id->type), dev->id->num,
>> +                             dev->id->mtd_id, dev->num_parts);
>> +             printf(" #: name\t\tsize\t\tnet size\toffset\t\tmask_flags\n");
>> +
>> +             if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
>> +                     return;
>> +
>> +             /* list partitions for given device */
>> +             part_num = 0;
>> +             list_for_each(pentry, &dev->parts) {
>> +                     u32 net_size;
>> +                     char *size_note;
>> +
>> +                     part = list_entry(pentry, struct part_info, link);
>> +                     net_size = net_part_size(mtd, part);
>> +                     size_note = part->size == net_size ? " " : " (!)";
>> +                     printf("%2d: %-20s0x%08x\t0x%08x%s\t0x%08x\t%d\n",
>> +                                     part_num, part->name, part->size,
>> +                                     net_size, size_note, part->offset,
>> +                                     part->mask_flags);
>> +#else /* !defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
>>       list_for_each(dentry, &devices) {
>>               dev = list_entry(dentry, struct mtd_device, link);
>>               printf("\ndevice %s%d <%s>, # parts = %d\n",
>> @@ -1241,12 +1288,25 @@ static void list_partitions(void)
>>                       printf("%2d: %-20s0x%08x\t0x%08x\t%d\n",
>>                                       part_num, part->name, part->size,
>>                                       part->offset, part->mask_flags);
>> -
>> +#endif /* defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES) */
>
> Is there any way you could share more of this between the two branches?

I definitely could. :)

I had everything-possible shared between the branches in v3 but I
think I took it too far since:

On Sat, Aug 7, 2010 at 4:08 PM, Wolfgang Denk <wd at denx.de> wrote:
> This is way too much #ifdef's here. Please separate the code and use a
> single #ifdef only.

I'll try my best to strike a balance here in v5.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca