diff mbox

[U-Boot,27/51] fpga: altera: Clean up the printing and debug

Message ID 1411305113-11649-1-git-send-email-marex@denx.de
State Superseded
Delegated to: Marek Vasut
Headers show

Commit Message

Marek Vasut Sept. 21, 2014, 1:11 p.m. UTC
Clean up the printf() statements and get rid of the PRINTF()
macro by replacing it with debug_cond().

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chin Liang See <clsee@altera.com>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
Cc: Pavel Machek <pavel@denx.de>
---
 drivers/fpga/altera.c | 117 +++++++++++++++++++++++++-------------------------
 1 file changed, 58 insertions(+), 59 deletions(-)

Comments

Michal Simek Sept. 24, 2014, 12:46 p.m. UTC | #1
On 09/21/2014 03:11 PM, Marek Vasut wrote:
> Clean up the printf() statements and get rid of the PRINTF()
> macro by replacing it with debug_cond().
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chin Liang See <clsee@altera.com>
> Cc: Dinh Nguyen <dinguyen@altera.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> Cc: Tom Rini <trini@ti.com>
> Cc: Wolfgang Denk <wd@denx.de>
> Cc: Pavel Machek <pavel@denx.de>
> ---
>  drivers/fpga/altera.c | 117 +++++++++++++++++++++++++-------------------------
>  1 file changed, 58 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/fpga/altera.c b/drivers/fpga/altera.c
> index 6e34a8e..ed3f0c8 100644
> --- a/drivers/fpga/altera.c
> +++ b/drivers/fpga/altera.c
> @@ -15,14 +15,8 @@
>  #include <ACEX1K.h>
>  #include <stratixII.h>
>  
> -/* Define FPGA_DEBUG to get debug printf's */
> -/* #define FPGA_DEBUG */
> -
> -#ifdef	FPGA_DEBUG
> -#define	PRINTF(fmt,args...)	printf (fmt ,##args)
> -#else
> -#define PRINTF(fmt,args...)
> -#endif
> +/* Define FPGA_DEBUG to 1 to get debug printf's */
> +#define FPGA_DEBUG	0
>  
>  /* Local Static Functions */
>  static int altera_validate (Altera_desc * desc, const char *fn);
> @@ -32,36 +26,39 @@ int altera_load(Altera_desc *desc, const void *buf, size_t bsize)
>  {
>  	int ret_val = FPGA_FAIL;	/* assume a failure */
>  
> -	if (!altera_validate (desc, (char *)__FUNCTION__)) {
> -		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
> +	if (!altera_validate (desc, (char *)__func__)) {
> +		printf("%s: Invalid device descriptor\n", __func__);
>  	} else {
>  		switch (desc->family) {
>  		case Altera_ACEX1K:
>  		case Altera_CYC2:
>  #if defined(CONFIG_FPGA_ACEX1K)
> -			PRINTF ("%s: Launching the ACEX1K Loader...\n",
> -					__FUNCTION__);
> +			debug_cond(FPGA_DEBUG,
> +				   "%s: Launching the ACEX1K Loader...\n",
> +				   __func__);
>  			ret_val = ACEX1K_load (desc, buf, bsize);
>  #elif defined(CONFIG_FPGA_CYCLON2)
> -			PRINTF ("%s: Launching the CYCLONE II Loader...\n",
> -					__FUNCTION__);
> +			debug_cond(FPGA_DEBUG,
> +				   "%s: Launching the CYCLONE II Loader...\n",
> +				   __func__);
>  			ret_val = CYC2_load (desc, buf, bsize);
>  #else
> -			printf ("%s: No support for ACEX1K devices.\n",
> -					__FUNCTION__);
> +			printf("%s: No support for ACEX1K devices.\n",
> +			       __func__);
>  #endif
>  			break;
>  
>  #if defined(CONFIG_FPGA_STRATIX_II)
>  		case Altera_StratixII:
> -			PRINTF ("%s: Launching the Stratix II Loader...\n",
> -				__FUNCTION__);
> +			debug_cond(FPGA_DEBUG,
> +				   "%s: Launching the Stratix II Loader...\n",
> +				   __func__);
>  			ret_val = StratixII_load (desc, buf, bsize);
>  			break;
>  #endif
>  		default:
> -			printf ("%s: Unsupported family type, %d\n",
> -					__FUNCTION__, desc->family);
> +			printf("%s: Unsupported family type, %d\n",
> +			       __func__, desc->family);
>  		}
>  	}
>  
> @@ -72,31 +69,33 @@ int altera_dump(Altera_desc *desc, const void *buf, size_t bsize)
>  {
>  	int ret_val = FPGA_FAIL;	/* assume a failure */
>  
> -	if (!altera_validate (desc, (char *)__FUNCTION__)) {
> -		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
> +	if (!altera_validate (desc, (char *)__func__)) {
> +		printf("%s: Invalid device descriptor\n", __func__);
>  	} else {
>  		switch (desc->family) {
>  		case Altera_ACEX1K:
>  #if defined(CONFIG_FPGA_ACEX)
> -			PRINTF ("%s: Launching the ACEX1K Reader...\n",
> -					__FUNCTION__);
> +			debug_cond(FPGA_DEBUG,
> +				   "%s: Launching the ACEX1K Reader...\n",
> +				   __func__);
>  			ret_val = ACEX1K_dump (desc, buf, bsize);
>  #else
> -			printf ("%s: No support for ACEX1K devices.\n",
> -					__FUNCTION__);
> +			printf("%s: No support for ACEX1K devices.\n",
> +			       __func__);
>  #endif
>  			break;
>  
>  #if defined(CONFIG_FPGA_STRATIX_II)
>  		case Altera_StratixII:
> -			PRINTF ("%s: Launching the Stratix II Reader...\n",
> -				__FUNCTION__);
> +			debug_cond(FPGA_DEBUG,
> +				   "%s: Launching the Stratix II Reader...\n",
> +				   __func__);
>  			ret_val = StratixII_dump (desc, buf, bsize);
>  			break;
>  #endif
>  		default:
> -			printf ("%s: Unsupported family type, %d\n",
> -					__FUNCTION__, desc->family);
> +			printf("%s: Unsupported family type, %d\n",
> +			       __func__, desc->family);
>  		}
>  	}
>  
> @@ -107,42 +106,42 @@ int altera_info( Altera_desc *desc )
>  {
>  	int ret_val = FPGA_FAIL;
>  
> -	if (altera_validate (desc, (char *)__FUNCTION__)) {
> -		printf ("Family:        \t");
> +	if (altera_validate (desc, (char *)__func__)) {
> +		printf("Family:        \t");
>  		switch (desc->family) {
>  		case Altera_ACEX1K:
> -			printf ("ACEX1K\n");
> +			printf("ACEX1K\n");
>  			break;
>  		case Altera_CYC2:
> -			printf ("CYCLON II\n");
> +			printf("CYCLON II\n");
>  			break;
>  		case Altera_StratixII:
> -			printf ("Stratix II\n");
> +			printf("Stratix II\n");
>  			break;
>  			/* Add new family types here */
>  		default:
> -			printf ("Unknown family type, %d\n", desc->family);
> +			printf("Unknown family type, %d\n", desc->family);
>  		}
>  
> -		printf ("Interface type:\t");
> +		printf("Interface type:\t");
>  		switch (desc->iface) {
>  		case passive_serial:
> -			printf ("Passive Serial (PS)\n");
> +			printf("Passive Serial (PS)\n");
>  			break;
>  		case passive_parallel_synchronous:
> -			printf ("Passive Parallel Synchronous (PPS)\n");
> +			printf("Passive Parallel Synchronous (PPS)\n");
>  			break;
>  		case passive_parallel_asynchronous:
> -			printf ("Passive Parallel Asynchronous (PPA)\n");
> +			printf("Passive Parallel Asynchronous (PPA)\n");
>  			break;
>  		case passive_serial_asynchronous:
> -			printf ("Passive Serial Asynchronous (PSA)\n");
> +			printf("Passive Serial Asynchronous (PSA)\n");
>  			break;
>  		case altera_jtag_mode:		/* Not used */
> -			printf ("JTAG Mode\n");
> +			printf("JTAG Mode\n");
>  			break;
>  		case fast_passive_parallel:
> -			printf ("Fast Passive Parallel (FPP)\n");
> +			printf("Fast Passive Parallel (FPP)\n");
>  			break;
>  		case fast_passive_parallel_security:
>  			printf
> @@ -150,31 +149,31 @@ int altera_info( Altera_desc *desc )
>  			break;
>  			/* Add new interface types here */
>  		default:
> -			printf ("Unsupported interface type, %d\n", desc->iface);
> +			printf("Unsupported interface type, %d\n", desc->iface);
>  		}
>  
>  		printf("Device Size:   \t%zd bytes\n"
> -		      "Cookie:        \t0x%x (%d)\n",
> -		      desc->size, desc->cookie, desc->cookie);
> +		       "Cookie:        \t0x%x (%d)\n",
> +		       desc->size, desc->cookie, desc->cookie);
>  
>  		if (desc->iface_fns) {
> -			printf ("Device Function Table @ 0x%p\n", desc->iface_fns);
> +			printf("Device Function Table @ 0x%p\n", desc->iface_fns);
>  			switch (desc->family) {
>  			case Altera_ACEX1K:
>  			case Altera_CYC2:
>  #if defined(CONFIG_FPGA_ACEX1K)
> -				ACEX1K_info (desc);
> +				ACEX1K_info(desc);
>  #elif defined(CONFIG_FPGA_CYCLON2)
> -				CYC2_info (desc);
> +				CYC2_info(desc);
>  #else
>  				/* just in case */
> -				printf ("%s: No support for ACEX1K devices.\n",
> -						__FUNCTION__);
> +				printf("%s: No support for ACEX1K devices.\n",
> +						__func__);
>  #endif
>  				break;
>  #if defined(CONFIG_FPGA_STRATIX_II)
>  			case Altera_StratixII:
> -				StratixII_info (desc);
> +				StratixII_info(desc);
>  				break;
>  #endif
>  				/* Add new family types here */
> @@ -183,12 +182,12 @@ int altera_info( Altera_desc *desc )
>  				break;
>  			}
>  		} else {
> -			printf ("No Device Function Table.\n");
> +			printf("No Device Function Table.\n");
>  		}
>  
>  		ret_val = FPGA_SUCCESS;
>  	} else {
> -		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
> +		printf("%s: Invalid device descriptor\n", __func__);
>  	}
>  
>  	return ret_val;
> @@ -208,17 +207,17 @@ static int altera_validate (Altera_desc * desc, const char *fn)
>  				if (desc->size) {
>  					ret_val = true;
>  				} else {
> -					printf ("%s: NULL part size\n", fn);
> +					printf("%s: NULL part size\n", fn);
>  				}
>  			} else {
> -				printf ("%s: Invalid Interface type, %d\n",
> -					fn, desc->iface);
> +				printf("%s: Invalid Interface type, %d\n",
> +				       fn, desc->iface);
>  			}
>  		} else {
> -			printf ("%s: Invalid family type, %d\n", fn, desc->family);
> +			printf("%s: Invalid family type, %d\n", fn, desc->family);
>  		}
>  	} else {
> -		printf ("%s: NULL descriptor!\n", fn);
> +		printf("%s: NULL descriptor!\n", fn);
>  	}
>  
>  	return ret_val;
> 


WARNING: space prohibited between function name and open parenthesis '('
#113: FILE: drivers/fpga/altera.c:29:
+	if (!altera_validate (desc, (char *)__func__)) {

WARNING: space prohibited between function name and open parenthesis '('
#165: FILE: drivers/fpga/altera.c:72:
+	if (!altera_validate (desc, (char *)__func__)) {

WARNING: space prohibited between function name and open parenthesis '('
#209: FILE: drivers/fpga/altera.c:109:
+	if (altera_validate (desc, (char *)__func__)) {

WARNING: line over 80 characters
#275: FILE: drivers/fpga/altera.c:160:
+			printf("Device Function Table @ 0x%p\n", desc->iface_fns);

CHECK: Alignment should match open parenthesis
#290: FILE: drivers/fpga/altera.c:171:
+				printf("%s: No support for ACEX1K devices.\n",
+						__func__);

WARNING: line over 80 characters
#330: FILE: drivers/fpga/altera.c:217:
+			printf("%s: Invalid family type, %d\n", fn, desc->family);


M
Marek Vasut Sept. 24, 2014, 1:22 p.m. UTC | #2
On Wednesday, September 24, 2014 at 02:46:17 PM, Michal Simek wrote:
> On 09/21/2014 03:11 PM, Marek Vasut wrote:
> > Clean up the printf() statements and get rid of the PRINTF()
> > macro by replacing it with debug_cond().
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Chin Liang See <clsee@altera.com>
> > Cc: Dinh Nguyen <dinguyen@altera.com>
> > Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> > Cc: Tom Rini <trini@ti.com>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Pavel Machek <pavel@denx.de>

[...]

> 
> WARNING: space prohibited between function name and open parenthesis '('
> #113: FILE: drivers/fpga/altera.c:29:
> +	if (!altera_validate (desc, (char *)__func__)) {
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #165: FILE: drivers/fpga/altera.c:72:
> +	if (!altera_validate (desc, (char *)__func__)) {
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #209: FILE: drivers/fpga/altera.c:109:
> +	if (altera_validate (desc, (char *)__func__)) {
> 
> WARNING: line over 80 characters
> #275: FILE: drivers/fpga/altera.c:160:
> +			printf("Device Function Table @ 0x%p\n", desc-
>iface_fns);
> 
> CHECK: Alignment should match open parenthesis
> #290: FILE: drivers/fpga/altera.c:171:
> +				printf("%s: No support for ACEX1K devices.\n",
> +						__func__);
> 
> WARNING: line over 80 characters
> #330: FILE: drivers/fpga/altera.c:217:
> +			printf("%s: Invalid family type, %d\n", fn, desc-
>family);

The patch does not address this issue, this is addressed by one of the patches 
further down the pipe (but it is addressed). I tried to keep these patches 
somewhat separated to keep the changes reasonably contained, but the file was a 
mess. The best suggestion I can give you is to use checkpatch -f on the 
resulting altera.c file ; there are still a few warnings, but it's much better 
than it was.

Best regards,
Marek Vasut
diff mbox

Patch

diff --git a/drivers/fpga/altera.c b/drivers/fpga/altera.c
index 6e34a8e..ed3f0c8 100644
--- a/drivers/fpga/altera.c
+++ b/drivers/fpga/altera.c
@@ -15,14 +15,8 @@ 
 #include <ACEX1K.h>
 #include <stratixII.h>
 
-/* Define FPGA_DEBUG to get debug printf's */
-/* #define FPGA_DEBUG */
-
-#ifdef	FPGA_DEBUG
-#define	PRINTF(fmt,args...)	printf (fmt ,##args)
-#else
-#define PRINTF(fmt,args...)
-#endif
+/* Define FPGA_DEBUG to 1 to get debug printf's */
+#define FPGA_DEBUG	0
 
 /* Local Static Functions */
 static int altera_validate (Altera_desc * desc, const char *fn);
@@ -32,36 +26,39 @@  int altera_load(Altera_desc *desc, const void *buf, size_t bsize)
 {
 	int ret_val = FPGA_FAIL;	/* assume a failure */
 
-	if (!altera_validate (desc, (char *)__FUNCTION__)) {
-		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
+	if (!altera_validate (desc, (char *)__func__)) {
+		printf("%s: Invalid device descriptor\n", __func__);
 	} else {
 		switch (desc->family) {
 		case Altera_ACEX1K:
 		case Altera_CYC2:
 #if defined(CONFIG_FPGA_ACEX1K)
-			PRINTF ("%s: Launching the ACEX1K Loader...\n",
-					__FUNCTION__);
+			debug_cond(FPGA_DEBUG,
+				   "%s: Launching the ACEX1K Loader...\n",
+				   __func__);
 			ret_val = ACEX1K_load (desc, buf, bsize);
 #elif defined(CONFIG_FPGA_CYCLON2)
-			PRINTF ("%s: Launching the CYCLONE II Loader...\n",
-					__FUNCTION__);
+			debug_cond(FPGA_DEBUG,
+				   "%s: Launching the CYCLONE II Loader...\n",
+				   __func__);
 			ret_val = CYC2_load (desc, buf, bsize);
 #else
-			printf ("%s: No support for ACEX1K devices.\n",
-					__FUNCTION__);
+			printf("%s: No support for ACEX1K devices.\n",
+			       __func__);
 #endif
 			break;
 
 #if defined(CONFIG_FPGA_STRATIX_II)
 		case Altera_StratixII:
-			PRINTF ("%s: Launching the Stratix II Loader...\n",
-				__FUNCTION__);
+			debug_cond(FPGA_DEBUG,
+				   "%s: Launching the Stratix II Loader...\n",
+				   __func__);
 			ret_val = StratixII_load (desc, buf, bsize);
 			break;
 #endif
 		default:
-			printf ("%s: Unsupported family type, %d\n",
-					__FUNCTION__, desc->family);
+			printf("%s: Unsupported family type, %d\n",
+			       __func__, desc->family);
 		}
 	}
 
@@ -72,31 +69,33 @@  int altera_dump(Altera_desc *desc, const void *buf, size_t bsize)
 {
 	int ret_val = FPGA_FAIL;	/* assume a failure */
 
-	if (!altera_validate (desc, (char *)__FUNCTION__)) {
-		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
+	if (!altera_validate (desc, (char *)__func__)) {
+		printf("%s: Invalid device descriptor\n", __func__);
 	} else {
 		switch (desc->family) {
 		case Altera_ACEX1K:
 #if defined(CONFIG_FPGA_ACEX)
-			PRINTF ("%s: Launching the ACEX1K Reader...\n",
-					__FUNCTION__);
+			debug_cond(FPGA_DEBUG,
+				   "%s: Launching the ACEX1K Reader...\n",
+				   __func__);
 			ret_val = ACEX1K_dump (desc, buf, bsize);
 #else
-			printf ("%s: No support for ACEX1K devices.\n",
-					__FUNCTION__);
+			printf("%s: No support for ACEX1K devices.\n",
+			       __func__);
 #endif
 			break;
 
 #if defined(CONFIG_FPGA_STRATIX_II)
 		case Altera_StratixII:
-			PRINTF ("%s: Launching the Stratix II Reader...\n",
-				__FUNCTION__);
+			debug_cond(FPGA_DEBUG,
+				   "%s: Launching the Stratix II Reader...\n",
+				   __func__);
 			ret_val = StratixII_dump (desc, buf, bsize);
 			break;
 #endif
 		default:
-			printf ("%s: Unsupported family type, %d\n",
-					__FUNCTION__, desc->family);
+			printf("%s: Unsupported family type, %d\n",
+			       __func__, desc->family);
 		}
 	}
 
@@ -107,42 +106,42 @@  int altera_info( Altera_desc *desc )
 {
 	int ret_val = FPGA_FAIL;
 
-	if (altera_validate (desc, (char *)__FUNCTION__)) {
-		printf ("Family:        \t");
+	if (altera_validate (desc, (char *)__func__)) {
+		printf("Family:        \t");
 		switch (desc->family) {
 		case Altera_ACEX1K:
-			printf ("ACEX1K\n");
+			printf("ACEX1K\n");
 			break;
 		case Altera_CYC2:
-			printf ("CYCLON II\n");
+			printf("CYCLON II\n");
 			break;
 		case Altera_StratixII:
-			printf ("Stratix II\n");
+			printf("Stratix II\n");
 			break;
 			/* Add new family types here */
 		default:
-			printf ("Unknown family type, %d\n", desc->family);
+			printf("Unknown family type, %d\n", desc->family);
 		}
 
-		printf ("Interface type:\t");
+		printf("Interface type:\t");
 		switch (desc->iface) {
 		case passive_serial:
-			printf ("Passive Serial (PS)\n");
+			printf("Passive Serial (PS)\n");
 			break;
 		case passive_parallel_synchronous:
-			printf ("Passive Parallel Synchronous (PPS)\n");
+			printf("Passive Parallel Synchronous (PPS)\n");
 			break;
 		case passive_parallel_asynchronous:
-			printf ("Passive Parallel Asynchronous (PPA)\n");
+			printf("Passive Parallel Asynchronous (PPA)\n");
 			break;
 		case passive_serial_asynchronous:
-			printf ("Passive Serial Asynchronous (PSA)\n");
+			printf("Passive Serial Asynchronous (PSA)\n");
 			break;
 		case altera_jtag_mode:		/* Not used */
-			printf ("JTAG Mode\n");
+			printf("JTAG Mode\n");
 			break;
 		case fast_passive_parallel:
-			printf ("Fast Passive Parallel (FPP)\n");
+			printf("Fast Passive Parallel (FPP)\n");
 			break;
 		case fast_passive_parallel_security:
 			printf
@@ -150,31 +149,31 @@  int altera_info( Altera_desc *desc )
 			break;
 			/* Add new interface types here */
 		default:
-			printf ("Unsupported interface type, %d\n", desc->iface);
+			printf("Unsupported interface type, %d\n", desc->iface);
 		}
 
 		printf("Device Size:   \t%zd bytes\n"
-		      "Cookie:        \t0x%x (%d)\n",
-		      desc->size, desc->cookie, desc->cookie);
+		       "Cookie:        \t0x%x (%d)\n",
+		       desc->size, desc->cookie, desc->cookie);
 
 		if (desc->iface_fns) {
-			printf ("Device Function Table @ 0x%p\n", desc->iface_fns);
+			printf("Device Function Table @ 0x%p\n", desc->iface_fns);
 			switch (desc->family) {
 			case Altera_ACEX1K:
 			case Altera_CYC2:
 #if defined(CONFIG_FPGA_ACEX1K)
-				ACEX1K_info (desc);
+				ACEX1K_info(desc);
 #elif defined(CONFIG_FPGA_CYCLON2)
-				CYC2_info (desc);
+				CYC2_info(desc);
 #else
 				/* just in case */
-				printf ("%s: No support for ACEX1K devices.\n",
-						__FUNCTION__);
+				printf("%s: No support for ACEX1K devices.\n",
+						__func__);
 #endif
 				break;
 #if defined(CONFIG_FPGA_STRATIX_II)
 			case Altera_StratixII:
-				StratixII_info (desc);
+				StratixII_info(desc);
 				break;
 #endif
 				/* Add new family types here */
@@ -183,12 +182,12 @@  int altera_info( Altera_desc *desc )
 				break;
 			}
 		} else {
-			printf ("No Device Function Table.\n");
+			printf("No Device Function Table.\n");
 		}
 
 		ret_val = FPGA_SUCCESS;
 	} else {
-		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
+		printf("%s: Invalid device descriptor\n", __func__);
 	}
 
 	return ret_val;
@@ -208,17 +207,17 @@  static int altera_validate (Altera_desc * desc, const char *fn)
 				if (desc->size) {
 					ret_val = true;
 				} else {
-					printf ("%s: NULL part size\n", fn);
+					printf("%s: NULL part size\n", fn);
 				}
 			} else {
-				printf ("%s: Invalid Interface type, %d\n",
-					fn, desc->iface);
+				printf("%s: Invalid Interface type, %d\n",
+				       fn, desc->iface);
 			}
 		} else {
-			printf ("%s: Invalid family type, %d\n", fn, desc->family);
+			printf("%s: Invalid family type, %d\n", fn, desc->family);
 		}
 	} else {
-		printf ("%s: NULL descriptor!\n", fn);
+		printf("%s: NULL descriptor!\n", fn);
 	}
 
 	return ret_val;