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

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

On Sun, May 13, 2007 at 12:07:28PM +0200, Andrea Merello wrote:
> On 5/13/07, Michael Wu <flamingice@sourmilk.net> 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?

<speculation snipped>

> A bit verbose...I think writing all those suppositions on the code is
> not very good, I'm not sure enough about those interpretations.. And
> those are just the obvious things all people who will read the code
> will imagine by themselves, I suppose.
 
See the part you quoted immediately below.

> >> 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?
> 
> ..And the original code I wrote contains those delays because Realtek
> gave me sample programming code that contanined them, or, for some of
> those delays Realtek changed the code directly or told me that they
> are needed to make sure the chip work correctly..

Fine.  Would you mind send a patch to Michael or me putting a comment
to that effect into the code?

> 
> >> > +   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.
> 
> I agree with Michael. I think filling file with a lot of define is
> useful only if those defines improve readability of the code, by
> clarify what those values means. This is not the case.

Bare hexadecimal isn't too wonderful either.  Anyway, see my reply
to Michael's post asking for a comment explaining the code fragment's
origin as an alternative.

> 
> >> 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.
> 
> Absolutely agree with Michael ;-)
> 
> Indeed I think that a note at the beginning of the file that notice
> about the fact that most is magic should be enough. I can't see any

ACK, as in my reply to Michael.

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