On Fri, Jan 18, 2002 at 12:07:56AM -0500, Jeff Garzik wrote:
> Jean Tourrilhes wrote:
>
Well... A bit of background...
The Wavelan hardware has been obsoleted for the last 5
years. I put the driver in minimal maintainance mode, because I
already have a lot on my place (you would not want me to spend less
time on the IrDA stack, the Orinoco driver or the new Wireless
Extensions ?).
The point is that the driver didn't work. I didn't look any
further and I decided for better or worse to use the version that is
in the Pcmcia package that has been working for more than one
year. Now that the two version are in sync, I can work from there.
I tried to make sure to not loose any changes. Of course, I'm
only human.
I hope that explains the situation.
Attached you will find a patch that address some of your
concerns. This patch will apply to both the kernel version AND the
Pcmcia package version.
Note also that it applies to the kernel *after* driver move
(that is pending - because I've put this patch at the end of my patch
queue).
> ug... you are much better to put spin_lock_irqsave directly into the
> code, don't wrap it. wrapping, here, only obscures the code using it to
> the casual reader.
>
> This change also makes the diff long.
That's only personal preference. And the wrapper in theory
would allow to simply change the locking strategy if needed (like
disabling irq on the hardware instead of in software).
> > +#if 0
> > + /* Some people don't need this, some other may need it */
> > + nwid=nwid^ntohs(beacon->domain_id);
> > +#endif
>
> maybe better as #ifdef I_NEED_THIS_FEATURE then?
Done.
> > + /* Busy wait while the LAN controller executes the command. */
> > + spin = 1000;
> > + do
> > {
> > - outb(OP0_NOP, LCCR(base));
> > + /* Time calibration of the loop */
> > + udelay(10);
>
> I hate busy-waits :) Long term I wonder if some of the longer ones can
> be pushed into tasklets or even kernel threads. (if a kernel thread,
> then the intr handler would really turn into an async notification to
> the kernel thread of new work)
For such an obsolete driver, it's not worth replacing code
that is working fine and has been debugged and tested for
years. Moreover, on most case the wait is really small.
> > +static inline void
> > +wv_82593_reconfig(device * dev)
> > {
> > - net_local *lp = (net_local *) dev->priv;
> > - dev_link_t *link = ((net_local *) dev->priv)->link;
> > + net_local * lp = (net_local *)dev->priv;
> > + dev_link_t * link = ((net_local *) dev->priv)->link;
>
> Is it possible to use standard kernel types, ie. "struct net_device"
> instead of "device"?
If you want to spend time on this, please send me a patch and
I'll apply it to both the kernel version and the Pcmcia package.
> > #ifdef DEBUG_IOCTL_INFO
> > - printk (KERN_DEBUG "%s: wv_82593_reconfig(): delayed (link =
>
> you could use __FUNCTION__ here if you wanted to. (make sure to pass it
> via %s if you do)
I would not bother.
By the way, I looked at the __FUNCTION__ business, and IrDA
will need a massive cleanup. *sight*...
> > @@ -1404,7 +1417,7 @@ wv_init_info(device * dev)
> > printk("2430.5");
> > break;
> > default:
> > - printk("unknown");
> > + printk("???");
> > }
> > }
> >
>
> you are reverting a change - "???" causes a trigraph-related warning in
> newer gcc
I didn't knew about that, and my gcc doesn't give any
warning. I can't believe that gcc is too stupid to realise that you
can't have trigraph within a string or a comment.
Done. And guess what, it will also go in the Pcmcia package ;-)
> > @@ -1968,7 +1981,7 @@ wavelan_ioctl(struct net_device * dev, /
> >
> > case SIOCGIWFREQ:
> > /* Attempt to recognise 2.00 cards (2.4 GHz frequency selectable)
> > - * (does it work for everybody XXX - especially old cards...) */
> > + * (does it work for everybody ??? - especially old cards...) */
> > if(!(mmc_in(base, mmroff(0, mmr_fee_status)) &
> > (MMR_FEE_STATUS_DWLD | MMR_FEE_STATUS_BUSY)))
> > {
>
> likewise
I'm sorry, but "XXX" doesn't mean "???" in any variation of
the english language that I've ever encoutered.
> > + /* Allocate the generic data structure */
> > + dev = kmalloc(sizeof(struct net_device), GFP_KERNEL);
> > + if (!dev) {
> > + kfree(link);
> > + return NULL;
> > + }
> > + memset(dev, 0x00, sizeof(struct net_device));
> > link->priv = link->irq.Instance = dev;
> >
> > + /* Allocate the wavelan-specific data structure. */
> > + dev->priv = lp = (net_local *) kmalloc(sizeof(net_local), GFP_KERNEL);
> > + if (!lp) {
> > + kfree(link);
> > + kfree(dev);
> > + return NULL;
> > + }
> > + memset(lp, 0x00, sizeof(net_local));
> > +
>
> use alloc_etherdev instead of all this...
No need to change working code.
> Jeff Garzik
Thanks for the quick review...
Jean
--RnlQjJ0d97Da+TV1
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="wav_pc.jeff.diff"
diff -u -p -r linux/drivers/net/wireless.v23/wavelan_cs.c linux/drivers/net/wireless/wavelan_cs.c
--- linux/drivers/net/wireless.v23/wavelan_cs.c Thu Jan 17 10:48:59 2002
+++ linux/drivers/net/wireless/wavelan_cs.c Fri Jan 18 10:36:22 2002
@@ -797,7 +797,7 @@ static inline void wl_roam_gather(device
wavepoint_history *wavepoint=NULL; /* WavePoint table entry */
net_local *lp=(net_local *)dev->priv; /* Device info */
-#if 0
+#ifdef I_NEED_THIS_FEATURE
/* Some people don't need this, some other may need it */
nwid=nwid^ntohs(beacon->domain_id);
#endif
@@ -1417,7 +1417,7 @@ wv_init_info(device * dev)
printk("2430.5");
break;
default:
- printk("???");
+ printk("unknown");
}
}
@@ -1732,7 +1732,7 @@ wv_set_frequency(u_long base, /* i/o po
memcmp(dac, dac_verify, 2 * 2))
{
#ifdef DEBUG_IOCTL_ERROR
- printk(KERN_INFO "Wavelan: wv_set_frequency : unable to write new frequency to EEprom (??)\n");
+ printk(KERN_INFO "Wavelan: wv_set_frequency : unable to write new frequency to EEprom (?)\n");
#endif
return -EOPNOTSUPP;
}
@@ -1981,7 +1981,7 @@ wavelan_ioctl(struct net_device * dev, /
case SIOCGIWFREQ:
/* Attempt to recognise 2.00 cards (2.4 GHz frequency selectable)
- * (does it work for everybody ??? - especially old cards...) */
+ * (does it work for everybody ? - especially old cards...) */
if(!(mmc_in(base, mmroff(0, mmr_fee_status)) &
(MMR_FEE_STATUS_DWLD | MMR_FEE_STATUS_BUSY)))
{
@@ -3163,7 +3163,7 @@ wv_mmc_init(device * dev)
*/
/* Attempt to recognise 2.00 cards (2.4 GHz frequency selectable)
- * (does it work for everybody ??? - especially old cards...) */
+ * (does it work for everybody ? - especially old cards...) */
/* Note : WFREQSEL verify that it is able to read from EEprom
* a sensible frequency (address 0x00) + that MMR_FEE_STATUS_ID
* is 0xA (Xilinx version) or 0xB (Ariadne version).
@@ -4718,7 +4718,7 @@ wavelan_event(event_t event, /* The ev
* obliged to close nicely the wavelan here. David, could you
* close the device before suspending them ? And, by the way,
* could you, on resume, add a "route add -net ..." after the
- * ifconfig up ??? Thanks... */
+ * ifconfig up ? Thanks... */
/* Stop receiving new messages and wait end of transmission */
wv_ru_stop(dev);
@@ -4745,7 +4745,7 @@ wavelan_event(event_t event, /* The ev
if(link->state & DEV_CONFIG)
{
CardServices(RequestConfiguration, link->handle, &link->conf);
- if(link->open) /* If RESET -> True, If RESUME -> False ??? */
+ if(link->open) /* If RESET -> True, If RESUME -> False ? */
{
wv_hw_reset(dev);
netif_device_attach(dev);
--RnlQjJ0d97Da+TV1--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/