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.
> 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.
> > > 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).
> ---------------------------------------------------------------------------
>-----
>
> 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
>
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.
> 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.
>
Yup, it's intentional.
-Michael Wu
Attachment:
pgp56HXhraQfu.pgp
Description: PGP signature
