On Thu, 2014-01-30 at 21:23 +0530, Jahnavi wrote: > From: Jahnavi Meher <jahnavi.meher@xxxxxxxxxxxxxxxxxx> > > This driver supports the RS9113 chipset from Redpine Signals Inc. This > chipset supports the 802.11a/b/g/n standards, but this driver currently > supports only b/g/n. Both SDIO and USB interfaces are supported. Can be > upgraded to 91x quite easily. More information can be found at: > http://www.redpinesignals.com/Technologies_&_Chipsets/Chipsets/RS9113.php You should probably propose it for staging... let me take a few minutes and make a quick review pass. > +/** > + * @file rsi_device_ops.h > + * @author > + * @version 1.0 > + * > + * @section LICENSE Those lines are all useless > + * Copyright (c) 2013 Redpine Signals Inc. It's 2014 now, surely you want to claim copyright this year? > + * @section DESCRIPTION That's unnecessary. > + * This file contians the data structures and variables/ macros commonly contains > +#define RSI_HEADER_SIZE 18 That should probably be some struct, and the define doing sizeof()? > +static inline unsigned int rsi_get_queueno(unsigned char *addr, > + unsigned short offset) > +{ > + return (le16_to_cpu(*(unsigned short *)&addr[offset]) & 0x7000) >> 12; > +static inline unsigned int rsi_get_length(unsigned char *addr, > + unsigned short offset) > +static inline unsigned char rsi_get_extended_desc(unsigned char *addr, > + unsigned short offset) > +static inline unsigned char rsi_get_rssi(unsigned char *addr) > +static inline unsigned char rsi_get_channel(unsigned char *addr) These seem like they should then just be replaced by some_struct_ptr->rssi and similar? > +int rsi_send_ampdu_indication_frame(struct rsi_common *common, > + unsigned short tid, > + unsigned short ssn, unsigned char buf_size, > + unsigned char event); what are "A-MPDU indication frame[s]"? > +#ifdef USE_USB_INTF Shouldn't that be just CONFIG_RSI_SOMETHING? > +typedef void (*SD_INTERRUPT)(void *pcontext); no typedefs please > +#define SDIO_BLOCK_SIZE 256 Seems like there should be a define for that already? > +++ b/drivers/net/wireless/rsi/include/rsi_mac80211.h 2014-01-30 16:01:00.194777922 +0530 > +static const struct ieee80211_channel rsi_2ghz_channels[] = { > +static const struct ieee80211_channel rsi_5ghz_channels[] = { > +static struct ieee80211_rate rsi_rates[] = { > +static const unsigned short rsi_mcsrates[] = { None of this belongs into a header file. > + * This file contians the os dependent( Linux) specific function prototypes > + * and macros That kind of thing is generally frowned upon. > +void rsi_print(int zone, unsigned char *vdata, int len); Unless it's more than just a simple printk wrapper? > +struct module_ps_config { > + unsigned int not_in_use:1; > + unsigned int clock_gate:1; > + unsigned int logical_prgm:1; > + unsigned int logical_poweroff:1; > + unsigned int power_off:1; > + unsigned int resvd:27; > +}; Uh, no, you can't use bitfields in hardware/firmware communication structs. They also need to be __packed, in most cases. > +struct rsi_mac_frame { All those unions seem a bit ... overblown? Probably better to define separate structs for different usages and cast appropriately. 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
