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

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

On Sat, May 12, 2007 at 09:56:39PM -0400, Michael Wu wrote:
> On Saturday 12 May 2007 15:18, John W. Linville wrote:
> > On Fri, May 11, 2007 at 04:02:18PM -0400, Michael Wu wrote:
> > > +void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data)
> > > +{
> > > +	struct rtl8187_priv *priv = dev->priv;
> > > +
> > > +	data <<= 8;
> > > +	data |= addr | 0x80;
> > > +
> > > +	rtl818x_iowrite8(priv, &priv->map->PHY[3], (data >> 24) & 0xFF);
> > > +	rtl818x_iowrite8(priv, &priv->map->PHY[2], (data >> 16) & 0xFF);
> > > +	rtl818x_iowrite8(priv, &priv->map->PHY[1], (data >> 8) & 0xFF);
> > > +	rtl818x_iowrite8(priv, &priv->map->PHY[0], data & 0xFF);
> > > +
> > > +	msleep(1);
> > > +}
> >
> > msleep seems better than mdelay, but why is it there at all?  There is
> > no need to speculate.  Just give us a comment for why you put it there,
> > even if it is "copied from app note" or somesuch.
> >
> Magic (copied from the original code). There are many magic seeming delays in 
> the code.. why single this one out?

Not really singling it out.  Anyway, see response to next block.

> > > +	msleep(200);
> > > +	rtl818x_iowrite8(priv, (u8 *)0xFE18, 0x10);
> > > +	rtl818x_iowrite8(priv, (u8 *)0xFE18, 0x11);
> > > +	rtl818x_iowrite8(priv, (u8 *)0xFE18, 0x00);
> > > +	msleep(200);
> >
> > Please comment these magic delays too, and give us a symbolic constant
> > for the magic addres.  Yes, "RTL8187_MAGIC_INIT_ADDR_1" is better than a
> > raw number. :-)
> >
> I can't say I agree on that. If it's just a number without any comments, it's 
> most likely magic. I don't want to put in #defines for constants which are 
> used once and merely serve the purpose of saying I don't know what it does. 
> That is counterproductive IMHO.

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?

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?
 
> > 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?
 
> > > +	__le32	TX_CONF;
> > > +#define RTL818X_TX_CONF_LOOPBACK_MAC	(1 << 17)
> > > +#define RTL818X_TX_CONF_NO_ICV		(1 << 19)
> > > +#define RTL818X_TX_CONF_DISCW		(1 << 20)
> > > +#define RTL818X_TX_CONF_R8180_ABCD	(2 << 25)
> > > +#define RTL818X_TX_CONF_R8180_F		(3 << 25)
> > > +#define RTL818X_TX_CONF_R8185_ABC	(4 << 25)
> > > +#define RTL818X_TX_CONF_R8185_D		(5 << 25)
> > > +#define RTL818X_TX_CONF_HWVER_MASK	(7 << 25)
> > > +#define RTL818X_TX_CONF_CW_MIN		(1 << 31)
> >
> > Using an enum for a sparsely defined bitmask like this should let the
> > compiler identify if we misuse a bitmask in the wrong place.
> >
> How? Can you give an example?

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

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

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

void blather(void)
{
	enum foo drizzle;

	drizzle = BAR_BIT_BLAH;
}

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

[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

> > Do we lose the benefits of the __le32 typechecking by using an enum?
> > There is probably some way to force that...
> >
> Bitmasks and register offsets are rarely typechecked in the first place. Why 
> does rtl8187 need to be so much better? I don't see any problem with the 
> bitmask definitions in rtl8187, as the register name prefixes make it obvious 
> what bits should go with which registers.

I don't know if I consider this a merge blocker.  Still, if sparse can
help us find "thinko" bugs it would be better to enable it to do so.

Interdiff from the prior version also shows this:

@@ -485,25 +485,16 @@
 
 	rtl818x_iowrite8(priv, &priv->map->SIFS, 0x22);
 
-	if (conf->flags & IEEE80211_CONF_SHORT_SLOT_TIME)
+	if (conf->flags & IEEE80211_CONF_SHORT_SLOT_TIME) {
 		rtl818x_iowrite8(priv, &priv->map->SLOT, 0x9);
-	else
+		rtl818x_iowrite8(priv, &priv->map->DIFS, 0x14);
+		rtl818x_iowrite8(priv, &priv->map->EIFS, 91 - 0x14);
+		rtl818x_iowrite8(priv, &priv->map->CW_VAL, 0x73);
+	} else {
 		rtl818x_iowrite8(priv, &priv->map->SLOT, 0x14);
-
-	switch (conf->phymode) {
-	case MODE_IEEE80211B:
 		rtl818x_iowrite8(priv, &priv->map->DIFS, 0x24);
 		rtl818x_iowrite8(priv, &priv->map->EIFS, 91 - 0x24);
 		rtl818x_iowrite8(priv, &priv->map->CW_VAL, 0xa5);
-		break;
-	case MODE_IEEE80211G:
-		rtl818x_iowrite8(priv, &priv->map->DIFS, 0x14);
-		rtl818x_iowrite8(priv, &priv->map->EIFS, 91 - 0x14);
-		rtl818x_iowrite8(priv, &priv->map->CW_VAL, 0x73);
-		break;
-	default:
-		BUG();
-		break;
 	}
 
 	rtl818x_iowrite16(priv, &priv->map->ATIM_WND, 2);

Which seems alright, but I wanted to make sure it was intentional.

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