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

Wolfgang Denk
2010-01-15T15:39:28Z

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