[U-Boot] [PATCH 2/8] tools: mkimage: code abstraction generic and image specific

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <1248804270-13715-2-git-send-email-prafulla at marvell.com> you wrote:
> This is first step towards cleaning mkimage code for kwbimage
> support in clean way. Current mkimage code is very specific to
> uimge generation whereas the same framework can be used to
> generate other image types like kwbimage.
> For this, the architecture of mkimage code need to modified.
> 
> Here is the brief plan for the same:-
> a) Abstract image specific code to saperate file (this patch)
> b) Move image header code from mkimage.c to respective
> 	image specific code
> c) Implement callback function for image specific functions
> d) Add kwbimage type support to this framework
> 
> In this patch-
> 1. Image specific code abstracted from mkimage.c to
> 	default_image.c/h to make mkimage code more generic
> 2. struct mkimage_params introduced to pass basic mkimage
> 	specific flags and variables to image specific functions
> 
> Signed-off-by: Prafulla Wadaskar <prafulla at marvell.com>
> ---
>  tools/Makefile        |    4 +
>  tools/default_image.c |  227 ++++++++++++++++++++++++++++++
>  tools/default_image.h |   36 +++++
>  tools/mkimage.c       |  368 ++++++++++++++-----------------------------------
>  tools/mkimage.h       |   25 ++++
>  5 files changed, 394 insertions(+), 266 deletions(-)
>  create mode 100644 tools/default_image.c
>  create mode 100644 tools/default_image.h

Just nitpicking...

> diff --git a/tools/Makefile b/tools/Makefile
> index b5a1e39..6ef15d9 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -171,6 +171,7 @@ $(obj)img2srec$(SFX):	$(obj)img2srec.o
>  	$(STRIP) $@
>  
>  $(obj)mkimage$(SFX):	$(obj)mkimage.o $(obj)crc32.o $(obj)image.o $(obj)md5.o \
> +			$(obj)default_image.o \
>  			$(obj)sha1.o $(LIBFDT_OBJS) $(obj)os_support.o

Please let's sort the files...

>  	$(CC) $(CFLAGS) $(HOST_LDFLAGS) -o $@ $^
>  	$(STRIP) $@
> @@ -203,6 +204,9 @@ $(obj)bin2header$(SFX): $(obj)bin2header.o
>  $(obj)image.o: $(SRCTREE)/common/image.c
>  	$(CC) -g $(FIT_CFLAGS) -c -o $@ $<
>  
> +$(obj)default_image.o: $(SRCTREE)/tools/default_image.c
> +	$(CC) -g $(FIT_CFLAGS) -c -o $@ $<
> +

Please sort here, too.

> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index b7b383a..66d6c8e 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -24,28 +24,30 @@
...
> +struct mkimage_params params = {
...
> +	.opt_os = IH_OS_LINUX,
> +	.opt_arch = IH_ARCH_PPC,
> +	.opt_type = IH_TYPE_KERNEL,
> +	.opt_comp = IH_COMP_GZIP,
> +	.opt_dtc = MKIMAGE_DEFAULT_DTC_OPTIONS,
...
> +	.imagename = "",
> +	.datafile = "",
> +	.imagefile = "",
> +	.cmdname = "",

I see no reason to initialize these strings. Please omit.

> +};
...
>  			case 'A':
>  				if ((--argc <= 0) ||
> -				    (opt_arch = genimg_get_arch_id (*++argv)) < 0)
> +					(params.opt_arch =
> +					genimg_get_arch_id (*++argv)) < 0)
>  					usage ();
>  				goto NXTARG;
>  			case 'C':
>  				if ((--argc <= 0) ||
> -				    (opt_comp = genimg_get_comp_id (*++argv)) < 0)
> +					(params.opt_comp =
> +					genimg_get_comp_id (*++argv)) < 0)

The new code looks really ugly here and in all the following case:s;
This is mostly caused by the fact that the new code is much longer
"params.opt_XXX" versus "opt_XXX"). With the new code all the options
are in the params structure anyway, so ther eis no danger to confuse
these withother variables, so we should omit the "opt_" part, I think.

This gives:

struct mkimage_params params = {
...
	.os = IH_OS_LINUX,
	.arch = IH_ARCH_PPC,
	.type = IH_TYPE_KERNEL,
	.comp = IH_COMP_GZIP,
	.dtc = MKIMAGE_DEFAULT_DTC_OPTIONS,
...
};
...
			case 'A':
				if ((--argc <= 0) ||
				    (params.arch = genimg_get_arch_id(*++argv)) < 0) {
					usage ();
				}
				goto NXTARG;
			case 'C':
				if ((--argc <= 0) ||
				    (params.comp = genimg_get_comp_id(*++argv)) < 0) {
					usage ();
				}
				goto NXTARG;
...


etc.



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
Extended Epstein-Heisenberg Principle: In an R & D orbit, only  2  of
the  existing 3 parameters can be defined simultaneously. The parame-
ters are: task, time and resources ($).