Krzysztof, Thanks for your review. I have some responses, below. But before that, I have a big favor to ask you. Can you please look at the TODO in my patch and give me a hint? I don't know how to relate the port instance (NPE A,B,C) to the two time stamping channels. On Thu, Jan 06, 2011 at 10:01:59PM +0100, Krzysztof Halasa wrote: > Richard Cochran <richardcochran@xxxxxxxxx> writes: > > + u32 SrcUUIDHi; /* 0x5C Sequence Identifier/Source UUID0 High */ > > I don't like these XxxYyyZzz either :-( Okay, I'll change that. > > +static void do_tx_timestamp(struct port *port, struct sk_buff *skb) > > +{ > > +#ifdef __ARMEB__ ... > > +#endif > > +} > > And what if we're little-endian? It is a NOOP in that case. > Why does it depend on BE? The time stamp code clones the skb, but the LE version frees the skb too early. Perhaps we can move that dev_kfree_skb(skb) in the LE case to be the last statement in eth_xmit(). What do you think? > > > @@ -1171,6 +1357,11 @@ static int __devinit eth_init_one(struct platform_device *pdev) > > char phy_id[MII_BUS_ID_SIZE + 3]; > > int err; > > > > + if (ptp_filter_init(ptp_filter, ARRAY_SIZE(ptp_filter))) { > > + pr_err("ixp4xx_eth: bad ptp filter\n"); > > + return -EINVAL; > > + } > > + > > if (!(dev = alloc_etherdev(sizeof(struct port)))) > > return -ENOMEM; > > Shouldn't it depend on CPU type? It won't hurt to init the BPF unconditionally. The important bits are checked with cpu_is_ixp46x(). > BTW which CPU is required? IXP46x (455/460/465)? Does it work on 43x? IIRC, it does not work on 43x. I don't know about 455 and 460, but I can find out... > > + if (NO_IRQ == irq) > > + return NO_IRQ; > > Don't like these either :-( Do you mean, you don't like the constant on the left hand side? Is that prohibited by CodingStyle or similar? I got into the habit of writing it that way to prevent a typo like: if (irq = NO_IRQ) > Not showstoppers but... > > Also I don't like the ixp_read/ixp_write() trivial macros. Why not > simply call __raw_readl() and __raw_writel()? Well, I have had the experience back in 2.4 days of having my drivers ruined by the changing IO macros in the kernel. The wrappers are supposed to help if that ever happens again. Seeing *two* leading underscores in the macro names certainly makes me nervous. Thanks again, Richard -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html