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?
> > + 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.
> > + /* setup card */
> > + rtl818x_iowrite8(priv, (u8 *)0xFF85, 0);
> > + rtl818x_iowrite8(priv, &priv->map->GPIO, 0);
> > +
> > + rtl818x_iowrite8(priv, (u8 *)0xFF85, 4);
>
> More magic addresses...
>
These are known. I'll add comments.
> > + /* host_usb_init */
> > + rtl818x_iowrite8(priv, (u8 *)0xFF85, 0);
> > + rtl818x_iowrite8(priv, &priv->map->GPIO, 0);
> > + reg = rtl818x_ioread8(priv, (u8 *)0xFE53);
> > + rtl818x_iowrite8(priv, (u8 *)0xFE53, reg | (1 << 7));
> > + rtl818x_iowrite8(priv, (u8 *)0xFF85, 4);
>
> And more...
>
Some of these are known too.
> > + rtl818x_iowrite16(priv, (__le16 *)0xFFFE, 0x10);
> > + rtl818x_iowrite8(priv, &priv->map->TALLY_SEL, 0x80);
> > + rtl818x_iowrite8(priv, (u8 *)0xFFFF, 0x60);
>
> And more...
>
> > +static void rtl8187_register_read(struct eeprom_93cx6 *eeprom)
> > +{
> > + struct ieee80211_hw *dev = eeprom->data;
> > + struct rtl8187_priv *priv = dev->priv;
> > + u8 reg = rtl818x_ioread8(priv, &priv->map->EEPROM_CMD);
> > +
> > + eeprom->reg_data_in = reg & RTL818X_EEPROM_CMD_WRITE;
> > + eeprom->reg_data_out = reg & RTL818X_EEPROM_CMD_READ;
> > + eeprom->reg_data_clock = reg & RTL818X_EEPROM_CMD_CK;
> > + eeprom->reg_chip_select = reg & RTL818X_EEPROM_CMD_CS;
> > +}
> > +
> > +static void rtl8187_register_write(struct eeprom_93cx6 *eeprom)
> > +{
> > + struct ieee80211_hw *dev = eeprom->data;
> > + struct rtl8187_priv *priv = dev->priv;
> > + u8 reg = RTL818X_EEPROM_CMD_PROGRAM;
> > +
> > + if (eeprom->reg_data_in)
> > + reg |= RTL818X_EEPROM_CMD_WRITE;
> > + if (eeprom->reg_data_out)
> > + reg |= RTL818X_EEPROM_CMD_READ;
> > + if (eeprom->reg_data_clock)
> > + reg |= RTL818X_EEPROM_CMD_CK;
> > + if (eeprom->reg_chip_select)
> > + reg |= RTL818X_EEPROM_CMD_CS;
> > +
> > + rtl818x_iowrite8(priv, &priv->map->EEPROM_CMD, reg);
> > + udelay(10);
> > +}
>
> It seems like these should be named "rtl8187_eeprom_register_{read,write}"
> instead? Current names seem like you are operating on the wireless
> parts instead of just the eeprom.
>
Sure.
> > + if (!is_valid_ether_addr(dev->wiphy->perm_addr)) {
> > + printk(KERN_WARNING "rtl8187: Invalid hwaddr! Using randomly generated
> > MAC address\n");
>
> I would prefer to see lines wrapped at 80 columns (or less) if at all
> possible.
>
OK.
> > + for (i = 0; i < 3; i++) {
> > + eeprom_93cx6_read(&eeprom, 0x16 + i, &txpwr);
> > + (*channel++).val = txpwr & 0xFF;
> > + (*channel++).val = txpwr >> 8;
> > + }
> > + for (i = 0; i < 2; i++) {
> > + eeprom_93cx6_read(&eeprom, 0x3D + i, &txpwr);
> > + (*channel++).val = txpwr & 0xFF;
> > + (*channel++).val = txpwr >> 8;
> > + }
> > + for (i = 0; i < 2; i++) {
> > + eeprom_93cx6_read(&eeprom, 0x1B + i, &txpwr);
> > + (*channel++).val = txpwr & 0xFF;
> > + (*channel++).val = txpwr >> 8;
> > + }
> > +
> > + eeprom_93cx6_read(&eeprom, 0x05, &priv->txpwr_base);
>
> Magic offsets for the eeprom data...
>
Will add comments.
> 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.
> > + __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?
> 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.
Thanks,
-Michael Wu
Attachment:
pgpDN96FdRB8W.pgp
Description: PGP signature
