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

[Pavel Machek <pavel@ucw.cz>]

On Thu 2008-01-31 10:29:30, Stephen Blackheath [to Foxconn] wrote:
> Pavel & all,
> 
> 
> Pavel Machek wrote:
> 
> >> +/* I/O ports and bit definitions for version 2 of the hardware */
> >> +
> >> +struct MEMCCR {
> >> +	unsigned short PCCOR;		/* Configuration Option Register */
> >> +	unsigned short PCCSR;		/* Configuration and Status Register */
> >> +	unsigned short PCPRR;		/* Pin Replacemant Register */
> >> +	unsigned short PCSCR;		/* Socket and Copy Register */
> >> +	unsigned short PCESR;		/* Extendend Status Register */
> >> +	unsigned short PCIOB;		/* I/O Base Register */
> >> +};

> > Could we get better names? PCIOB is cryptic, pci_io_base is pretty
> > good.
> >
> >   
> We should keep these names because they are part of the interface
> between host and card defined by the manufacturer.

No. Use sensible names, and put manufacturer-defined 5-letter crap in
the comments. Heck, notice that they just took first letter of each
word of good name....

> > Is some locking needed around *hw?
> >   
> I don't think so, but I'm happy to be corrected.

You have a structure, and are accessing its fields from interrupts. I
assume you access the fields outside interrupt, too? As the fields are
not of atomic_t, I believe you need locking.

(Oh, and I should have said that earlier: Thanks for the driver and
congratulations for getting it this far).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html