[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Mike Frysinger]

If we move the local funcs to the top of the file, and use the
__maybe_unused define, we can drop a lot of ugly ifdef logic and
duplicated prototypes.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 common/cmd_bdinfo.c |   89 ++++++++++++++++++--------------------------------
 1 files changed, 32 insertions(+), 57 deletions(-)

diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
index 688b238..67cb0da 100644
--- a/common/cmd_bdinfo.c
+++ b/common/cmd_bdinfo.c
@@ -26,24 +26,45 @@
  */
 #include <common.h>
 #include <command.h>
+#include <linux/compiler.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static void print_num(const char *, ulong);
+__maybe_unused
+static void print_num(const char *name, ulong value)
+{
+	printf("%-12s= 0x%08lX\n", name, value);
+}
 
-#if !(defined(CONFIG_ARM) || defined(CONFIG_M68K) || defined(CONFIG_SANDBOX)) \
-	|| defined(CONFIG_CMD_NET)
-#define HAVE_PRINT_ETH
-static void print_eth(int idx);
-#endif
+__maybe_unused
+static void print_eth(int idx)
+{
+	char name[10], *val;
+	if (idx)
+		sprintf(name, "eth%iaddr", idx);
+	else
+		strcpy(name, "ethaddr");
+	val = getenv(name);
+	if (!val)
+		val = "(not set)";
+	printf("%-12s= %s\n", name, val);
+}
 
-#if (!defined(CONFIG_ARM) && !defined(CONFIG_X86) && !defined(CONFIG_SANDBOX))
-#define HAVE_PRINT_LNUM
-static void print_lnum(const char *, u64);
-#endif
+__maybe_unused
+static void print_lnum(const char *name, u64 value)
+{
+	printf("%-12s= 0x%.8llX\n", name, value);
+}
+
+__maybe_unused
+static void print_mhz(const char *name, unsigned long hz)
+{
+	char buf[32];
+
+	printf("%-12s= %6s MHz\n", name, strmhz(buf, hz));
+}
 
 #if defined(CONFIG_PPC)
-static void print_mhz(const char *, unsigned long);
 
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
@@ -208,8 +229,6 @@ int do_bdinfo(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_M68K)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	bd_t *bd = gd->bd;
@@ -257,8 +276,6 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_BLACKFIN)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	bd_t *bd = gd->bd;
@@ -377,8 +394,6 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_X86)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i;
@@ -464,46 +479,6 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  #error "a case for this architecture does not exist!"
 #endif
 
-static void print_num(const char *name, ulong value)
-{
-	printf("%-12s= 0x%08lX\n", name, value);
-}
-
-#ifdef HAVE_PRINT_ETH
-static void print_eth(int idx)
-{
-	char name[10], *val;
-	if (idx)
-		sprintf(name, "eth%iaddr", idx);
-	else
-		strcpy(name, "ethaddr");
-	val = getenv(name);
-	if (!val)
-		val = "(not set)";
-	printf("%-12s= %s\n", name, val);
-}
-#endif
-
-#ifdef HAVE_PRINT_LNUM
-static void print_lnum(const char *name, u64 value)
-{
-	printf("%-12s= 0x%.8llX\n", name, value);
-}
-#endif
-
-#if	defined(CONFIG_PPC) || \
-	defined(CONFIG_M68K) || \
-	defined(CONFIG_BLACKFIN) || \
-	defined(CONFIG_X86)
-static void print_mhz(const char *name, unsigned long hz)
-{
-	char buf[32];
-
-	printf("%-12s= %6s MHz\n", name, strmhz(buf, hz));
-}
-#endif	/* CONFIG_PPC */
-
-
 /* -------------------------------------------------------------------- */
 
 U_BOOT_CMD(
-- 
1.7.6.1

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Simon Glass]

On Sun, Oct 30, 2011 at 5:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> If we move the local funcs to the top of the file, and use the
> __maybe_unused define, we can drop a lot of ugly ifdef logic and
> duplicated prototypes.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

