Return-path: Received: from mail-ve0-f171.google.com ([209.85.128.171]:43300 "EHLO mail-ve0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755921Ab3KFHta (ORCPT ); Wed, 6 Nov 2013 02:49:30 -0500 MIME-Version: 1.0 In-Reply-To: <1383684000.4387.42.camel@joe-AO722> References: <1383684000.4387.42.camel@joe-AO722> Date: Wed, 6 Nov 2013 07:49:29 +0000 Message-ID: (sfid-20131106_084956_858904_73E4EC53) Subject: Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon From: Eugene Krasnikov To: Joe Perches Cc: "John W. Linville" , wcn36xx , linux-wireless , netdev@vger.kernel.org, "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-wireless-owner@vger.kernel.org List-ID: Hm... when it comes to semicolon the patch seems to be good. When it comes to dynamic debugging i think we should have a separate discussion about that. I personally like the whole idea about dynamic debug but if you want to change it i would suggest to have some kind of framework for all ath drivers(or maybe all wireless drivers). Because obviously you can find common code in every driver that defines it's own debug functions/debug levels and so on. Why not to make a framework with standard API/levels? On Tue, Nov 5, 2013 at 8:40 PM, Joe Perches wrote: > The wcn36xx_err macro should not end in a semicolon as > there are 2 consecutive semicolons in the preprocessed > output. > > Remove it. > > Neaten the logging macros to use a more common style. > Convert to use pr_debug to enable dynamic debug. > > Signed-off-by: Joe Perches > --- > drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 28 +++++++++++++--------------- > 1 file changed, 13 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > index 58b6383..d157bb7 100644 > --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h > @@ -53,22 +53,20 @@ enum wcn36xx_debug_mask { > WCN36XX_DBG_ANY = 0xffffffff, > }; > > -#define wcn36xx_err(fmt, arg...) \ > - printk(KERN_ERR pr_fmt("ERROR " fmt), ##arg); > - > -#define wcn36xx_warn(fmt, arg...) \ > - printk(KERN_WARNING pr_fmt("WARNING " fmt), ##arg) > - > -#define wcn36xx_info(fmt, arg...) \ > - printk(KERN_INFO pr_fmt(fmt), ##arg) > - > -#define wcn36xx_dbg(mask, fmt, arg...) do { \ > - if (wcn36xx_dbg_mask & mask) \ > - printk(KERN_DEBUG pr_fmt(fmt), ##arg); \ > +#define wcn36xx_err(fmt, ...) \ > + pr_err("ERROR " fmt, ##__VA_ARGS__) > +#define wcn36xx_warn(fmt, ...) \ > + pr_warn("WARNING " fmt, ##__VA_ARGS__) > +#define wcn36xx_info(fmt, ...) \ > + pr_info(fmt, ##__VA_ARGS__) > +#define wcn36xx_dbg(mask, fmt, ...) \ > +do { \ > + if (wcn36xx_dbg_mask & mask) \ > + pr_debug(fmt, ##__VA_ARGS__); \ > } while (0) > - > -#define wcn36xx_dbg_dump(mask, prefix_str, buf, len) do { \ > - if (wcn36xx_dbg_mask & mask) \ > +#define wcn36xx_dbg_dump(mask, prefix_str, buf, len) \ > +do { \ > + if (wcn36xx_dbg_mask & mask) \ > print_hex_dump(KERN_DEBUG, pr_fmt(prefix_str), \ > DUMP_PREFIX_OFFSET, 32, 1, \ > buf, len, false); \ > > -- Best regards, Eugene