Message-Id: <200701270253.08320.arnd@arndb.de>
Date:	Sat, 27 Jan 2007 02:53:07 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Marcelo Tosatti <marcelo@...ck.org>
Cc:	netdev <netdev@...r.kernel.org>, Jeff Garzik <jgarzik@...ox.com>,
	"John W. Linville" <linville@...hat.com>,
	Dan Williams <dcbw@...hat.com>,
	"Luis R. Rodriguez" <mcgrof@...il.com>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Subject: Re: [PATCH] Marvell Libertas 8388 802.11b/g USB driver (v2)

On Tuesday 16 January 2007 19:55, Marcelo Tosatti wrote:
> 
> Announcing an updated patch of the Marvell Libertas 8388 802.11 USB
> driver.
> 
> Diff can be found at 
> http://dev.laptop.org/~marcelo/libertas-8388-16012007.patch
> 
> _Please_ review, this driver is targeted for mainline inclusion.
> 
> There have been almost no comments resulting from the first submission.

I think the main problem is the size of the driver. I have spend several
hours on reviewing it, but this has barely scratched the surface of
the problems that are still in it. Most of the important problems
have already been discussed widely, but I'll still post what I noticed
about the driver.

My general feeling is that even at this point it would be better to
write a new driver for it from scratch than trying to remove all
the bloat from the existing one. Of course, that requires deep
knowledge of how the hardware works, so I don't consider the time
you spent on hacking it lost.

I also think that your work done so far is an enormous improvement
over where it started, you are definitely on the right track.

The level of detail in my review drops towards the end, mostly
because some issues found in the first files are continuing to
throughout the driver.

> --- /dev/null
> +++ b/drivers/net/wireless/libertas/Makefile
> @@ -0,0 +1,21 @@
> +# EXTRA_CFLAGS += -Wpacked
> +
> +usb8xxx-objs := wlan_main.o wlan_fw.o wlan_wext.o \
> +		wlan_rx.o wlan_tx.o wlan_cmd.o 	  \
> +		wlan_cmdresp.o wlan_scan.o	  \
> +		wlan_join.o wlan_11d.o 		  \
> +		wlan_ioctl.o wlan_debugfs.o	  \
> +		wlan_ethtool.o wlan_assoc.o
> +

I guess you can get rid of the wlan_ file name prefix, since all files
are already in the same directory to give them  a name space.

> +
> + (c) Copyright  2003-2006, Marvell International Ltd. 
> + All Rights Reserved
> +

'All Rights Reserved' sounds wrong, considering it's actually GPL.

> +  * This file contains definitions of WLAN commands.
> +  *  
> +  * (c) Copyright © 2003-2006, Marvell International Ltd. 

Something went wrong with the (c) character here, better not use UTF8
characters in source.

> +Change log:
> +    10/11/05: Add Doxygen format comments
> +    01/11/06: Remove assoc response codes; full IEEE assoc resp now returned
> +    04/06/06: Add TSPEC, queue metrics, and MSDU expiry support
> +    04/10/06: Add power_adapt_cfg_ext command
> +    04/18/06: Remove old Subscrive Event and add new Subscribe Event
> +	      implementation through generic hostcmd API

changelog should go away

> +/** Command Processing States and Options */
> +#define HostCmd_STATE_IDLE                    0x0000
> +#define HostCmd_STATE_IN_USE_BY_HOST          0x0001
> +#define HostCmd_STATE_IN_USE_BY_MINIPORT      0x0002
> +#define HostCmd_STATE_FINISHED                0x000f

No SilLYcAps please

> +
> +/** General Result Code*/
> +/* OK */
> +#define HostCmd_RESULT_OK                    0x0000
> +/* Genenral error */
> +#define HostCmd_RESULT_ERROR                 0x0001
> +/* Command is not valid */
> +#define HostCmd_RESULT_NOT_SUPPORT           0x0002
> +/* Command is pending */
> +#define HostCmd_RESULT_PENDING               0x0003
> +/* System is busy (command ignored) */
> +#define HostCmd_RESULT_BUSY                  0x0004
> +/* Data buffer is not big enough */
> +#define HostCmd_RESULT_PARTIAL_DATA          0x0005