This is much cleaner - is the correct style to put attribute tags on
the previous line?

Acked-by: Simon Glass <sjg@chromium.org>

> ---
> ?common/cmd_bdinfo.c | ? 89 ++++++++++++++++++--------------------------------
> ?1 files changed, 32 insertions(+), 57 deletions(-)
>

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Mike Frysinger]

On Monday 31 October 2011 15:11:35 Simon Glass wrote:
> On Sun, Oct 30, 2011 at 5:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > If we move the local funcs to the top of the file, and use the
> > __maybe_unused define, we can drop a lot of ugly ifdef logic and
> > duplicated prototypes.
> > 
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> 
> This is much cleaner - is the correct style to put attribute tags on
> the previous line?

when responding to add your own, there isn't any real protocol.  just normal 
e-mail etiquette (no top posting/etc...).  patchwork/humans will do the right 
thing when manually updating the changelog.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111031/5fb80343/attachment.pgp 

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Simon Glass]

Hi Mike,

On Mon, Oct 31, 2011 at 1:44 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 31 October 2011 15:11:35 Simon Glass wrote:
>> On Sun, Oct 30, 2011 at 5:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> > If we move the local funcs to the top of the file, and use the
>> > __maybe_unused define, we can drop a lot of ugly ifdef logic and
>> > duplicated prototypes.
>> >
>> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>>
>> This is much cleaner - is the correct style to put attribute tags on
>> the previous line?
>
> when responding to add your own, there isn't any real protocol. ?just normal
> e-mail etiquette (no top posting/etc...). ?patchwork/humans will do the right
> thing when manually updating the changelog.
> -mike
>

Actually I meant the __maybe_unused tag before the function name.

Regards,
Simon

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Tabi Timur-B04825]

On Sun, Oct 30, 2011 at 7:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>
>
> -static void print_num(const char *, ulong);
> +__maybe_unused
> +static void print_num(const char *name, ulong value)
> +{
> + ? ? ? printf("%-12s= 0x%08lX\n", name, value);
> +}
>

Will the linker remove the functions from the binary if they are unusued?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Mike Frysinger]

On Monday 31 October 2011 16:47:08 Simon Glass wrote:
> On Mon, Oct 31, 2011 at 1:44 PM, Mike Frysinger wrote:
> > On Monday 31 October 2011 15:11:35 Simon Glass wrote:
> >> On Sun, Oct 30, 2011 at 5:54 PM, Mike Frysinger wrote:
> >> > If we move the local funcs to the top of the file, and use the
> >> > __maybe_unused define, we can drop a lot of ugly ifdef logic and
> >> > duplicated prototypes.
> >> > 
> >> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> >> 
> >> This is much cleaner - is the correct style to put attribute tags on
> >> the previous line?
> > 
> > when responding to add your own, there isn't any real protocol.  just
> > normal e-mail etiquette (no top posting/etc...).  patchwork/humans will
> > do the right thing when manually updating the changelog.
> 
> Actually I meant the __maybe_unused tag before the function name.

ah.  i'm not sure there is a hard rule here.  i did that because one line 
would make the func def too long to fit.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111031/0b6ba257/attachment.pgp 

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Simon Glass]

On Mon, Oct 31, 2011 at 2:15 PM, Tabi Timur-B04825 <B04825@freescale.com> wrote:
> On Sun, Oct 30, 2011 at 7:54 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>>
>>
>> -static void print_num(const char *, ulong);
>> +__maybe_unused
>> +static void print_num(const char *name, ulong value)
>> +{
>> + ? ? ? printf("%-12s= 0x%08lX\n", name, value);
>> +}
>>
>
> Will the linker remove the functions from the binary if they are unusued?

If built with -ffunction-sections and --gc-sections,, then the linker
can do this sort of thing. Otherwise it can't, but the compiler can. I
just tested Mike's code on my ARM compiler to make sure and it happily
removed print_eth() when it was not used.

