Dear Prafulla Wadaskar,
In message 1248804270-13715-2-git-send-email-prafulla@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-
- Image specific code abstracted from mkimage.c to default_image.c/h to make mkimage code more generic
- struct mkimage_params introduced to pass basic mkimage specific flags and variables to image specific functions
Signed-off-by: Prafulla Wadaskar prafulla@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