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