These seem to be completely unused, except HostCmd_RESULT_OK, which
you can then remove as well.

> +/* Definition of action or option for each command */
> +
> +/* Define general purpose action */
> +#define HostCmd_ACT_GEN_READ                    0x0000
> +#define HostCmd_ACT_GEN_WRITE                   0x0001
> +#define HostCmd_ACT_GEN_GET                     0x0000
> +#define HostCmd_ACT_GEN_SET                     0x0001
> +#define HostCmd_ACT_GEN_REMOVE			0x0002
> +#define HostCmd_ACT_GEN_OFF                     0x0000
> +#define HostCmd_ACT_GEN_ON                      0x0001
> +
> +/* Define action or option for HostCmd_CMD_802_11_AUTHENTICATE */
> +#define HostCmd_ACT_AUTHENTICATE                0x0001
> +#define HostCmd_ACT_DEAUTHENTICATE              0x0002
> +
> +/* Define action or option for HostCmd_CMD_802_11_ASSOCIATE */
> +#define HostCmd_ACT_ASSOCIATE                   0x0001
> +#define HostCmd_ACT_DISASSOCIATE                0x0002
> +#define HostCmd_ACT_REASSOCIATE                 0x0003
> +
> +#define HostCmd_CAPINFO_ESS                     0x0001
> +#define HostCmd_CAPINFO_IBSS                    0x0002
> +#define HostCmd_CAPINFO_CF_POLLABLE             0x0004
> +#define HostCmd_CAPINFO_CF_REQUEST              0x0008
> +#define HostCmd_CAPINFO_PRIVACY                 0x0010

Most of these are unused as well, I guess it would be good
to go through the whole list and see what can be removed.

> +/* TxPD descriptor */
> +struct TxPD {
> +	/* Current Tx packet status */
> +	u32 TxStatus;
> +	/* Tx Control */
> +	u32 TxControl;
> +	u32 TxPacketLocation;
> +	/* Tx packet length */
> +	u16 TxPacketLength;
> +	/* First 2 byte of destination MAC address */
> +	u8 TxDestAddrHigh[2];
> +	/* Last 4 byte of destination MAC address */
> +	u8 TxDestAddrLow[4];
> +	/* Pkt Priority */
> +	u8 Priority;
> +	/* Pkt Trasnit Power control */
> +	u8 PowerMgmt;
> +	/* Amount of time the packet has been queued in the driver (units = 2ms) */
> +	u8 PktDelay_2ms;
> +	/* Reserved */
> +	u8 Reserved1;
> +};

More SilLYcAps to convert...

> +struct WLAN_802_11_KEY {
> +	u16 type; /* KEY_TYPE_* from wlan_defs.h */
> +	u32 len;
> +	u8 key[MRVL_MAX_KEY_WPA_KEY_LENGTH];
> +	u32 flags;  /* KEY_INFO_* from wlan_defs.h */
> +};
> +

This structure has member misalignment which requires
padding. If you use them only in the kernel, please reorder the
members to avoid this, otherwise it's probably better to
make the padding explicit.

> +/* HostCmd_DS_GET_HW_SPEC */
> +struct HostCmd_DS_GET_HW_SPEC {
> +	/* HW Interface version number */
> +	u16 HWIfVersion;
> +
> +	/* HW version number */
> +	u16 Version;
> +
> +	/* Max number of TxPD FW can handle */
> +	u16 NumOfTxPD;
> +
> +	/* Max no of Multicast address */
> +	u16 NumOfMCastAdr;
> +
> +	/* MAC address */
> +	u8 PermanentAddr[6];
> +
> +	/* Region Code */
> +	u16 RegionCode;
> +
> +	/* Number of antenna used */
> +	u16 NumberOfAntenna;
> +
> +	/* FW release number, example 0x1234=1.2.3.4 */
> +	u32 FWReleaseNumber;
> +
> +	/* Base Address of TxPD queue */
> +	u32 WcbBase;
> +	/* Read Pointer of RxPd queue */
> +	u32 RxPdRdPtr;
> +
> +	/* Write Pointer of RxPd queue */
> +	u32 RxPdWrPtr;
> +
> +	/*FW/HW Capability */
> +	u32 fwCapInfo;
> +} __attribute__ ((packed));
> +
> +/* HostCmd_CMD_EEPROM_UPDATE */
> +struct HostCmd_DS_EEPROM_UPDATE {
> +	u16 Action;
> +	u32 Value;
> +} __attribute__ ((packed));
> +

