Re: [PATCH] wavelan_cs update (Pcmcia backport)

Jeff Garzik (jgarzik@mandrakesoft.com)
Fri, 18 Jan 2002 00:07:56 -0500


Jean Tourrilhes wrote:

> + * Wrapper for disabling interrupts.
> + * (note : inline, so optimised away)
> + */
> +static inline void
> +wv_splhi(net_local * lp,
> + unsigned long * pflags)
> +{
> + spin_lock_irqsave(&lp->spinlock, *pflags);
> + /* Note : above does the cli(); itself */
> +}
> +
> +/*------------------------------------------------------------------*/
> +/*
> + * Wrapper for re-enabling interrupts.
> + */
> +static inline void
> +wv_splx(net_local * lp,
> + unsigned long * pflags)
> +{
> + spin_unlock_irqrestore(&lp->spinlock, *pflags);

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.

> +#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?

> + /* 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)

> +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"?

> #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)

> @@ -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

> @@ -1719,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;
> }

likewise

> @@ -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

> + /* 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...

> @@ -4720,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 XXX Thanks... */
> + * ifconfig up ??? Thanks... */
>
> /* Stop receiving new messages and wait end of transmission */
> wv_ru_stop(dev);

reverting valid change

> @@ -4748,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 XXX */
> + if(link->open) /* If RESET -> True, If RESUME -> False ??? */
> {
> wv_hw_reset(dev);
> netif_device_attach(dev);

likewise

-- 
Jeff Garzik      | Alternate titles for LOTR:
Building 1024    | Fast Times at Uruk-Hai
MandrakeSoft     | The Took, the Elf, His Daughter and Her Lover
                 | Samwise Gamgee: International Hobbit of Mystery
-
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/