Regards,
Simon

>
> --
> Timur Tabi
> Linux kernel developer at Freescale
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Mike Frysinger]

On Monday 31 October 2011 17:49:58 Simon Glass wrote:
> On Mon, Oct 31, 2011 at 2:15 PM, Tabi Timur-B04825 wrote:
> > On Sun, Oct 30, 2011 at 7:54 PM, Mike Frysinger wrote:
> >> -static void print_num(const char *, ulong);
> >> +__maybe_unused
> >> +static void print_num(const char *name, ulong value)
> >> +{
> >> +       printf("%-12s= 0x%08lX\n", name, value);
> >> +}
> > 
> > Will the linker remove the functions from the binary if they are unusued?
> 
> If built with -ffunction-sections and --gc-sections,, then the linker
> can do this sort of thing. Otherwise it can't, but the compiler can. I
> just tested Mike's code on my ARM compiler to make sure and it happily
> removed print_eth() when it was not used.

i don't think you need function-sections to make this happen.  since it is 
marked "static", gcc should do DCE on it.  the __maybe_unused markings is just 
to kill off any warnings about the func not being used (which might occur in 
the #ifdef jungle below).  that attribute does not tell gcc to retain the 
function even if it isn't referenced in this file (as far as gcc can tell).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111031/233f254c/attachment.pgp 

[U-Boot] [PATCH] cmd_bdinfo: simplify local static funcs a bit

[Simon Glass]

Hi Mike,

On Mon, Oct 31, 2011 at 3:37 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 31 October 2011 17:49:58 Simon Glass wrote:
>> On Mon, Oct 31, 2011 at 2:15 PM, Tabi Timur-B04825 wrote:
>> > On Sun, Oct 30, 2011 at 7:54 PM, Mike Frysinger wrote:
>> >> -static void print_num(const char *, ulong);
>> >> +__maybe_unused
>> >> +static void print_num(const char *name, ulong value)
>> >> +{
>> >> + ? ? ? printf("%-12s= 0x%08lX\n", name, value);
>> >> +}
>> >
>> > Will the linker remove the functions from the binary if they are unusued?
>>
>> If built with -ffunction-sections and --gc-sections,, then the linker
>> can do this sort of thing. Otherwise it can't, but the compiler can. I
>> just tested Mike's code on my ARM compiler to make sure and it happily
>> removed print_eth() when it was not used.
>
> i don't think you need function-sections to make this happen. ?since it is
> marked "static", gcc should do DCE on it. ?the __maybe_unused markings is just
> to kill off any warnings about the func not being used (which might occur in
> the #ifdef jungle below). ?that attribute does not tell gcc to retain the
> function even if it isn't referenced in this file (as far as gcc can tell).
> -mike
>

That's right, you don't need function-sections for the compiler to
eliminate the code - my point was that the linker can't do this sort
of thing...luckily it doesn't need to.

There might be an option to control this, a bit like
-fkeep-static-consts, but I can't see it. Can't see it being very
useful anyway.

Regards,
Simon

[U-Boot] [PATCH v2] cmd_bdinfo: simplify local static funcs a bit

[Mike Frysinger]

If we move the local funcs to the top of the file, and use the
__maybe_unused define, we can drop a lot of ugly ifdef logic and
duplicated prototypes.

Acked-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- rebased onto current master

 common/cmd_bdinfo.c |   89 ++++++++++++++++++---------------------------------
 1 files changed, 31 insertions(+), 58 deletions(-)

diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
index 6c48594..73c03a8 100644
--- a/common/cmd_bdinfo.c
+++ b/common/cmd_bdinfo.c
@@ -26,25 +26,44 @@
  */
 #include <common.h>
 #include <command.h>
+#include <linux/compiler.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
-static void print_num(const char *, ulong);
+__maybe_unused
+static void print_num(const char *name, ulong value)
+{
+	printf("%-12s= 0x%08lX\n", name, value);
+}
 
-#if !(defined(CONFIG_ARM) || defined(CONFIG_M68K) || \
-	defined(CONFIG_SANDBOX) || defined(CONFIG_X86)) \
-	|| defined(CONFIG_CMD_NET)
-#define HAVE_PRINT_ETH
-static void print_eth(int idx);
-#endif
+static void print_eth(int idx)
+{
+	char name[10], *val;
+	if (idx)
+		sprintf(name, "eth%iaddr", idx);
+	else
+		strcpy(name, "ethaddr");
+	val = getenv(name);
+	if (!val)
+		val = "(not set)";
+	printf("%-12s= %s\n", name, val);
+}
 
-#if (!defined(CONFIG_ARM) && !defined(CONFIG_X86) && !defined(CONFIG_SANDBOX))
-#define HAVE_PRINT_LNUM
-static void print_lnum(const char *, u64);
-#endif
+__maybe_unused
+static void print_lnum(const char *name, u64 value)
+{
+	printf("%-12s= 0x%.8llX\n", name, value);
+}
+
+__maybe_unused
+static void print_mhz(const char *name, unsigned long hz)
+{
+	char buf[32];
+
+	printf("%-12s= %6s MHz\n", name, strmhz(buf, hz));
+}
 
 #if defined(CONFIG_PPC)
-static void print_mhz(const char *, unsigned long);
 
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
@@ -209,8 +228,6 @@ int do_bdinfo(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_M68K)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	bd_t *bd = gd->bd;
@@ -258,8 +275,6 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_BLACKFIN)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	bd_t *bd = gd->bd;
@@ -378,8 +393,6 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 #elif defined(CONFIG_X86)
 
