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

Ben Gardiner
2010-08-27T13:51:19Z

Hi Scott,

Thank you for reviewing patches 2-4.

On Thu, Aug 26, 2010 at 2:57 PM, Scott Wood <scottwood@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@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.