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

[David Sterba]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/