Re: [Prism54-devel] Re: [PATCH 2.6] Intersil Prism54 wireless driver

[mcgrof@ruslug.rutgers.edu (Luis R. Rodriguez)]

[-- Attachment #1: Type: text/plain, Size: 4381 bytes --]

On Wed, Mar 10, 2004 at 09:21:14AM -0800, Jean Tourrilhes wrote:
> On Wed, Mar 10, 2004 at 04:55:48PM +0000, Christoph Hellwig wrote:
> > On Wed, Mar 03, 2004 at 06:35:24PM -0800, Jean Tourrilhes wrote:
> > > 	Hi Dave & Jeff,
> > > 
> > > 	The attached .bz2 file is a patch for 2.6.3 adding the
> > > Intersil Prism54 wireless driver. Sorry for the attachement, the file
> > > is rather big, if you want inline+plaintext, I'll send that personal
> > > to you.
> > > 	I've been using this driver with great success on 2.6.3 and
> > > 2.6.4-rc1 (SMP). This driver support various popular CardBus and PCI
> > > 802.11g cards (54 Mb/s) based on the Intersil PrismGT/PrismDuette
> > > chipset.
> > > 	I would like this driver to go into 2.6.X. However, I
> > > understand that it's lot's of code to review.
> > 
> > Here's a few things I found.
> 
> 	I'm forwarding to prism54-devel where the real developpers can
> answer your questions.
> 
> >  It's not exactly a full review, there's
> > too much new snow to spend lots of time in front of a computer here :)
> 
> 	Grrr... This year, no snow for me.
> 
> > diff -Naur -X /home/mcgrof/lib/dontdiff linux-2.6.3/drivers/net/wireless/prism54/Makefile linux-2.6.3-prism54/drivers/net/wireless/prism54/Makefile
> > --- linux-2.6.3/drivers/net/wireless/prism54/Makefile	Thu Jan  1 00:00:00 1970
> > +++ linux-2.6.3-prism54/drivers/net/wireless/prism54/Makefile	Thu Mar  4 02:00:01 2004
> > @@ -0,0 +1,10 @@
> > +# $Id: Makefile.k26,v 1.7 2004/01/30 16:24:00 ajfa Exp $
> > +
> > +prism54-objs := islpci_eth.o islpci_mgt.o \
> > +                isl_38xx.o isl_ioctl.o islpci_dev.o \
> > +		islpci_hotplug.o isl_wds.o oid_mgt.o
> > 
> > 	please use foo-y for new drivers.

TODO

> > 
> > +
> > +obj-$(CONFIG_PRISM54) += prism54.o
> > +
> > +EXTRA_CFLAGS = -I$(PWD) #-DCONFIG_PRISM54_WDS
> > 
> > 	This is bogus, especially with srcdir != objdir.
> > 	please fixup the includes instead
> > 
> > +#define __KERNEL_SYSCALLS__
> > 
> > 	this shouldn't be used anymore.

Done

> > 
> > +
> > +#include <linux/version.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/delay.h>
> > +
> > +#include "isl_38xx.h"
> > +#include <linux/firmware.h>
> > +
> > +#include <asm/uaccess.h>
> > +#include <asm/io.h>
> > 
> > 	Please include headers in the following order <linux/*.h>,
> > 	<asm/*.h>, driver-specific.

Done

> > 
> > +#if (LINUX_VERSION_CODE > KERNEL_VERSION(2,5,75))
> > +#include <linux/device.h>
> > +# define _REQ_FW_DEV_T struct device *
> > +#else
> > +# define _REQ_FW_DEV_T char *
> > +#endif
> > 
> > 	Eeek, why don't you simply pass the pci_dev down?

TODO

> > 
> > 
> > +typedef struct isl38xx_cb isl38xx_control_block;
> > 
> > 	No useless typedefs please.
> > 
> > +MODULE_PARM(init_mode, "i");
> > +MODULE_PARM_DESC(init_mode,
> > +		 "Set card mode:\n0: Auto\n1: Ad-Hoc\n2: Managed Client (Default)\n3: Master / Access Point\n4: Repeater (Not supported yet)\n5: Secondary (Not supported yet)\n6: Monitor");
> > 
> > 	Please use module_param

Done

> 
> 	I would even say that this is useless because the driver
> support WE, and WE scripts set the mode before the card is up.

True, we can just remove the param for iw_mode.

> 
> > diff -Naur -X /home/mcgrof/lib/dontdiff linux-2.6.3/drivers/net/wireless/prism54/isl_wds.c linux-2.6.3-prism54/drivers/net/wireless/prism54/isl_wds.c
> > --- linux-2.6.3/drivers/net/wireless/prism54/isl_wds.c	Thu Jan  1 00:00:00 1970
> > +++ linux-2.6.3-prism54/drivers/net/wireless/prism54/isl_wds.c	Thu Mar  4 02:00:01 2004
> > 
> > 	WDS doesn't belong into a driver but in higher-level code.

The driver features some firmware-specific WDS functionality, such as
adding/removing WDS links. We haven't looked much into it yet though since
it's not high priority. Where exactly should this code go to then?

Elements noted as DONE were just committed into our CVS repository. You
can always find our latest 2.6 kernel patch at:

http://prism54.org/pub/linux/snapshot/kernel/v2.6/patch-2.6-prism54-cvs-latest.bz2

FWIW, the same driver code base supports 2.4:
http://prism54.org/pub/linux/snapshot/kernel/v2.4/patch-2.4-prism54-cvs-latest.bz2

	Luis

-- 
GnuPG Key fingerprint = 113F B290 C6D2 0251 4D84  A34A 6ADD 4937 E20A 525E

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]