[U-Boot] [PATCH 3/8] tools: mkimage: Move image header code to image specific files

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <1248804270-13715-3-git-send-email-prafulla at marvell.com> you wrote:
> This is second step towards cleaning mkimage code for kwbimage
> support in clean way. In this patch-
> 1. The image_get_header_size function call is replaced by sizeof(image_header_t)
>    in default_image.c

I still fail to understand why this would be needed, or even wanted.

> 2. image header code moved form mkimage.c to default_image .c

This looks incorrect to me (not to mention the "form/from" typo):
I see no code being moved, only variable declarations.

> +/*
> + * Default image parameters
> + */
> +struct image_type_params defimage_params = {
> +	.header_size = sizeof(image_header_t),
> +	.hdr = (void*)&header,
> +};
> +
> +void *image_get_tparams (void){
> +	return (void *)&defimage_params;
> +}

We need better documentation. Why would that function be needed?

>  int image_check_params (struct mkimage_params *params)
>  {
>  	return	((params->dflag && (params->fflag || params->lflag)) ||
> @@ -100,13 +114,13 @@ void image_set_header (char *ptr, struct stat *sbuf,
>  	image_header_t * hdr = (image_header_t *)ptr;
>  
>  	checksum = crc32 (0,
> -			(const char *)(ptr + image_get_header_size ()),
> -			sbuf->st_size - image_get_header_size ());
> +			(const char *)(ptr + sizeof(image_header_t)),
> +			sbuf->st_size - sizeof(image_header_t));

I cannot help it, but this change looks like a step backward to me.
Why don't we use the header_size entry from the image_type_params
here, too?

...
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -49,9 +49,6 @@ struct mkimage_params params = {
>  	.cmdname = "",
>  };
>  
> -image_header_t header;
> -image_header_t *hdr = &header;
> -
>  int
>  main (int argc, char **argv)
>  {
> @@ -59,6 +56,7 @@ main (int argc, char **argv)
>  	struct stat sbuf;
>  	unsigned char *ptr;
>  	int retval;
> +	struct image_type_params *tparams = image_get_tparams ();
>  
>  	params.cmdname = *argv;
>  
> @@ -163,7 +161,7 @@ NXTARG:		;
>  		params.ep = params.addr;
>  		/* If XIP, entry point must be after the U-Boot header */
>  		if (params.xflag)
> -			params.ep += image_get_header_size ();
> +			params.ep += tparams->header_size;

Would it not be better to stick with the image_get_header_size()
function, and instead make it aware which image type we are asking
for?

The old code was homnogenuous: it consistently used
image_get_header_size(); now we have a sizeof() here and a
ptr->header_size there.

I really dislike that.


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
"Here's a fish hangs in the net like a poor man's right in  the  law.
'Twill hardly come out."     - Shakespeare, Pericles, Act II, Scene 1