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

[Wolfgang Denk]

Dear Ben Gardiner,

In message <1278366212-24023-3-git-send-email-bengardiner at 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 at nanometrics.ca>
> CC: Wolfgang Denk <wd at 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 at 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!"