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