Dear Prafulla Wadaskar, In message <1248804270-13715-3-git-send-email-prafu...@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: w...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot