Re: [PATCH 2/2] Add rtl8187 wireless driver

["John W. Linville" <linville@tuxdriver.com>]

On Thu, May 17, 2007 at 01:48:02AM -0400, Michael Wu wrote:
> On Monday 14 May 2007 16:29, John W. Linville wrote:
> > If you don't know why it is there, how about a comment indicating
> > what gave you the notion of putting it there?  E.g. "copied from
> > realtek example code" or somesuch?
> >
> The driver is mostly a port of the original r8187 driver.
 
OK.  We still could use a "reminder" comment before magic bits.

> > For this block in particular, I think you had stated that the
> > hardware works without it.  Is there any reason not to just remove it?
> > Just precaution?
> >
> Yes. Don't want to change code that's known to be working at this point.

Understandable.  Please add a "magic from realtek" comment.
 
> > > > More magic number tables of unknown origin...you get the idea. :-)  I
> > > > realize that these are either copied straight from a datasheet or from
> > > > someone's reverse engineered sources -- let's just have a comment
> > > > saying so for each block of these.
> > >
> > > The *entire* rtl8187_rtl8225.c file is full of magic numbers. I'm not
> > > willing to put comments saying so for every single function/definition. I
> > > really don't know what's going on in that file.
> >
> > OK, "each block" would be excessive if they all come from the
> > same place.  A single comment is probably fine.  I do see "Based on
> > the r8187 driver" at the top, but more information would be better.
> > Since Andrea is still around maybe the origin of that information is
> > still identifiable?
> >
> The r8187 driver is still around. (Realtek has it on their website) For the 
> most part, that information is comes from some Realtek engineer(s).

Fine.  How about a (hopefully permanent) URL?

> > [linville-t43.mobile]:> sparse example.c
> > example.c:15:12: warning: mixing different enum types
> > example.c:15:12:     int [signed] enum bar  versus
> > example.c:15:12:     int [signed] enum foo
> >
> That's what I thought.. but it's not that useful for me. Using the wrong 
> register bit definition is extremely unlikely to happen. (I've never done 
> it.) However, using the wrong size register read or write function happens 
> and has been caught a number of times by the register typechecking.

At the risk of belaboring the point... :-)

--------------------------------------------------------------------------------

/* definitions to make things work outside of kernel... */

#define __bitwise __attribute__((bitwise))
#define __force __attribute__((force))

typedef unsigned int __u32;
typedef __u32 __bitwise __le32;

typedef unsigned short __u16;
typedef __u16 __bitwise __le16;

/* example starts here... */

enum foo {
	FOO_BIT_BLAH	= (__force __le32)(1 << 1),
	FOO_BIT_BLECH	= (__force __le32)(1 << 2),
};

enum bar {
	BAR_BIT_BLAH	= (__force __le16)(1 << 3),
	BAR_BIT_BLECH	= (__force __le16)(1 << 4),
};

static void blather(void)
{
	enum foo drizzle;
	__le16 drazzle;

	drizzle = BAR_BIT_BLAH;

	drazzle = (__force __le16)5;
	drizzle = drazzle;
}

--------------------------------------------------------------------------------

[linville-t43.mobile]:> sparse -Wall example.c
example.c:29:10: warning: incorrect type in assignment (different base types)
example.c:29:10:    expected restricted unsigned int enum foo drizzle
example.c:29:10:    got restricted unsigned short enum bar [toplevel] BAR_BIT_BLAH
example.c:29:12: warning: mixing different enum types
example.c:29:12:     restricted unsigned short enum bar  versus
example.c:29:12:     restricted unsigned int enum foo
example.c:32:10: warning: incorrect type in assignment (different base types)
example.c:32:10:    expected restricted unsigned int enum foo drizzle
example.c:32:10:    got restricted unsigned short [assigned] [usertype] drazzle

Again, I don't really see this as a merge blocker.  But, it is worth
considering.

So, how about you sprinkle-in a couple more comments for the magic
copied from Realtek and resubmit?  Thanks!

John
-- 
John W. Linville
linville@tuxdriver.com