[U-Boot] [PATCH v2 2/4] tools: mkimage (type=kwbimage) kirkwood boot image support

[Prafulla Wadaskar]

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de] 
> Sent: Wednesday, July 22, 2009 2:06 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik
> Subject: Re: [U-Boot] [PATCH v2 2/4] tools: mkimage 
> (type=kwbimage) kirkwood boot image support
> 
<snip>
> 
> > +		if (datacmd_cnt > KWBIMAGE_MAX_CONFIG ) {
> > +			printf("Err.. found more than max "
> > +				"allowed(%d) configurations\n",
> > +				KWBIMAGE_MAX_CONFIG);
> 
> Where is this limit coming from? Can the be arbitrarily 
> changed, or is it some inherent linif of your image format?
This limit is coming from the BootROM header size on kirkwood.

<snip>
> > @@ -164,6 +165,10 @@ NXTARG:		;
> >  		(lflag && (dflag || fflag)))
> >  		usage();
> >  
> > +	if (((argc != 1) && (opt_type == IH_TYPE_KWBIMAGE)) &&
> > +		(xflag || lflag))
> > +		usage();
> 
> Do you promise to add support for listing the image (lflag) later?
Yes, I have already done it :-)

> 
> >  	if (!eflag) {
> >  		ep = addr;
> >  		/* If XIP, entry point must be after the U-Boot 
> header */
> > @@ -251,7 +256,12 @@ NXTARG:		;
> >  	 *
> >  	 * write dummy header, to be fixed later
> >  	 */
> > -	hdr_size = image_get_header_size ();
> > +	if (opt_type == IH_TYPE_KWBIMAGE) {
> > +		hdr = kwbimage_get_header_ptr();
> > +		hdr_size = kwbimage_get_header_size ();
> > +	} else
> > +		hdr_size = image_get_header_size ();
> 
> Maybe we should use a function pointer for 
> image_get_header_size() which can be set just once, so we 
> don't have to change the code?
<snip>
> > +		sbuf.st_size += sizeof(uint32_t);
> > +		kwbimage_set_header (hdr, &sbuf, addr, ep, name);
> > +	} else {
> >  	checksum = crc32 (0,
> 
> Here indentation becomes wrong.
> 
> And I don't like this if()...else... thingy here. I think we 
> should move the respective code into functions, and just set 
> a pointer to the function the builds the header.
> 
> This prevents an ever growing list of if()...else...else...else...
> in case more image formats need to get added.
That's good idea. My initial intention was not to disturb mkimage code much.
But doing this will make more easier for growing support.
I will have to restructure my code and mkimage code too.
I will do it.

Regards..
Prafulla . .

> 
>