[U-Boot] [PATCH V2 1/9] mkimage: Add Freescale imx Boot Image support (imximage)

[Wolfgang Denk <wd@denx.de>]

Dear Stefano Babic,

In message <1263376259-29764-2-git-send-email-sbabic@denx.de> you wrote:
> This patch adds support for "imximage" (MX Boot Image)
> to the mkimage utility. The imximage is used on the Freescales's
> MX.51 processors.
> 
> Further details under doc/README.imximage.
> 
> This patch was tested on a Freescale mx51evk board.
...
> diff --git a/tools/Makefile b/tools/Makefile
> index 5b8c3c3..afc1d44 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -73,6 +73,7 @@ OBJ_FILES-$(CONFIG_CMD_NET) += gen_eth_addr.o
>  OBJ_FILES-$(CONFIG_CMD_LOADS) += img2srec.o
>  OBJ_FILES-$(CONFIG_INCA_IP) += inca-swap-bytes.o
>  NOPED_OBJ_FILES-y += kwbimage.o
> +NOPED_OBJ_FILES-y += imximage.o
>  NOPED_OBJ_FILES-y += mkimage.o
>  OBJ_FILES-$(CONFIG_NETCONSOLE) += ncb.o
>  NOPED_OBJ_FILES-y += os_support.o
> @@ -159,6 +160,7 @@ $(obj)mkimage$(SFX):	$(obj)crc32.o \
>  			$(obj)fit_image.o \
>  			$(obj)image.o \
>  			$(obj)kwbimage.o \
> +			$(obj)imximage.o \
>  			$(obj)md5.o \
>  			$(obj)mkimage.o \
>  			$(obj)os_support.o \

Please keep lists sorted.

> +			case CFG_COMMAND:
> +				cmd = get_table_entry_id(imximage_cmds,
> +					"imximage commands", token);
> +				if (cmd < 0) {
> +					fprintf(stderr, "Error: %s[%d] - Invalid command (%s)\n",

Line to long. And please try to stick with a consistent error message
format.

> +			case CFG_REG_SIZE:
> +				switch (cmd) {
> +				case CMD_BOOT_FROM:
> +					/* Get flash header offset */
> +					imxhdr->flash_offset = get_table_entry_id(imximage_bootops,

Ditto, and so on. Please fix globally.

> +int imximage_check_params(struct mkimage_params *params)
> +{
> +	if (!params)
> +		return CFG_INVALID;
> +	if (!strlen(params->imagename)) {
> +		fprintf(stderr, "Error:%s - Configuration file not specified, "
> +			"it is needed for imximage generation\n",
> +			params->cmdname);
> +		return CFG_INVALID;
> +	}
> +	return	(params->dflag && (params->fflag || params->lflag)) ||
> +		(params->fflag && (params->dflag || params->lflag)) ||
> +		(params->lflag && (params->dflag || params->fflag)) ||
> +		(params->xflag) || !(strlen(params->imagename));

I cannot understand this code.

> +static struct image_type_params imximage_params = {
> +	.name = "Freescale i.MX 51 Boot Image support",
> +	.header_size = sizeof(struct imx_header),
> +	.hdr = (void *)&imximage_header,
> +	.check_image_type = imximage_check_image_types,
> +	.verify_header = imximage_verify_header,
> +	.print_header = imximage_print_header,
> +	.set_header = imximage_set_header,
> +	.check_params = imximage_check_params,
> +};

How about using TABs to make this easier to read, say:

static struct image_type_params imximage_params = {
	.name		= "Freescale i.MX 51 Boot Image support",
	.header_size	= sizeof(struct imx_header),
	.hdr		= (void *)&imximage_header,
	.check_image_type = imximage_check_image_types,
	.verify_header	= imximage_verify_header,
	.print_header	= imximage_print_header,
	.set_header	= imximage_set_header,
	.check_params	= imximage_check_params,
};


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
Brontosaurus Principle: Organizations  can  grow  faster  than  their
brains  can manage them in relation to their environment and to their
own physiology: when this occurs, they are an endangered species.
                                                - Thomas K. Connellan