Hi Scott,
Thank you for reviewing patches 2-4.
On Thu, Aug 26, 2010 at 2:57 PM, Scott Wood <scottw...@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 <w...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
