LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Marcel Holtmann <marcel@holtmann.org>
Cc: torvalds@linux-foundation.org, linux-kernel@vger.kernel.org,
	jkosina@suse.cz, benm@symmetric.co.nz, stephen@symmetric.co.nz
Subject: Re: [PATCH][v4] ipwireless: driver for 3G PC Card
Date: Wed, 6 Feb 2008 12:15:58 +0100	[thread overview]
Message-ID: <200802061215.58688.dsterba@suse.cz> (raw)
In-Reply-To: <1201883579.15090.14.camel@violet>

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

  reply	other threads:[~2008-02-06 11:16 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-01 15:37 [PATCH][v4] ipwireless: driver for 3G PC Card David Sterba
2008-02-01 16:32 ` Marcel Holtmann
2008-02-06 11:15   ` David Sterba [this message]
2008-02-05 10:19 ` Jiri Kosina
2008-02-05 23:46   ` Marcel Holtmann
2008-02-06  0:02     ` Linus Torvalds
2008-02-06  0:15       ` Jiri Kosina
2008-02-06  8:14         ` Ben Martel
2008-02-07  9:57       ` [PATCH][v5] " David Sterba
2008-02-07 23:27         ` Jiri Kosina

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200802061215.58688.dsterba@suse.cz \
    --to=dsterba@suse.cz \
    --cc=benm@symmetric.co.nz \
    --cc=jkosina@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcel@holtmann.org \
    --cc=stephen@symmetric.co.nz \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).