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

Pavel Machek
2008-01-30T23:15:19Z

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