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. http://www.nanometrics.ca