Michael Wu wrote:
On Saturday 15 September 2007 17:32, Jeff Garzik wrote:
Review summary: many minor issues, only one major one: irq handler loop
CCing me would help.
Sorry. I just hit 'reply to all'... apparently you were not CC'd on
the submission.
+ if (flags & IFF_PROMISC)
+ dev->flags |= IEEE80211_HW_RX_INCLUDES_FCS;
+ else
+ dev->flags &= ~IEEE80211_HW_RX_INCLUDES_FCS;
why does promisc dictate inclusion of FCS?
Because that's the way the hardware works.
Why not always include it, regardless of promisc?
+ } while (count++ < 20);
NAK -- talk about back to the past.
It's bogus to loop in the interrupt handler, then loop again in both RX
and TX paths. You are getting close to reinventing the wheel here.
Use NAPI rather than doing such a loop.
This is old interrupt handler code from Jouni's original driver. I never
bothered to replace it with the simpler designs used in newer drivers I've
worked on.
Also - mac80211 drivers have no access to NAPI.
Not true as of net-2.6.24.
+/* CSR (Host Control and Status Registers) */
+struct adm8211_csr {
[snip]
+} __attribute__ ((packed));
attributed(packed) is unneccesary here, and its use results in
sub-optimal code
How? Doesn't this just turn into a bunch of offsets?
It depends on how its used in the code.
enums are preferred. they do not disappear at the cpp stage, and confer
type information.
I'd rather not.
Elaboration?
Remember this code is not just for you, but for all people working with
the code. It makes debugging (and sometimes tracebacks) much more
readable, since it hasn't been eaten by cpp.
Thanks,
Jeff
-
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