-static void print_mhz(const char *, unsigned long);
-
 int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	int i;
@@ -465,46 +478,6 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
  #error "a case for this architecture does not exist!"
 #endif
 
-static void print_num(const char *name, ulong value)
-{
-	printf("%-12s= 0x%08lX\n", name, value);
-}
-
-#ifdef HAVE_PRINT_ETH
-static void print_eth(int idx)
-{
-	char name[10], *val;
-	if (idx)
-		sprintf(name, "eth%iaddr", idx);
-	else
-		strcpy(name, "ethaddr");
-	val = getenv(name);
-	if (!val)
-		val = "(not set)";
-	printf("%-12s= %s\n", name, val);
-}
-#endif
-
-#ifdef HAVE_PRINT_LNUM
-static void print_lnum(const char *name, u64 value)
-{
-	printf("%-12s= 0x%.8llX\n", name, value);
-}
-#endif
-
-#if	defined(CONFIG_PPC) || \
-	defined(CONFIG_M68K) || \
-	defined(CONFIG_BLACKFIN) || \
-	defined(CONFIG_X86)
-static void print_mhz(const char *name, unsigned long hz)
-{
-	char buf[32];
-
-	printf("%-12s= %6s MHz\n", name, strmhz(buf, hz));
-}
-#endif	/* CONFIG_PPC */
-
-
 /* -------------------------------------------------------------------- */
 
 U_BOOT_CMD(
-- 
1.7.6.1

[U-Boot] [PATCH v2] cmd_bdinfo: simplify local static funcs a bit

[Wolfgang Denk]

Dear Mike Frysinger,

In message <1323056722-22453-1-git-send-email-vapier@gentoo.org> you wrote:
> If we move the local funcs to the top of the file, and use the
> __maybe_unused define, we can drop a lot of ugly ifdef logic and
> duplicated prototypes.
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> v2
> 	- rebased onto current master
> 
>  common/cmd_bdinfo.c |   89 ++++++++++++++++++---------------------------------
>  1 files changed, 31 insertions(+), 58 deletions(-)

Applied, thanks.

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've seen it. It's rubbish."          - Marvin the Paranoid Android