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

Wolfgang Denk
2010-08-07T20:08:50Z

Dear Ben Gardiner,

In message <1278366212-24023-3-git-send-email-bengardiner@nanometrics.ca> you wrote:
> This patch adds an additional column to the output of list_partitions. The
> additional column will contain the net size and a '(!)' beside it if the net
> size is not equal to the partition size.
>
> Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
> CC: Wolfgang Denk <wd@denx.de>
>
> ---
>
> V2:
> * formatting: spaces after 'if' and 'for'
> * the entire new feature is conditional on a macro, there is now a zero-byte
> binary size impact when the macro is not defined.
> * return the net parition size directly from net_part_size instead of using
> an output variable
>
> V3:
> * rebased to 54841ab50c20d6fa6c9cc3eb826989da3a22d934 of
> git://git.denx.de/u-boot.git
> * fix line length over 80 chars
> * update copyright of cmd_mtdparts.c

That was a bad idea. We do not add change logs to files, because we
have the full history of changes in git.

> --- a/common/cmd_mtdparts.c
> +++ b/common/cmd_mtdparts.c
> @@ -15,6 +15,10 @@
> * Parsing routines are based on driver/mtd/cmdline.c from the linux 2.4
> * kernel tree.
> *
> + * (C) Copyright 2010
> + * Ben Gardiner, Nanometrics Inc., <bengardiner@nanometrics.ca>
> + * Added net partition size output to mtdparts list command
> + *

NAK for this hunk.

> +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
> +/** 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
> + */

Incorrect multiline comment style.

> + printf(" #: name\t\tsize\t\t"
> +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
> + "net size\t"
> +#endif
> + "offset\t\tmask_flags\n");

Incorrect indentation.

> +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
> + if (get_mtd_info(dev->id->type, dev->id->num, &mtd))
> + return;
> +#endif
>
> /* list partitions for given device */
> part_num = 0;
> list_for_each(pentry, &dev->parts) {
> part = list_entry(pentry, struct part_info, link);
> - printf("%2d: %-20s0x%08x\t0x%08x\t%d\n",
> +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
> + net_size = net_part_size(mtd, part);
> +#endif
> + printf("%2d: %-20s0x%08x\t"
> +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
> + "0x%08x%s\t"
> +#endif
> + "0x%08x\t%d\n",
> part_num, part->name, part->size,
> +#if defined(CONFIG_CMD_MTDPARTS_SHOW_NET_SIZES)
> + net_size,
> + part->size == net_size ? " " : " (!)",
> +#endif

This is way too much #ifdef's here. Please separate the code and use a
single #ifdef only.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Quote from the Boss after overriding the decision of a task force he
created to find a solution: "I'm sorry if I ever gave you the
impression your input would have any effect on my decision for the
outcome of this project!"