Re: [PATCH] [staging] bcm: Fix codingstyle problems
From: devendra.aaru
Date: Mon May 28 2012 - 03:22:33 EST
Hello Joe,
Thanks for a very quick reply with a code suggestion.
Just a small change at your code,
On Mon, May 28, 2012 at 10:07 AM, Joe Perches <joe@xxxxxxxxxxx> wrote:
>
> I think it'd be better with 3 functions.
>
> Maybe something like the below:
>
> But read the TODO file. I'm not sure if
> there's any value in working on bcm at all.
> ---
>
> create mode 100644 drivers/staging/bcm/Debug.c
>
> diff --git a/drivers/staging/bcm/Debug.c b/drivers/staging/bcm/Debug.c
> new file mode 100644
> index 0000000..3ca720c
> --- /dev/null
> +++ b/drivers/staging/bcm/Debug.c
> @@ -0,0 +1,53 @@
> +#include "headers.h"
/**
* for the BCM_DEBUG_PRINT macro
*/
+#include "debug.h"
> +
> +void bcm_debug_print(PMINI_ADAPTER Adapter, int Type, int SubType,
> + int dbg_level, const char *fmt, ...)
> +{
> + struct va_format vaf;
> + va_list args;
> +
> + va_start(args, fmt);
> +
> + vaf.fmt = fmt;
> + vaf.va = &args;
> +
> + if (DBG_TYPE_PRINTK == Type)
> + pr_info("%s: %pV", __func__, &vaf);
> + else if (Adapter &&
> + (dbg_level & DBG_LVL_BITMASK) <= Adapter->stDebugState.debug_level &&
> + (Type & Adapter->stDebugState.type) &&
> + (SubType & Adapter->stDebugState.subtype[Type])) {
> + if (dbg_level & DBG_NO_FUNC_PRINT)
> + printk(KERN_DEBUG "%pV" , &vaf);
> + else
> + printk(KERN_DEBUG "%s: %pV", __func__, &vaf);
> + }
> +
> + va_end(args);
> +}
> +
> +void bcm_debug_print_buffer(PMINI_ADAPTER Adapter, int Type, int SubType,
> + int dbg_level, const void *buffer, size_t bufferlen)
> +{
> + if (DBG_TYPE_PRINTK == Type ||
> + (Adapter &&
> + (dbg_level & DBG_LVL_BITMASK) <= Adapter->stDebugState.debug_level &&
> + (Type & Adapter->stDebugState.type) &&
> + (SubType & Adapter->stDebugState.subtype[Type]))) {
> + printk(KERN_DEBUG "%s:\n", __func__);
> + print_hex_dump(KERN_DEBUG, " ", DUMP_PREFIX_OFFSET,
> + 16, 1, buffer, bufferlen, false);
> + }
> +}
> +
> +void bcm_show_debug_bitmap(PMINI_ADAPTER Adapter)
> +{
> + int i;
> + for (i = 0; i < (NUMTYPES * 2) + 1; i++) {
> + if ((i == 1) || (i == 2) || (i == 4) || (i == 8)) {
> + BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0,
> + "subtype[%d] = 0x%08x\n",
> + i, Adapter->stDebugState.subtype[i]);
> + }
> + }
> +}
> diff --git a/drivers/staging/bcm/Debug.h b/drivers/staging/bcm/Debug.h
> index 420382d..0fbb206 100644
> --- a/drivers/staging/bcm/Debug.h
> +++ b/drivers/staging/bcm/Debug.h
> @@ -203,57 +203,35 @@ typedef struct _S_BCM_DEBUG_STATE {
> * corresponding to valid Type values. Hence we use the 'Type' field
> * as the index value, ignoring the array entries 0,3,5,6,7 !
> */
> - UINT subtype[(NUMTYPES*2)+1];
> + UINT subtype[(NUMTYPES * 2) + 1];
> UINT debug_level;
> } S_BCM_DEBUG_STATE;
> /* Instantiated in the Adapter structure */
> /* We'll reuse the debug level parameter to include a bit (the MSB) to indicate whether or not
> * we want the function's name printed. */
> -#define DBG_NO_FUNC_PRINT 1 << 31
> +#define DBG_NO_FUNC_PRINT (1 << 31)
> #define DBG_LVL_BITMASK 0xFF
>
> //--- Only for direct printk's; "hidden" to API.
> #define DBG_TYPE_PRINTK 3
>
> -#define BCM_DEBUG_PRINT(Adapter, Type, SubType, dbg_level, string, args...) \
> - do { \
> - if (DBG_TYPE_PRINTK == Type) \
> - pr_info("%s:" string, __func__, ##args); \
> - else if (Adapter && \
> - (dbg_level & DBG_LVL_BITMASK) <= Adapter->stDebugState.debug_level && \
> - (Type & Adapter->stDebugState.type) && \
> - (SubType & Adapter->stDebugState.subtype[Type])) { \
> - if (dbg_level & DBG_NO_FUNC_PRINT) \
> - printk(KERN_DEBUG string, ##args); \
> - else \
> - printk(KERN_DEBUG "%s:" string, __func__, ##args); \
> - } \
> - } while (0)
> -
> -#define BCM_DEBUG_PRINT_BUFFER(Adapter, Type, SubType, dbg_level, buffer, bufferlen) do { \
> - if (DBG_TYPE_PRINTK == Type || \
> - (Adapter && \
> - (dbg_level & DBG_LVL_BITMASK) <= Adapter->stDebugState.debug_level && \
> - (Type & Adapter->stDebugState.type) && \
> - (SubType & Adapter->stDebugState.subtype[Type]))) { \
> - printk(KERN_DEBUG "%s:\n", __func__); \
> - print_hex_dump(KERN_DEBUG, " ", DUMP_PREFIX_OFFSET, \
> - 16, 1, buffer, bufferlen, false); \
> - } \
> -} while(0)
> -
> -
> -#define BCM_SHOW_DEBUG_BITMAP(Adapter) do { \
> - int i; \
> - for (i=0; i<(NUMTYPES*2)+1; i++) { \
> - if ((i == 1) || (i == 2) || (i == 4) || (i == 8)) { \
> - /* CAUTION! Forcefully turn on ALL debug paths and subpaths! \
> - Adapter->stDebugState.subtype[i] = 0xffffffff; */ \
> - BCM_DEBUG_PRINT (Adapter, DBG_TYPE_PRINTK, 0, 0, "subtype[%d] = 0x%08x\n", \
> - i, Adapter->stDebugState.subtype[i]); \
> - } \
> - } \
> -} while (0)
> +struct _MINI_ADAPTER;
>
> -#endif
> +__printf(5, 6)
> +void bcm_debug_print(struct _MINI_ADAPTER *Adapter, int Type, int SubType,
> + int dbg_level, const char *fmt, ...);
> +void bcm_debug_print_buffer(struct _MINI_ADAPTER *Adapter, int Type, int SubType,
> + int dbg_level, const void *buffer, size_t bufferlen);
> +void bcm_show_debug_bitmap(struct _MINI_ADAPTER *Adapter);
> +
> +#define BCM_DEBUG_PRINT(Adapter, Type, SubType, dbg_level, fmt, ...) \
> + bcm_debug_print(Adapter, Type, SubType, dbg_level, fmt, ##__VA_ARGS__)
>
> +#define BCM_DEBUG_PRINT_BUFFER(Adapter, Type, SubType, dbg_level, \
> + buffer, bufferlen) \
> + bcm_debug_print_buffer(Adapter, Type, SubType, dbg_level, \
> + buffer, bufferlen)
> +#define BCM_SHOW_DEBUG_BITMAP(Adapter) \
> + bcm_show_debug_bitmap(Adapter)
> +
> +#endif
> diff --git a/drivers/staging/bcm/Makefile b/drivers/staging/bcm/Makefile
> index 652b7f8..a858ac1 100644
> --- a/drivers/staging/bcm/Makefile
> +++ b/drivers/staging/bcm/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_BCM_WIMAX) += bcm_wimax.o
> bcm_wimax-y := InterfaceDld.o InterfaceIdleMode.o InterfaceInit.o InterfaceRx.o \
> InterfaceIsr.o InterfaceMisc.o InterfaceTx.o \
> CmHost.o IPv6Protocol.o Qos.o Transmit.o\
> + Debug.o\
> Bcmnet.o DDRInit.o HandleControlPacket.o\
> LeakyBucket.o Misc.o sort.o Bcmchar.o hostmibs.o PHSModule.o\
> led_control.o nvm.o vendorspecificextn.o
> --
> 1.7.6.rc3
>
>
>
The TODO says these are developer debug prints, and may be set for
removal. I think its better to have macros, anyway they are going to
be removed in future.
Thanks,
Devendra.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/