Hi Dan.
On Mon, 31 Mar 2008, Dan Williams wrote:
So a few comments... the driver as-is needs to be cleaned up for a few
general issues, inline below. Do you have some time to address the
cleanups, or would you like some help with them?
I am working on it. Some comments inline.
Kill this file; as long as the rest of the files have the short GPL,
it's OK, as the kernel is already distributed under the GPL.
OK.
[...]
+ifeq ($(CONFIG_MRV8K_DEBUG),y)
+EXTRA_CFLAGS += -DDEBUG
+endif
Don't do this; in your code, just use:
#ifdef CONFIG_MRV8K_DEBUG
<debug stuff>
#endif
Don't need additional defines in the makefile.
OK.
[...]
+#if !defined(MAC_ARG)
+#define MAC_ARG(x) (\
+((u8 *)(x))[0], ((u8 *)(x))[1], \
+((u8 *)(x))[2], ((u8 *)(x))[3], \
+((u8 *)(x))[4], ((u8 *)(x))[5])
+#endif
Remove this; just use the kernel definitions in ethdev.h or whatever it
is. If the define is already in kernel git, it doesn't need to be in
the driver. If you want to keep compat with older kernels, you get to
maintain those bits out-of-tree as additional patches on top of the
upstream kernel.
OK, it was only used once anyway. Instead I am gonna use perm_addr
directly.
[...]
+static void _mrv_send_cmd(struct mrv_priv *priv, u32 addr)
+{
+ /* printk(KERN_ERR"_mrv_send_cmd\n");*/
Either remove the commented out code, or set up debugging prints that
are switched on/off with CONFIG_MRV8K_DEBUG. Or better yet, have a
'debug' module parameter that can dynamically update the debugging
output if the driver has been compiled with CONFIG_MRV8K_DEBUG by
echoing something else to /sys/modules/mrv8k/parameters/debug.
Setting up debugging prints for the moment.
[...]
+ } else {
+ printk(KERN_ERR"status %x\n", priv->cmd_status);
Formatting issues; you'll want to run the patch through checkpatch.pl
before submitting.
I was running it through checkpatch.pl, might have slipped through the
warnings for 80 chars/line.
[...]
+ mrv_int_enable(priv);
+ return IRQ_NONE;
+ }
+
+ if (status & 0x4) {
Any idea if these status bits can be #defined somewhere instead of being
magic numbers?
Defining them in the header.
+/*
+static int mrv_set_allowed_rates(struct mrv_priv *priv, int allowg)
If the function is commented out, it should probably either be removed
entirely and added back later when it works.
I am uncommenting the function. Did not want to through code away, that
was in the original driver without reason.
[...]
Any idea why the .flags bits are commented out?
I have added #include <net/ieee80211.h>, but the compiler complains with
error "'IEEE80211_CCK_RATE_1MB_MASK' undeclared here (not in a function)"
and I don't know how to fix it.
[...]
I think this was already shadowed above? Since it's included in
mrv8k.c, doesn't need to be here too.
OK.
[...]
+ int AntNum;
+ int FwVer;
+ int HwVer;
+ int TxRingNum;
+ int RegionCode;
+ int TxHwDma0;
+ int TxHwDma1;
+ int TxHwDma2;
+ int TxHwDma3;
+ int RxHwDmaR;
+ int RxHwDmaW;
Any chance you could un-studly-caps the variables? Normal kernel style
is first_second_third, so in this case it would be like "rx_hw_dma_w"
not "RxHwDmaW".
No problem.
Thanks,
Markus
--
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
