login
Search
Make the function name match the function purpose. ath_debug is a debug only facility. ath_print seems too generic a name for a debug only use. Removed an unnecessary trailing space in htc_drv_main.c Signed-off-by: Joe Perches <joe@perches.com> --- drivers/net/wireless/ath/ath9k/ahb.c | 2 +- drivers/net/wireless/ath/ath9k/ani.c | 24 +++--- drivers/net/wireless/ath/ath9k/ar5008_phy.c | 46 ++++++------ drivers/net/wireless/ath/ath9k/ar9002_calib.c | 78 ++++++++++---------- drivers/net/wireless/ath/ath9k/ar9002_hw.c | 2 +- drivers/net/wireless/ath/ath9k/ar9002_mac.c | 10 ++-- drivers/net/wireless/ath/ath9k/ar9003_calib.c | 76 ++++++++++---------- drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 68 +++++++++--------- drivers/net/wireless/ath/ath9k/ar9003_mac.c | 6 +- drivers/net/wireless/ath/ath9k/ar9003_phy.c | 38 +++++----- drivers/net/wireless/ath/ath9k/beacon.c | 36 +++++----- drivers/net/wireless/ath/ath9k/calib.c | 18 +++--- drivers/net/wireless/ath/ath9k/common.c | 2 +- drivers/net/wireless/ath/ath9k/eeprom.c | 2 +- drivers/net/wireless/ath/ath9k/eeprom_4k.c | 24 +++--- drivers/net/wireless/ath/ath9k/eeprom_9287.c | 18 +++--- drivers/net/wireless/ath/ath9k/eeprom_def.c | 24 +++--- drivers/net/wireless/ath/ath9k/gpio.c | 10 ++-- drivers/net/wireless/ath/ath9k/htc_drv_beacon.c | 10 ++-- drivers/net/wireless/ath/ath9k/htc_drv_gpio.c | 8 +- drivers/net/wireless/ath/ath9k/htc_drv_init.c | 28 ++++---- drivers/net/wireless/ath/ath9k/htc_drv_main.c | 82 +++++++++++----------- drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 16 ++-- drivers/net/wireless/ath/ath9k/hw.c | 78 ++++++++++---------- drivers/net/wireless/ath/ath9k/init.c | 12 ++-- drivers/net/wireless/ath/ath9k/mac.c | 58 ++++++++-------- drivers/net/wireless/ath/ath9k/main.c | 88 ...
Nack, I don't see the point. Luis --
The point is to improve readability. I like the patch. //Peter --
And how exactly does this improve readability? Don't get me wrong, I generally like to see more cleanups merged to the ath/ath9k drivers (they do need it, after all). But in my opinion, simple renaming churn like this does nothing but annoy people who want to get other work done at the same time. Consider the large number of lines touched (and the potential merge conflicts with other code because of that), relative to the microscopic aesthetic gain (if any). Instead I'd like to see more cleanups that go beyond trivial function renames. - Felix --
Thanks for putting it so clearly. Agreed. Luis --
It's considered polite to cc the patch author. print is generic, debug is specific. This function has a specific use for debugging not a generic use all for logging. If it was ath_print(level, etc) with KERN_<level> passed as the first argument that'd be similar This sort of thing can be done when other changes have I gauge my willingness to spend time on subsystems on the maintainers willingness to merge things that improve readability and correctness. cheers, Joe --
Sorry, I forgot to add you back. The Cc list was cleared somewhere in this thread and I added a few entries back, but apparently accidentally I'm not trying to discourage you from spending time on improving this code, just doubting the usefulness of such simple function renames. - Felix --
Poor function naming is just that. It reduces readability and the uses are counter expectation. --
The name is perfect, we use it to print anything, even non-debugging stuff. Luis --
If you are writing trivial patches please be ready to accept trivial NACKs for them as well. IMHO its best if the large diffstat trivial patches had more purpose than just scratch a small tiny itch. Luis --
'fraid not.
ath/debug.h
#ifdef CONFIG_ATH_DEBUG
void ath_print(struct ath_common *common, int dbg_mask, const char *fmt, ...)
__attribute__ ((format (printf, 3, 4)));
#else
static inline void __attribute__ ((format (printf, 3, 4)))
ath_print(struct ath_common *common, int dbg_mask, const char *fmt, ...)
{
}
#endif /* CONFIG_ATH_DEBUG */
--
Now we're getting closer to something worth fixing. IMHO we should change the code so that ath_print(common, ATH_DBG_FATAL, ...) prints something even with CONFIG_ATH_DEBUG unset. To get this done, some renaming would make sense here. - Felix --
Perhaps the function name is bad after all if Luis believed it be be always active. If there are going to be other non-debug uses, I suggest adding the more standard styles of ath_printk and ath_<level> similar to dev_printk and dev_<level> from device.h Something like this for a start, then a more gradual rename of ath_print to ath_dbg (or ath_debug) as the original patch suggested. This basically removes debug.h leaving only an #define ath_printk ath_dbg there and moving all the ATH_DBG_<foo> enums to ath.h I do not think there's much purpose for struct ath_common * being passed to the ath_printk functions, but perhaps there might be. Signed-off-by: Joe Perches <joe@perches.com> --- drivers/net/wireless/ath/ath.h | 103 ++++++++++++++++++++++++++++++++++++++ drivers/net/wireless/ath/debug.c | 20 ------- drivers/net/wireless/ath/debug.h | 72 +-------------------------- drivers/net/wireless/ath/main.c | 20 +++++++ 4 files changed, 124 insertions(+), 91 deletions(-) diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h index 26bdbee..5a598b9 100644 --- a/drivers/net/wireless/ath/ath.h +++ b/drivers/net/wireless/ath/ath.h @@ -186,4 +186,107 @@ bool ath_hw_keyreset(struct ath_common *common, u16 entry); void ath_hw_cycle_counters_update(struct ath_common *common); int32_t ath_hw_get_listen_time(struct ath_common *common); +extern __attribute__ ((format (printf, 3, 4))) int +ath_printk(const char *level, struct ath_common *common, const char *fmt, ...); + +#define ath_emerg(common, fmt, ...) \ + ath_printk(KERN_EMERG, common, fmt, ##__VA_ARGS) +#define ath_alert(common, fmt, ...) \ + ath_printk(KERN_ALERT, common, fmt, ##__VA_ARGS) +#define ath_crit(common, fmt, ...) \ + ath_printk(KERN_CRIT, common, fmt, ##__VA_ARGS) +#define ath_err(common, fmt, ...) \ + ath_printk(KERN_ERR, common, fmt, ##__VA_ARGS) +#define ath_warn(common, fmt, ...) \ + ath_printk(KERN_WARNING, common, fmt, ...
Sorry, that patch is defective. (and an old version) That's what I get for not using git format-patch to send it. I'll resend it appropriately. --
Add ath_printk and ath_<level> similar to dev_printk and dev_<level> from device.h This allows a more gradual rename of ath_print to to ath_dbg or perhaps ath_debug. This basically removes debug.h leaving only an #define ath_printk ath_dbg there and moving all the ATH_DBG_<foo> enums to ath.h I do not think there's much purpose for struct ath_common * being passed to the ath_printk functions, but perhaps there might be. Signed-off-by: Joe Perches <joe@perches.com> --- drivers/net/wireless/ath/ath.h | 103 ++++++++++++++++++++++++++++++++++++++ drivers/net/wireless/ath/debug.c | 20 ------- drivers/net/wireless/ath/debug.h | 72 +-------------------------- drivers/net/wireless/ath/main.c | 20 +++++++ 4 files changed, 124 insertions(+), 91 deletions(-) diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h index 26bdbee..5a598b9 100644 --- a/drivers/net/wireless/ath/ath.h +++ b/drivers/net/wireless/ath/ath.h @@ -186,4 +186,107 @@ bool ath_hw_keyreset(struct ath_common *common, u16 entry); void ath_hw_cycle_counters_update(struct ath_common *common); int32_t ath_hw_get_listen_time(struct ath_common *common); +extern __attribute__ ((format (printf, 3, 4))) int +ath_printk(const char *level, struct ath_common *common, const char *fmt, ...); + +#define ath_emerg(common, fmt, ...) \ + ath_printk(KERN_EMERG, common, fmt, ##__VA_ARGS) +#define ath_alert(common, fmt, ...) \ + ath_printk(KERN_ALERT, common, fmt, ##__VA_ARGS) +#define ath_crit(common, fmt, ...) \ + ath_printk(KERN_CRIT, common, fmt, ##__VA_ARGS) +#define ath_err(common, fmt, ...) \ + ath_printk(KERN_ERR, common, fmt, ##__VA_ARGS) +#define ath_warn(common, fmt, ...) \ + ath_printk(KERN_WARNING, common, fmt, ##__VA_ARGS) +#define ath_notice(common, fmt, ...) \ + ath_printk(KERN_NOTICE, common, fmt, ##__VA_ARGS) +#define ath_info(common, fmt, ...) \ + ath_printk(KERN_INFO, common, fmt, ##__VA_ARGS) + +/** + * enum ath_debug_level - atheros ...
So these errors are always emitted at KERN_ERR level.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/wireless/ath/ath9k/ahb.c | 7 +-
drivers/net/wireless/ath/ath9k/ar5008_phy.c | 12 +---
drivers/net/wireless/ath/ath9k/ar9002_hw.c | 6 +-
drivers/net/wireless/ath/ath9k/beacon.c | 14 ++---
drivers/net/wireless/ath/ath9k/eeprom_4k.c | 13 ++---
drivers/net/wireless/ath/ath9k/eeprom_9287.c | 13 ++---
drivers/net/wireless/ath/ath9k/eeprom_def.c | 14 ++---
drivers/net/wireless/ath/ath9k/gpio.c | 4 +-
drivers/net/wireless/ath/ath9k/htc_drv_beacon.c | 4 +-
drivers/net/wireless/ath/ath9k/htc_drv_init.c | 27 +++-----
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 59 ++++++++---------
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 25 +++-----
drivers/net/wireless/ath/ath9k/hw.c | 78 ++++++++++-------------
drivers/net/wireless/ath/ath9k/init.c | 6 +-
drivers/net/wireless/ath/ath9k/mac.c | 31 ++++-----
drivers/net/wireless/ath/ath9k/main.c | 56 +++++++----------
drivers/net/wireless/ath/ath9k/pci.c | 7 +-
drivers/net/wireless/ath/ath9k/rc.c | 4 +-
drivers/net/wireless/ath/ath9k/recv.c | 15 ++---
drivers/net/wireless/ath/ath9k/xmit.c | 30 ++++-----
drivers/net/wireless/ath/key.c | 15 ++---
21 files changed, 186 insertions(+), 254 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ahb.c b/drivers/net/wireless/ath/ath9k/ahb.c
index 1a984b0..25a6e44 100644
--- a/drivers/net/wireless/ath/ath9k/ahb.c
+++ b/drivers/net/wireless/ath/ath9k/ahb.c
@@ -35,10 +35,9 @@ static bool ath_ahb_eeprom_read(struct ath_common *common, u32 off, u16 *data)
pdata = (struct ath9k_platform_data *) pdev->dev.platform_data;
if (off >= (ARRAY_SIZE(pdata->eeprom_data))) {
- ath_print(common, ATH_DBG_FATAL,
- "%s: flash read failed, offset %08x "
- "is ...This file pattern is not currently shown as maintained by atheros. Perhaps it should be? Signed-off-by: Joe Perches <joe@perches.com> --- MAINTAINERS | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index b162fbb..b744d08 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1080,6 +1080,12 @@ S: Supported F: Documentation/aoe/ F: drivers/block/aoe/ +ATHEROS ATH GENERIC UTILITIES +M: "Luis R. Rodriguez" <lrodriguez@atheros.com> +L: linux-wireless@vger.kernel.org +S: Supported +F: drivers/net/wireless/ath/* + ATHEROS ATH5K WIRELESS DRIVER M: Jiri Slaby <jirislaby@gmail.com> M: Nick Kossifidis <mickflemm@gmail.com> --
Sure but its maintained by the community and Atheros hackers, not just me. Luis --
No doubt.
$ git log next-20101130 -- drivers/net/wireless/ath/*.[ch] | \
grep -P "(^Author)" | cut -f2- -d":" | sort | uniq -c | sort -rn | head
27 Luis R. Rodriguez <lrodriguez@atheros.com>
6 Felix Fietkau <nbd@openwrt.org>
4 Bruno Randolf <br1@einfach.org>
3 Sujith <Sujith.Manoharan@atheros.com>
3 John W. Linville <linville@tuxdriver.com>
3 Joe Perches <joe@perches.com>
3 Bob Copeland <me@bobcopeland.com>
2 Pavel Roskin <proski@gnu.org>
2 Johannes Berg <johannes@sipsolutions.net>
2 David S. Miller <davem@davemloft.net>
Without any sort of MAINTAINERS entry, the results
for files in that directory are like this:
$ ./scripts/get_maintainer.pl -f --rolestats drivers/net/wireless/ath/main.c
"John W. Linville" <linville@tuxdriver.com> (maintainer:NETWORKING [WIREL...)
linux-wireless@vger.kernel.org (open list:NETWORKING [WIREL...)
netdev@vger.kernel.org (open list:NETWORKING DRIVERS)
linux-kernel@vger.kernel.org (open list)
Perhaps you want to be cc'd?
Perhaps there are atheros lists that should be cc'd?
If cc'ing linux-wireless is good enough,
that's good enough for me too.
cheers, Joe
--
OK you're patch is fine then. Luis --
But you sent it as part of an old thread, not sure if John will see it. Luis --
He was on the To: list, I think he'll see it eventually. --
