On Mon, 2014-03-03 at 13:25 +0530, Fariya Fatima wrote:
> +config RSI_91x
Convention would be to use RSI_91X.
> + tristate "Redpine Signals Inc 91x WLAN driver support"
> + depends on MAC80211
> + default m
Please don't add a default, the driver is probably useless for almost
everyone running 'make oldconfig' and similar.
> +config RSI_DEBUGFS
> + bool "Redpine Signals Inc debug support"
> + depends on RSI_91x
> + default y
This on the other hand is fine since it depends on RSI_91x
> +#ifndef __RSI_DEBUGFS_H__
> +#define __RSI_DEBUGFS_H__
> +
> +#include "rsi_main.h"
> +#include <linux/debugfs.h>
> +
> +#define FOPS(fopen) { \
> + .owner = THIS_MODULE, \
> + .open = (fopen), \
> + .read = seq_read, \
> + .llseek = seq_lseek, \
> +}
I'm not sure these should be in a header file? Seems only the debugfs
implemnetation would need them.
> +#define TID_TO_WME_AC(_tid) ( \
> + ((_tid) == 0 || (_tid) == 3) ? BE_Q : \
> + ((_tid) < 3) ? BK_Q : \
> + ((_tid) < 6) ? VI_Q : \
> + VO_Q)
> +
> +#define WME_AC(_q) ( \
> + ((_q) == BK_Q) ? IEEE80211_AC_BK : \
> + ((_q) == BE_Q) ? IEEE80211_AC_BE : \
> + ((_q) == VI_Q) ? IEEE80211_AC_VI : \
> + IEEE80211_AC_VO)
What are those "BE_Q" constants? Those seem a tad short to me.
> +enum EDCA_QUEUE {
Ah, here they are. enums should typically not be uppercase though.
> +struct rsi_event {
> + atomic_t event_condition;
> + wait_queue_head_t event_queue;
> +};
> +
> +struct rsi_thread {
> + void (*thread_function)(void *);
> + struct completion completion;
> + struct task_struct *task;
> + struct rsi_event event;
> + atomic_t thread_done;
> +};
These seem odd ... maybe they should at least come with comments about
how generic kernel functionality can't be used and why it needs another
abstraction layer?
> + /* Generic */
> + u8 channel;
> + u8 *rx_data_pkt;
That seems rather odd, how can you have one rx_data_pkt in the global
struct?
> +#ifdef CONFIG_RSI_DEBUGFS
> + struct rsi_debugfs *dfsentry;
> + u8 num_debugfs_entries;
> +#endif
What do you need num_entries for?
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