Why do you want to have these structures packed? This often causes
much worse code than reordering the members for proper alignment.

> +/* HostCmd_RET_802_11_ASSOCIATE */
> +struct HostCmd_DS_802_11_ASSOCIATE_RSP {
> +	struct IEEEtypes_AssocRsp assocRsp;
> +} __attribute__ ((packed));

this ((packed)) looks really weird, are you sure it does what you
want it to?

> +/* HostCmd_DS_802_11_AFC */
> +struct HostCmd_DS_802_11_AFC {
> +	u16 afc_auto;
> +	union {
> +		struct {
> +			u16 threshold;
> +			u16 period;
> +		} auto_mode;
> +		struct {
> +			s16 timing_offset;
> +			s16 carrier_offset;
> +		} manual_mode;
> +	} b;
> +} __attribute__ ((packed));
> +
> +#define afc_data b.data
> +#define afc_thre b.auto_mode.threshold
> +#define afc_period b.auto_mode.period
> +#define afc_toff b.manual_mode.timing_offset
> +#define afc_foff b.manual_mode.carrier_offset

should we use anonymous unions here? all recent gcc versions
allow it.

> +		struct HostCmd_TX_RATE_QUERY txrate;
> +		struct HostCmd_DS_BT_ACCESS bt;
> +		struct HostCmd_DS_FWT_ACCESS fwt;
> +		struct HostCmd_DS_MESH_ACCESS mesh;
> +		struct HostCmd_DS_GET_TSF gettsf;
> +		struct HostCmd_DS_802_11_SUBSCRIBE_EVENT subscribe_event;
> +	} params;
> +} __attribute__ ((packed));

same here.

> +
> +extern struct BootCMDStr	sBootCMD;
> +

'extern' declarations should _all_ go into header files. Normally,
sparse should catch this. Did you run the file through a recent sparse
successfully?

> +int if_usb_issue_boot_command(wlan_private *priv, int iValue)
> +{
> +	struct usb_card_rec	*cardp = priv->wlan_dev.card;
> +	int i;
> +
> +	/* Prepare Command */
> +	sBootCMD.u32MagicNumber = BOOT_CMD_MAGIC_NUMBER;
> +	sBootCMD.u8CMD_Tag = iValue;
> +	for (i=0; i<11; i++)
> +		sBootCMD.au8Dumy[i]=0x00;
> +	memcpy(cardp->bulk_out_buffer, &sBootCMD, sizeof(struct BootCMDStr));

It looks like a bad idea to use the global sBootCMD variable here. I
can't see any locking to protect it, and it's small enough to fit on the
stack. What's the point?

> +
> +	/* Issue Command */
> +	usb_tx_block(priv, cardp->bulk_out_buffer, sizeof(struct BootCMDStr));
> +
> +	return WLAN_STATUS_SUCCESS;
> +}

WLAN_STATUS_SUCCESS is not a standard return code. It would be better to
use the standard calling conventions returning '0' or '-ESOMETHING'
for failure instead of defining your own.

> +static usb_notifier_fn_add wlan_card_add_cb = NULL;
> +static usb_notifier_fn_remove wlan_card_remove_cb = NULL;

You currently link the usb parts and the wlan parts of the driver
into a single module, which makes the dynamic registration of these
callbacks rather pointless. Can't you just get rid of that entirely?

> +static u8 usb_int_cause = 0;

This is a global variable, but you seem to use it per device. Is that
intended?

> +struct BootCMDStr       sBootCMD;

This variable is used only in one file, but defined in another...

