On Thu, 19 Jul 2007, Li YanBo wrote:
> On 7/19/07, Johannes Berg <johannes@xxxxxxxxxxxxxxxx> wrote:
>
> > It might be useful for you to put all your current code into a git tree
> > and publish that to linux-wireless@xxxxxxxxxxxxxxx so other people can
> > take a look too :)
> >
>
> First of all, the code is far away be finished and full of bugs, We
> can't success control the RX and TX yet.
It's important to have an early reviews too though not all issues
are not yet resolved.
> Thanks for all your suggestions and fixes, I'll fix it later.
> Because the code is big , so I attach it in this mail. (Be attention,
> it is far from finished and has tons of bugs).
Here are some things which you'll probably encounter when you try to get
this stuff merged, so you'll have to adjust the code sooner or later
(these can easily add many unnecessary post-comment round-trips when you
would like to get it merged asap):
- You seem to define assert and never use it, besides you could probably
use either WARN_ON or BUG_ON which belongs to kernel's generic machinery
already...
- agnx_read32 & agnx_write32
* change to static inline, solves also the use of implicit
variable (reg) as you have return & assign instead (implicit
variables confuse readers, see Documentation/CodingStyle)
* You seem to be also doing direct access using ioread/write32
here and there?!?
- no // comments allowed, use /* */ instead, however, most of them are
for dead code anyway which just makes things messy looking and should be
removed (I'd say that removing them speeds up review too)... Besides,
if you use repository, there's no need to keep dead code lying around.
> +
> +#ifdef CONFIG_PM
> +static int agnx_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> [...snip...]
> +
> + return 0;
> +}
Usually they ask you to add these...:
#else
#define agnx_pci_suspend NULL
#define agnx_pci_resume NULL
> +#endif /* CONFIG_PM */
> +
> +static struct pci_driver agnx_pci_driver = {
> + .name = "agnx-pci",
> + .id_table = agnx_pci_id_tbl,
> + .probe = agnx_pci_probe,
> + .remove = __devexit_p(agnx_pci_remove),
> +#ifdef CONFIG_PM
> + .suspend = agnx_pci_suspend,
> + .resume = agnx_pci_resume,
> +#endif /* CONFIG_PM */
> +};
...and then to remove this #ifdefing.
> + /* FIXME */
> + reg = (bssid[0] << 24) | (bssid[1] << 16) | (bssid[2] << 8) | bssid[3];
> + agnx_write32(ctl_mem, AGNX_RXMC_BSSIDHI, reg);
> +
> + agnx_read32(ctl_mem, AGNX_RXMC_BSSIDLO);
> + reg &= 0xffff0000;
Define this BSSIDLO_MASK and use it here with ~ operator.
> + reg |= (bssid[4] << 8) | bssid[5];
> + agnx_write32(ctl_mem, AGNX_RXMC_BSSIDLO, reg);
...Maybe the "FIXME" already indicates the questionability of that
bssid endianess stuff...? The same comments apply to MACHI/LO stuff...
- I'm not sure if the comments per reg in phy_init() are that useful if
the same information is found in the header file already (just a matter
of taste though, shouldn't prevent merging, just IMHO)
> + /* Clear the Disable Tx interrupt bit in Interrupt Mask */
> + agnx_read32(ctl_mem, AGNX_INT_MASK);
> + reg &= ~0x20000;
> + agnx_write32(ctl_mem, AGNX_INT_MASK, reg);
You should not use numeric constant then (besides, IRQ_TX_DISABLE is
already defined!), if the purpose of this is known (there are more this
kind of things I suppose)... Then the comment would not be necessary any
more either as the code itself would explain the same to the reader... :-)
Obviously some/many values (in other places) will remain unknown as
reverse-engineered and that's something we just have to live with but
that's no excuse for known bits...
> +/* Recieve Management Control Registers */
typo
> +#define calibra_delay(priv) \
> + do { \
> + udelay(40); \
> + } while (0)
static inline
- ...other endianess issues should be dealt with...
> +#define FIR_TABLE_NUM 24
> +static const u32
> +tx_fir_table[FIR_TABLE_NUM] = { 0x19, 0x5d, 0xce, 0x151, 0x1c3, 0x1ff, 0x1ea,
[...snip...]
> + for (i = 0; i < FIR_TABLE_NUM; i++)
Drop FIR_TABLE_NUM and use ARRAY_SIZE macro in the for construct (and
elsewhere if there are other users). It's available in linux/kernel.h,
also check if your code implements something that is already given in
linux/kernel.h and replace them with the generic machinery. Same applies
to #define GAIN_TABLE_NUM 32 (and others?)
...those people specialized to wireless stuff have probably some / many
additional notes too...
--
i.
-
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
