On 11/10/06, David Miller <[EMAIL PROTECTED]> wrote:
Please use GREG_STAT_* instead of magic constants for the
interrupt mask and ACK register writes. In fact, there are
some questionable values you use, in particular this one:
+static inline void gem_ack_int(struct gem *gp)
+{
+ writel(0x3f, gp->regs + GREG_IACK);
+}
This code clears bits 0 through 6, which GREG_IACK is for.
There is no bit defined in GREG_STAT_* for 0x08, but you
set it in this magic bitmask. It is another reason not
to use magic constants like this :-)
Yes, the bit 4 isn't used, but I assumed clearing it shouldn't be prolem.
Would you prefer that:
#define GREG_STAT_IACK 0x3f
static inline void gem_ack_int(struct gem *gp)
{
writel(GREG_STAT_IACK, gp->regs + GREG_IACK);
}
or that:
#define GREG_STAT_IACK (GREG_STAT_TXINTME | GREG_STAT_TXALL | \
GREG_STAT_RXDONE | GREG_STAT_RXNOBUF)
to avoid bit 4, and bit 3 (TX_DONE) which is masked.
Personnaly, I like the first way best.
Also, if you need to use an attachment to get the tabbing
right, that's fine, but please also provide a copy inline
so that it is easy to quote the patch for review purposes.
It's a truly a pain in the rear to quote things when you use
a binary attachment.
I will.
I'd like these very simple and straightforward issues to
be worked out before I even begin to review the actual
locking change itself.
Ok.
I'll wait for you regarding gem_ack_int() and send out another patch.
Thanks,
--
Eric
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