> +int if_usb_issue_boot_command(wlan_private *priv, int iValue);
> +
> +static int if_prog_firmware(wlan_private * priv);
> +static void if_usb_receive(struct urb *urb);
> +static void if_usb_receive_fwload(struct urb *urb);
> +static int if_usb_probe(struct usb_interface *intf,
> +			const struct usb_device_id *id);
> +static void if_usb_disconnect(struct usb_interface *intf);
> +#ifdef CONFIG_PM
> +static int if_usb_suspend(struct usb_interface *intf, pm_message_t message);
> +static int if_usb_resume(struct usb_interface *intf);
> +#else
> +#define if_usb_suspend NULL
> +#define if_usb_resume NULL
> +#endif

It would be good to avoid forward declarations by reordering the
functions into their call order (bottom up).

> +static void if_usb_write_bulk_callback(struct urb *urb)
> +{
> +	wlan_private *priv = (wlan_private *) (urb->context);
> +	wlan_adapter *adapter = priv->adapter;
> +	struct net_device *dev = priv->wlan_dev.netdev;
> +
> +	/* handle the transmission complete validations */
> +
> +	if (urb->status != 0) {
> +		/* print the failure status number for debug */
> +		printk(KERN_INFO "URB in failure status\n");
> +	} else {
> +		dprintk(2, "URB status is successfull\n");
> +		dprintk(2, "Actual length transmitted %d\n",
> +		       urb->actual_length);

The printk and dprintk statements should probably be converted
to dev_info/dev_dbg/... standard macros, or something derived
from that.

> +void if_usb_free(struct usb_card_rec *cardp)
> +{
> +	ENTER();

Do you find the ENTER() and LEAVE() macros really useful?
usually, you're better off taking them out...

> +	return (-ENOMEM);

	return -ENOMEM;

> +	if (!(rinfo = kmalloc(sizeof(struct read_cb_info), GFP_ATOMIC))) {
> +		printk(KERN_ERR "No free read_callback_info\n");
> +		goto rx_ret;
> +	}

I usually find it more readable to do the assignment first, and have the
error check in a separate line.

> +struct BootCMDRespStr bootcmdresp;
> +

This variable is used only inside of one function, so you should
be able to just put it on the stack (it's small).

> +/**  
> + *  @brief This function reads of the packet into the upload buff, 
> + *  wake up the main thread and initialise the Rx callack.
> + *  
> + *  @param urb		pointer to struct urb
> + *  @return 	   	N/A
> + */
> +static void if_usb_receive(struct urb *urb)
> +{

This function is a little long, there is a switch() statement in it
that can probably be converted into one function per case.

> +/** 
> + *  @brief Given a usb_card_rec return its wlan_private
> + *  @param card		pointer to a usb_card_rec
> + *  @return 	   	pointer to wlan_private
> + */
> +wlan_private *libertas_sbi_get_priv(void *card)
> +{
> +	struct usb_card_rec *cardp = (struct usb_card_rec *)card;
> +	return (wlan_private *)cardp->priv;
> +}

Don't do explicit casts of void* pointers.

> +	data = *((int *)(wrq->u.name + SUBCMD_OFFSET));

You use this weird line in many places, it would be good to make it
a helper function.

> +static u8 Is_Command_Allowed_In_PS(u16 Command)
> +{
> +	int count = sizeof(Commands_Allowed_In_PS)
> +	    / sizeof(Commands_Allowed_In_PS[0]);
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		if (Command == cpu_to_le16(Commands_Allowed_In_PS[i]))
> +			return 1;
> +	}
> +
> +	return 0;
> +}

In places where you use variables that are strictly little-endian,
it would be nice to use the __le32/__le16 types instead of u32/u16.

> +/** 
> + *  @brief This function handles the command response
> + *  
> + *  @param priv    A pointer to wlan_private structure
> + *  @return 	   WLAN_STATUS_SUCCESS or WLAN_STATUS_FAILURE
> + */
> +int libertas_process_rx_command(wlan_private * priv)
> +{

This function is incredibly long, to the point where it gets
unreadable.

> +static struct dentry *libertas_dir = NULL;
> +static const int big_buffer_len = 4096;
> +static char big_buffer[4096];
> +static DECLARE_MUTEX(big_buffer_sem);

This seems to provide a generalized interface for buffering debugfs
files. Have you considered using the existing simple_attribute
and seq_file interfaces instead?

Even if they don't work for this, I would consider it cleaner to
use get_zeroed_page/free_page instead of the global buffer here.

> +static struct file_operations libertas_devinfo_fops = { 
> +	.owner = THIS_MODULE,
> +	.open = open_file_generic,
> +	.write = write_file_dummy,
> +	.read = libertas_dev_info,
> +};
> +
> +static struct file_operations libertas_getscantable_fops = { 
> +	.owner = THIS_MODULE,
> +	.open = open_file_generic,
> +	.write = write_file_dummy,
> +	.read = libertas_getscantable,
> +};
> +
> +static struct file_operations libertas_sleepparams_fops = { 
> +	.owner = THIS_MODULE,
> +	.open = open_file_generic,
> +	.write = libertas_sleepparams_write,
> +	.read = libertas_sleepparams_read,
> +};

I would guess you can better express this with some array, like

#define FOPS(read, write) { \
	.owner = THIS_MODULE, \
	.open = open_file_generic, \
	.read = (read), \
	.write = (write), \
}

struct libertas_debugfs_files {
	char *name;
	int perm;
	struct file_operations fops;
} debugfs_files = {
	{ "devinfo", 0444, FOPS(libertas_dev_info, NULL), },
	{ "getscantable", 0444, FOPS(libertas_getscantable, NULL), },
	{ "sleepparams", 0644, FOPS(libertas_sleepparams_read,
					libertas_sleepparams_write), },
	...
};

> +void libertas_debugfs_init_one(wlan_private *priv, struct net_device *dev)
> +{
> +	if (!libertas_dir)
> +		goto exit;
> +
> +	priv->debugfs_dir = debugfs_create_dir(dev->name, libertas_dir);
> +	if (!priv->debugfs_dir)
> +		goto exit;
> +
> +	priv->debugfs_devinfo = debugfs_create_file("info", 0444,
> +						    priv->debugfs_dir,
> +						    priv,
> +						    &libertas_devinfo_fops);

And then here do a loop over that array.

> +/** Double-Word(32Bit) Bit definition */
> +#define DW_BIT_0	0x00000001
> +#define DW_BIT_1	0x00000002
> +#define DW_BIT_2	0x00000004
> +#define DW_BIT_3	0x00000008
> +#define DW_BIT_4	0x00000010
> +#define DW_BIT_5	0x00000020
> +#define DW_BIT_6	0x00000040
> +#define DW_BIT_7	0x00000080
> +#define DW_BIT_8	0x00000100
> +#define DW_BIT_9	0x00000200
> +#define DW_BIT_10	0x00000400
> +#define DW_BIT_11       0x00000800
> +#define DW_BIT_12       0x00001000
> +#define DW_BIT_13       0x00002000
> +#define DW_BIT_14       0x00004000
> +#define DW_BIT_15       0x00008000
> +#define DW_BIT_16       0x00010000
> +#define DW_BIT_17       0x00020000
> +#define DW_BIT_18       0x00040000
> +#define DW_BIT_19       0x00080000
> +#define DW_BIT_20       0x00100000
> +#define DW_BIT_21       0x00200000
> +#define DW_BIT_22       0x00400000
> +#define DW_BIT_23       0x00800000
> +#define DW_BIT_24       0x01000000
> +#define DW_BIT_25       0x02000000
> +#define DW_BIT_26       0x04000000
> +#define DW_BIT_27       0x08000000
> +#define DW_BIT_28       0x10000000
> +#define DW_BIT_29       0x20000000
> +#define DW_BIT_30	0x40000000
> +#define DW_BIT_31	0x80000000

These should go away.

> +
> +/** Word (16bit) Bit Definition*/
> +#define W_BIT_0		0x0001
> +#define W_BIT_1		0x0002
> +#define W_BIT_2		0x0004
> +#define W_BIT_3		0x0008
> +#define W_BIT_4		0x0010
> +#define W_BIT_5		0x0020
> +#define W_BIT_6		0x0040
> +#define W_BIT_7		0x0080
> +#define W_BIT_8		0x0100
> +#define W_BIT_9		0x0200
> +#define W_BIT_10	0x0400
> +#define W_BIT_11	0x0800
> +#define W_BIT_12	0x1000
> +#define W_BIT_13	0x2000
> +#define W_BIT_14	0x4000
> +#define W_BIT_15	0x8000
> +
> +/** Byte (8Bit) Bit definition*/
> +#define B_BIT_0		0x01
> +#define B_BIT_1		0x02
> +#define B_BIT_2		0x04
> +#define B_BIT_3		0x08
> +#define B_BIT_4		0x10
> +#define B_BIT_5		0x20
> +#define B_BIT_6		0x40
> +#define B_BIT_7		0x80

These as well.

> +extern unsigned int libertas_debug;
> +
> +/** Debug Macro definition*/
> +#ifdef	DEBUG
> +#define dprintk(level,format, args...)	if (libertas_debug >= level) printk(KERN_INFO format, ##args)
> +#else
> +#define dprintk(format, args...)	do {} while (0)
> +#endif

dev_dbg should be enough, if you want it dynamic, you can use
netif_msg_* together with dev_info.

> +#define ASSERT(cond)						\
> +do {								\
> +	if (!(cond))						\
> +		dprintk(1, "ASSERT: %s, %s:%i\n",		\
> +		       __FUNCTION__, __FILE__, __LINE__);	\
> +} while(0)

just use WARN_ON instead of defining your own ASSERT

> +#define	ENTER()			dprintk(1, "Enter: %s, %s:%i\n", __FUNCTION__, \
> +							__FILE__, __LINE__)
> +#define	LEAVE()			dprintk(1, "Leave: %s, %s:%i\n", __FUNCTION__, \
> +							__FILE__, __LINE__)

As mentioned, these should probably just be removed

> +
> +#if defined(DEBUG)
> +static inline void HEXDUMP(char *prompt, u8 * buf, int len)
> +{
> +	int i = 0;
> +
> +	if (!libertas_debug)
> +		return;
> +
> +	printk(KERN_DEBUG "%s: ", prompt);
> +	for (i = 1; i <= len; i++) {
> +		printk(KERN_DEBUG "%02x ", (u8) * buf);
> +		buf++;
> +	}
> +	printk("\n");
> +}
> +#else
> +#define HEXDUMP(x,y,z)
> +#endif

The second definition should be an empty inline function so you don't break
for code like

if (condition)
	HEXDUMP(x,y,z);
else
	something_else();

maybe rename to dev_dbg_hex() and move to the same place as the generic
dev_dbg function.

> +#ifndef NELEMENTS
> +#define NELEMENTS(x) (sizeof(x)/sizeof(x[0]))
> +#endif

array_size()

> +static int wlan_setup_station_hw(wlan_private * priv)
> +{
> +	int ret = WLAN_STATUS_FAILURE;
> +	wlan_adapter *adapter = priv->adapter;
> +
> +	ENTER();
> +
> +	if ((ret = request_firmware(&priv->firmware, libertas_fw_name,
> +				    priv->hotplug_device)) < 0) {
> +		printk(KERN_ERR "request_firmware() failed, error code = %#x\n",
> +		       ret);
> +		printk(KERN_ERR "%s not found in /lib/firmware\n", libertas_fw_name);
> +		goto done;
> +	}

Since we now have an Open Firmware based system, the libertas firmware
could be stored inside of the firmware device tree. The most significant
advantage of that is that you can boot with the libertas driver enabled
before mounting the file system.

Another advantage is that if we get a forth implementation of the driver
that can do PXE boot, the open firmware can use the same copy of the
libertas firmware blob as Linux.

> diff --git a/drivers/net/wireless/libertas/wlan_ioctl.c b/drivers/net/wireless/libertas/wlan_ioctl.c
> new file mode 100644
> index 0000000..0cc852b
> --- /dev/null
> +++ b/drivers/net/wireless/libertas/wlan_ioctl.c
> @@ -0,0 +1,2814 @@
> +/** 
> +  * This file contains ioctl functions

The number of private ioctl functions defined in this file (and some
other places is a hint that there is still something very wrong with
this driver. The best solution I can think of is to throw them all
away and add new interface for those that are really needed, but do
it in a generic way, in the common wireless extensions for Linux.

> +#define WLAN_TX_PWR_DEFAULT		20	/*100mW */
> +#define WLAN_TX_PWR_US_DEFAULT		20	/*100mW */
> +#define WLAN_TX_PWR_JP_DEFAULT		16	/*50mW */
> +#define WLAN_TX_PWR_FR_DEFAULT		20	/*100mW */
> +#define WLAN_TX_PWR_EMEA_DEFAULT	20	/*100mW */
> +
> +/* Format { Channel, Frequency (MHz), MaxTxPower } */
> +/* Band: 'B/G', Region: USA FCC/Canada IC */
> +static struct chan_freq_power channel_freq_power_US_BG[] = {
> +	{1, 2412, WLAN_TX_PWR_US_DEFAULT},
> +	{2, 2417, WLAN_TX_PWR_US_DEFAULT},
> +	{3, 2422, WLAN_TX_PWR_US_DEFAULT},
> +	{4, 2427, WLAN_TX_PWR_US_DEFAULT},
> +	{5, 2432, WLAN_TX_PWR_US_DEFAULT},
> +	{6, 2437, WLAN_TX_PWR_US_DEFAULT},
> +	{7, 2442, WLAN_TX_PWR_US_DEFAULT},
> +	{8, 2447, WLAN_TX_PWR_US_DEFAULT},
> +	{9, 2452, WLAN_TX_PWR_US_DEFAULT},
> +	{10, 2457, WLAN_TX_PWR_US_DEFAULT},
> +	{11, 2462, WLAN_TX_PWR_US_DEFAULT}
> +};
> +
> +/* Band: 'B/G', Region: Europe ETSI */
> +static struct chan_freq_power channel_freq_power_EU_BG[] = {
> +	{1, 2412, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{2, 2417, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{3, 2422, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{4, 2427, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{5, 2432, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{6, 2437, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{7, 2442, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{8, 2447, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{9, 2452, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{10, 2457, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{11, 2462, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{12, 2467, WLAN_TX_PWR_EMEA_DEFAULT},
> +	{13, 2472, WLAN_TX_PWR_EMEA_DEFAULT}
> +};
> +
> +/* Band: 'B/G', Region: Spain */
> +static struct chan_freq_power channel_freq_power_SPN_BG[] = {
> +	{10, 2457, WLAN_TX_PWR_DEFAULT},
> +	{11, 2462, WLAN_TX_PWR_DEFAULT}
> +};
> +
> +/* Band: 'B/G', Region: France */
> +static struct chan_freq_power channel_freq_power_FR_BG[] = {
> +	{10, 2457, WLAN_TX_PWR_FR_DEFAULT},
> +	{11, 2462, WLAN_TX_PWR_FR_DEFAULT},
> +	{12, 2467, WLAN_TX_PWR_FR_DEFAULT},
> +	{13, 2472, WLAN_TX_PWR_FR_DEFAULT}
> +};
> +
> +/* Band: 'B/G', Region: Japan */
> +static struct chan_freq_power channel_freq_power_JPN_BG[] = {
> +	{1, 2412, WLAN_TX_PWR_JP_DEFAULT},
> +	{2, 2417, WLAN_TX_PWR_JP_DEFAULT},
> +	{3, 2422, WLAN_TX_PWR_JP_DEFAULT},
> +	{4, 2427, WLAN_TX_PWR_JP_DEFAULT},
> +	{5, 2432, WLAN_TX_PWR_JP_DEFAULT},
> +	{6, 2437, WLAN_TX_PWR_JP_DEFAULT},
> +	{7, 2442, WLAN_TX_PWR_JP_DEFAULT},
> +	{8, 2447, WLAN_TX_PWR_JP_DEFAULT},
> +	{9, 2452, WLAN_TX_PWR_JP_DEFAULT},
> +	{10, 2457, WLAN_TX_PWR_JP_DEFAULT},
> +	{11, 2462, WLAN_TX_PWR_JP_DEFAULT},
> +	{12, 2467, WLAN_TX_PWR_JP_DEFAULT},
> +	{13, 2472, WLAN_TX_PWR_JP_DEFAULT},
> +	{14, 2484, WLAN_TX_PWR_JP_DEFAULT}
> +};

None of this looks libertas specific to me. Is it perhaps already
provided by the common code?

	Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists