[U-Boot] [PATCH] mkimage: add "-V" option to print version information

[U-Boot] [PATCH] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
 tools/mkimage.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index f5859d7..788484d 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -23,6 +23,7 @@
 
 #include "mkimage.h"
 #include <image.h>
+#include <version.h>
 
 static void copy_file(int, const char *, int);
 static void usage(void);
@@ -246,6 +247,10 @@ main (int argc, char **argv)
 			case 'v':
 				params.vflag++;
 				break;
+			case 'V':
+				printf("mkimage version %s\n",
+					U_BOOT_VERSION + 7);
+				exit(EXIT_SUCCESS);
 			case 'x':
 				params.xflag++;
 				break;
@@ -590,6 +595,7 @@ usage ()
 		params.cmdname);
 	fprintf (stderr, "       %s [-D dtc_options] -f fit-image.its fit-image\n",
 		params.cmdname);
+	fprintf (stderr, "       %s -V ==> print version information and exit\n");
 
 	exit (EXIT_FAILURE);
 }
-- 
1.7.4

[U-Boot] [PATCH v2] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
v2: fix missing argument to printf() call.

 tools/mkimage.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index f5859d7..127be57 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -23,6 +23,7 @@
 
 #include "mkimage.h"
 #include <image.h>
+#include <version.h>
 
 static void copy_file(int, const char *, int);
 static void usage(void);
@@ -246,6 +247,10 @@ main (int argc, char **argv)
 			case 'v':
 				params.vflag++;
 				break;
+			case 'V':
+				printf("mkimage version %s\n",
+					U_BOOT_VERSION + 7);
+				exit(EXIT_SUCCESS);
 			case 'x':
 				params.xflag++;
 				break;
@@ -590,6 +595,8 @@ usage ()
 		params.cmdname);
 	fprintf (stderr, "       %s [-D dtc_options] -f fit-image.its fit-image\n",
 		params.cmdname);
+	fprintf (stderr, "       %s -V ==> print version information and exit\n",
+		params.cmdname);
 
 	exit (EXIT_FAILURE);
 }
-- 
1.7.4

[U-Boot] [PATCH] mkimage: add "-V" option to print version information

[Albert ARIBAUD]

Hi Wolfgang,

Le 11/02/2011 09:56, Wolfgang Denk a ?crit :
> Signed-off-by: Wolfgang Denk<wd@denx.de>
> ---
>   tools/mkimage.c |    6 ++++++
>   1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index f5859d7..788484d 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -23,6 +23,7 @@
>
>   #include "mkimage.h"
>   #include<image.h>
> +#include<version.h>
>
>   static void copy_file(int, const char *, int);
>   static void usage(void);
> @@ -246,6 +247,10 @@ main (int argc, char **argv)
>   			case 'v':
>   				params.vflag++;
>   				break;
> +			case 'V':
> +				printf("mkimage version %s\n",
> +					U_BOOT_VERSION + 7);

That 7 is a pretty awful magic number here, isn't it? At least if 
there's a sane reason for this it should be summarized in a short comment.

> +				exit(EXIT_SUCCESS);
>   			case 'x':
>   				params.xflag++;
>   				break;
> @@ -590,6 +595,7 @@ usage ()
>   		params.cmdname);
>   	fprintf (stderr, "       %s [-D dtc_options] -f fit-image.its fit-image\n",
>   		params.cmdname);
> +	fprintf (stderr, "       %s -V ==>  print version information and exit\n");
>
>   	exit (EXIT_FAILURE);
>   }

Amicalement,
-- 
Albert.

[U-Boot] [PATCH v2] mkimage: add "-V" option to print version information

[Albert ARIBAUD]

  Wolfgang,

Please ignore my previous post on V1, I had not seen V2. My comment 
holds, though:

Le 11/02/2011 13:22, Wolfgang Denk a ?crit :
> Signed-off-by: Wolfgang Denk<wd@denx.de>
> ---
> v2: fix missing argument to printf() call.
>
>   tools/mkimage.c |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index f5859d7..127be57 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -23,6 +23,7 @@
>
>   #include "mkimage.h"
>   #include<image.h>
> +#include<version.h>
>
>   static void copy_file(int, const char *, int);
>   static void usage(void);
> @@ -246,6 +247,10 @@ main (int argc, char **argv)
>   			case 'v':
>   				params.vflag++;
>   				break;
> +			case 'V':
> +				printf("mkimage version %s\n",
> +					U_BOOT_VERSION + 7);

If that magic number 7 (and the addition, as well) has any reason, it 
should at least be explained in a short comment.

> +				exit(EXIT_SUCCESS);
>   			case 'x':
>   				params.xflag++;
>   				break;
> @@ -590,6 +595,8 @@ usage ()
>   		params.cmdname);
>   	fprintf (stderr, "       %s [-D dtc_options] -f fit-image.its fit-image\n",
>   		params.cmdname);
> +	fprintf (stderr, "       %s -V ==>  print version information and exit\n",
> +		params.cmdname);
>
>   	exit (EXIT_FAILURE);
>   }


Amicalement,
-- 
Albert.

[U-Boot] [PATCH v2] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Dear Albert ARIBAUD,

In message <4D55B03A.9030602@free.fr> you wrote:
> 
> Please ignore my previous post on V1, I had not seen V2. My comment
> holds, though:
...
> > +			case 'V':
> > +				printf("mkimage version %s\n",
> > +					U_BOOT_VERSION + 7);
>
> If that magic number 7 (and the addition, as well) has any reason, it >
> should at least be explained in a short comment.

Heh... I though I could leave this as an exercide for the reader ;-)

New version following...


Thanks for pointing out.

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
"The hottest places in Hell are reserved for those who, in  times  of
moral crisis, preserved their neutrality."                    - Dante

[U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
v2: fix missing argument to printf() call.
v3: explain the magic "+ 7" offset into the version string

 tools/mkimage.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index f5859d7..febb536 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -23,6 +23,7 @@
 
 #include "mkimage.h"
 #include <image.h>
+#include <version.h>
 
 static void copy_file(int, const char *, int);
 static void usage(void);
@@ -246,6 +247,14 @@ main (int argc, char **argv)
 			case 'v':
 				params.vflag++;
 				break;
+			case 'V':
+				/*
+				 * Skip the "U-Boot " part in
+				 * U_BOOT_VERSION by adding 7
+				 */
+				printf("mkimage version %s\n",
+					U_BOOT_VERSION + 7);
+				exit(EXIT_SUCCESS);
 			case 'x':
 				params.xflag++;
 				break;
@@ -590,6 +599,8 @@ usage ()
 		params.cmdname);
 	fprintf (stderr, "       %s [-D dtc_options] -f fit-image.its fit-image\n",
 		params.cmdname);
+	fprintf (stderr, "       %s -V ==> print version information and exit\n",
+		params.cmdname);
 
 	exit (EXIT_FAILURE);
 }
-- 
1.7.4

[U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

[Kim Phillips]

On Fri, 11 Feb 2011 23:41:43 +0100
Wolfgang Denk <wd@denx.de> wrote:

> +			case 'V':
> +				/*
> +				 * Skip the "U-Boot " part in
> +				 * U_BOOT_VERSION by adding 7
> +				 */
> +				printf("mkimage version %s\n",
> +					U_BOOT_VERSION + 7);

I'd have done it without magic nor comments as

U_BOOT_VERSION[sizeof("U-Boot ") + 1]

or even

U_BOOT_VERSION[strlen("U-Boot ")]

Kim

[U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

[Albert ARIBAUD]

Le 12/02/2011 00:11, Kim Phillips a ?crit :
> On Fri, 11 Feb 2011 23:41:43 +0100
> Wolfgang Denk<wd@denx.de>  wrote:
>
>> +			case 'V':
>> +				/*
>> +				 * Skip the "U-Boot " part in
>> +				 * U_BOOT_VERSION by adding 7
>> +				 */
>> +				printf("mkimage version %s\n",
>> +					U_BOOT_VERSION + 7);

Now I get it. :)

I might argue that this is kind of a hack, and that rather than trying 
to prune the U_BOOT_VERSION string, one should define two macros, for 
instance:

	#define PLAIN_VERSION "whatever"
	#define U_BOOT_VERSION "U-Boot " PLAIN_VERSION

... which has the advantage of not affecting the existing code, or a 
cleaner, but more invasive, change...

	#define U_BOOT_VERSION "whatever" /* without "U-Boot " */
	#define U_BOOT_VERSION_BANNER "U-Boot " U_BOOT_VERSION

... and use the right macro for the right need.

> I'd have done it without magic nor comments as
>
> U_BOOT_VERSION[sizeof("U-Boot ") + 1]
>
> or even
>
> U_BOOT_VERSION[strlen("U-Boot ")]

The second one calls strlen() at run-time, plus it allocates the "U-Boot 
" string for no justifiable reason -- assuming the first one can be 
compile-time evaluated by the compiler, of course.

> Kim

Amicalement,
-- 
Albert.

[U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Dear Albert ARIBAUD,

In message <4D562D0A.9060800@free.fr> you wrote:
>
> I might argue that this is kind of a hack, and that rather than trying 
> to prune the U_BOOT_VERSION string, one should define two macros, for 
> instance:
> 
> 	#define PLAIN_VERSION "whatever"
> 	#define U_BOOT_VERSION "U-Boot " PLAIN_VERSION

I know this works find in C, but I'm not absolutely sure if this is
acceptable with all assemblers.

But I get the idea...

Updated patch follows.

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
People seldom know what they want until you give them what  they  ask
for.

[U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
v2: fix missing argument to printf() call.
v3: explain the magic "+ 7" offset into the version string
v3: avoid offset into U_BOOT_VERSION string completely and define
    a new PLAIN_VERSION variable instead; this has the benefit that it
    can be used in other places as well where such version information
    might be needed (fw_{set,print}env etc. comes to mind).

 Makefile        |    8 ++++++--
 tools/mkimage.c |    6 ++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 05b404d..8721b59 100644
--- a/Makefile
+++ b/Makefile
@@ -414,8 +414,12 @@ $(U_BOOT_ONENAND):	$(ONENAND_IPL) $(obj)u-boot.bin
 		cat $(ONENAND_BIN) $(obj)u-boot.bin > $(obj)u-boot-onenand.bin
 
 $(VERSION_FILE):
-		@( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \
-		 '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp
+		@( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \
+		   printf '#define PLAIN_VERSION "%s%s"\n' \
+		   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
+		   printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \
+		   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
+		) > $@.tmp
 		@( printf '#define CC_VERSION_STRING "%s"\n' \
 		 '$(shell $(CC) --version | head -n 1)' )>>  $@.tmp
 		@( printf '#define LD_VERSION_STRING "%s"\n' \
diff --git a/tools/mkimage.c b/tools/mkimage.c
index f5859d7..60f7263 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -23,6 +23,7 @@
 
 #include "mkimage.h"
 #include <image.h>
+#include <version.h>
 
 static void copy_file(int, const char *, int);
 static void usage(void);
@@ -246,6 +247,9 @@ main (int argc, char **argv)
 			case 'v':
 				params.vflag++;
 				break;
+			case 'V':
+				printf("mkimage version %s\n", PLAIN_VERSION);
+				exit(EXIT_SUCCESS);
 			case 'x':
 				params.xflag++;
 				break;
@@ -590,6 +594,8 @@ usage ()
 		params.cmdname);
 	fprintf (stderr, "       %s [-D dtc_options] -f fit-image.its fit-image\n",
 		params.cmdname);
+	fprintf (stderr, "       %s -V ==> print version information and exit\n",
+		params.cmdname);
 
 	exit (EXIT_FAILURE);
 }
-- 
1.7.4

[U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Dear Kim Phillips,

In message <20110211171117.b9b05b01.kim.phillips@freescale.com> you wrote:
> On Fri, 11 Feb 2011 23:41:43 +0100
> Wolfgang Denk <wd@denx.de> wrote:
> 
> > +			case 'V':
> > +				/*
> > +				 * Skip the "U-Boot " part in
> > +				 * U_BOOT_VERSION by adding 7
> > +				 */
> > +				printf("mkimage version %s\n",
> > +					U_BOOT_VERSION + 7);
> 
> I'd have done it without magic nor comments as
> 
> U_BOOT_VERSION[sizeof("U-Boot ") + 1]
> 
> or even
> 
> U_BOOT_VERSION[strlen("U-Boot ")]

Hm... I would have cleaned up such locations in U-Boot as part of my
v4 patch that introduces PLAIN_VERSION - if I had found any such
code. Am I missing something?

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
"I haven't lost my mind - it's backed up on tape somewhere."

[U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

[Kim Phillips]

On Sat, 12 Feb 2011 07:47:38 +0100
Albert ARIBAUD <albert.aribaud@free.fr> wrote:

> Le 12/02/2011 00:11, Kim Phillips a ?crit :
> > I'd have done it without magic nor comments as
> >
> > U_BOOT_VERSION[sizeof("U-Boot ") + 1]
> >
> > or even
> >
> > U_BOOT_VERSION[strlen("U-Boot ")]
> 
> The second one calls strlen() at run-time, plus it allocates the "U-Boot 
> " string for no justifiable reason -- assuming the first one can be 
> compile-time evaluated by the compiler, of course.

no, the compiler evaluates the strlen at compile time and eliminates
the need to allocate the "U-Boot " string in both cases.

that's not to say that there aren't a couple of gaffes above: the
sizeof needs a - 1 instead of a + 1 (because it includes the trailing
'\0'), and both expressions need to be prepended with an '&'.  So, we
have something like:

&U_BOOT_VERSION[sizeof("U-Boot ") - 1]

or

&U_BOOT_VERSION[strlen("U-Boot ")]

Kim

[U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

[Kim Phillips]

On Sat, 12 Feb 2011 10:37:16 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Kim Phillips,
> 
> In message <20110211171117.b9b05b01.kim.phillips@freescale.com> you wrote:
> > On Fri, 11 Feb 2011 23:41:43 +0100
> > Wolfgang Denk <wd@denx.de> wrote:
> > 
> > > +			case 'V':
> > > +				/*
> > > +				 * Skip the "U-Boot " part in
> > > +				 * U_BOOT_VERSION by adding 7
> > > +				 */
> > > +				printf("mkimage version %s\n",
> > > +					U_BOOT_VERSION + 7);
> > 
> > I'd have done it without magic nor comments as
> > 
> > U_BOOT_VERSION[sizeof("U-Boot ") + 1]
> > 
> > or even
> > 
> > U_BOOT_VERSION[strlen("U-Boot ")]
> 
> Hm... I would have cleaned up such locations in U-Boot as part of my
> v4 patch that introduces PLAIN_VERSION - if I had found any such
> code. Am I missing something?

ok, apart from the missing & and +/- gaffes, what are you talking
about?  The code does exactly what the + 7 does, except it's more
self-explanatory.

Kim

[U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

[Kim Phillips]

On Sat, 12 Feb 2011 10:37:11 +0100
Wolfgang Denk <wd@denx.de> wrote:

> -		@( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \
> -		 '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp
> +		@( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \
> +		   printf '#define PLAIN_VERSION "%s%s"\n' \
> +		   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
> +		   printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \
> +		   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
> +		) > $@.tmp

IMO, PLAIN_VERSION isn't descriptive enough (should really be called
VERSION..?).  How about going with something like:

#define U_BOOT_STR "U-Boot"
#define U_BOOT_VERSION U_BOOT_STR " %s%s"...

and then

> +			case 'V':
> +				printf("mkimage version %s\n", PLAIN_VERSION);
> +				exit(EXIT_SUCCESS);

&U_BOOT_VERSION[sizeof(U_BOOT_STR)]

(the - 1 is not necessary since we want to include the ' ')

this maintains consistency and the fact that the mkimage version is
directly tied to it's parent project, U-Boot's, version number.

Kim

[U-Boot] [PATCH v3] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Dear Kim Phillips,

In message <20110212161715.111c368f.kim.phillips@freescale.com> you wrote:
>
> > > I'd have done it without magic nor comments as
...
> > Hm... I would have cleaned up such locations in U-Boot as part of my
> > v4 patch that introduces PLAIN_VERSION - if I had found any such
> > code. Am I missing something?
> 
> ok, apart from the missing & and +/- gaffes, what are you talking
> about?  The code does exactly what the + 7 does, except it's more
> self-explanatory.

Sorry, my fault. I've misread your "I'd have done it" as "I have done
it" but did not find any such code.  Please ignore me.

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
Ill-chosen abstraction is particularly evident in the design  of  the
ADA  runtime  system.  The  interface to the ADA runtime system is so
opaque that it is impossible to model  or  predict  its  performance,
making it effectively useless for real-time systems.
                              - Marc D.  Donner and David H. Jameson.

[U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Dear Kim Phillips,

In message <20110212171349.f0f5d472.kim.phillips@freescale.com> you wrote:
> 
> > -		@( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \
> > -		 '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp
> > +		@( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \
> > +		   printf '#define PLAIN_VERSION "%s%s"\n' \
> > +		   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
> > +		   printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \
> > +		   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
> > +		) > $@.tmp
> 
> IMO, PLAIN_VERSION isn't descriptive enough (should really be called
> VERSION..?).  How about going with something like:
> 
> #define U_BOOT_STR "U-Boot"
> #define U_BOOT_VERSION U_BOOT_STR " %s%s"...

No - not unless you guarantee that this syntax is compatible with all
assemblers that may be used to build U-Boot.

> and then
> 
> > +			case 'V':
> > +				printf("mkimage version %s\n", PLAIN_VERSION);
> > +				exit(EXIT_SUCCESS);
> 
> &U_BOOT_VERSION[sizeof(U_BOOT_STR)]
> 
> (the - 1 is not necessary since we want to include the ' ')

No again.  This is, um, ugly, and completely unnecessary.

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
Man is the best computer we can put aboard a spacecraft ...  and  the
only one that can be mass produced with unskilled labor.
                                                  - Wernher von Braun

[U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

[Kim Phillips]

On Sun, 13 Feb 2011 00:35:08 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Kim Phillips,
> 
> In message <20110212171349.f0f5d472.kim.phillips@freescale.com> you wrote:
> > 
> > > -		@( printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' "$(U_BOOT_VERSION)" \
> > > -		 '$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ) > $@.tmp
> > > +		@( localvers='$(shell $(TOPDIR)/tools/setlocalversion $(TOPDIR))' ; \
> > > +		   printf '#define PLAIN_VERSION "%s%s"\n' \
> > > +		   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
> > > +		   printf '#define U_BOOT_VERSION "U-Boot %s%s"\n' \
> > > +		   	"$(U_BOOT_VERSION)" "$${localvers}" ; \
> > > +		) > $@.tmp
> > 
> > IMO, PLAIN_VERSION isn't descriptive enough (should really be called
> > VERSION..?).  How about going with something like:
> > 
> > #define U_BOOT_STR "U-Boot"
> > #define U_BOOT_VERSION U_BOOT_STR " %s%s"...
> 
> No - not unless you guarantee that this syntax is compatible with all
> assemblers that may be used to build U-Boot.

I cannot do that, but, ok, not such a big deal:

/* prefix lengths must match */
#define U_BOOT_STR "U-Boot"
#define U_BOOT_VERSION "U-Boot %s%s"...

> > > +			case 'V':
> > > +				printf("mkimage version %s\n", PLAIN_VERSION);
> > > +				exit(EXIT_SUCCESS);
> > 
> > &U_BOOT_VERSION[sizeof(U_BOOT_STR)]
> > 
> > (the - 1 is not necessary since we want to include the ' ')
> 
> No again.  This is, um, ugly, and completely unnecessary.

that's a matter of personal taste, but IMO it's better than
PLAIN_VERSION (what's that? - VERSION_NUMBERONLY would be way more
descriptive).

If it's the &..[..] that's not appealing, feel free to do as the '+ 7'
code but as '+ sizeof(...)' (to maintain a not-so-completely
unnecessary clarity & consistency..but that's my opinion).

Kim

[U-Boot] [PATCH v4] mkimage: add "-V" option to print version information

[Wolfgang Denk]

Dear Wolfgang Denk,

In message <1297503431-19435-1-git-send-email-wd@denx.de> you wrote:
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> ---
> v2: fix missing argument to printf() call.
> v3: explain the magic "+ 7" offset into the version string
> v3: avoid offset into U_BOOT_VERSION string completely and define
>     a new PLAIN_VERSION variable instead; this has the benefit that it
>     can be used in other places as well where such version information
>     might be needed (fw_{set,print}env etc. comes to mind).
> 
>  Makefile        |    8 ++++++--
>  tools/mkimage.c |    6 ++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)

Applied.

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
If something is different, it's either better or worse,  and  usually
both.                                                    - Larry Wall