Re: [PATCH][v4] ipwireless: driver for 3G PC Card

David Sterba
2008-02-06T11:16:22Z

Hi,

> > +irqreturn_t ipwireless_interrupt(int irq, void *dev_id, struct pt_regs
> > *regs) +{
> > + struct ipw_hardware *hw = dev_id;
> > +
> > + if (hw->hw_version == HW_VERSION_1)
> > + return ipwireless_handle_v1_interrupt(irq, hw);
> > + else
> > + return ipwireless_handle_v2_v3_interrupt(irq, hw);
> > +}
>
> why is this not static? I think the interrupt routine should be in the
> file where you actually claim the interrupt.

This is due to current separation into files. hardware.c has many functions
for handler bodies, while main.c registers the pcmcia structs in module init
(and takes address of ipwireless_interrupt()). Long ago, the driver was one
large file, and I don't want to merge the files back just to make some funcs
static (and make the sources less readable).

>
> > +/*
> > + * Associate the specified network with this hardware, so it will
> > receive events + * from it.
> > + */
> > +void ipwireless_associate_network(struct ipw_hardware *hw,
> > + struct ipw_network *network)
> > +{
> > + hw->network = network;
> > +}
>
> I think a #define would be simpler in this case.

Same reason, the macro would be used in a file where the struct is not yet
defined.

> > +module_param(tty_major, int, 0);
> > +module_param_named(debug, ipwireless_debug, int, 0);
> > +module_param_named(loopback, ipwireless_loopback, int, 0);
> > +module_param_named(out_queue, ipwireless_out_queue, int, 0);
> > +MODULE_PARM_DESC(tty_major, "ttyIPWp major number [0]");
>
> Why is allowing to change the major still needed. I think we passed the
> bridge of the need for static numbers a long long long time ago.

Sounds reasonable. I personally haven't used this param (besides testing the
code) and don't know of any related problems.

> > +static const char drv_name[] = IPWIRELESS_PCCARD_NAME;
>
> This looks useless to. Do we need that?

Removed. Used only in one place.

Thanks for comments. Updated version 5 will follow.

Dave