Re: [PATCH V8 12/13] ptp: Added a clock driver for the IXP46x.

Richard Cochran [off-list ref] writes:

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?

I think so. Or something similar.

Do you mean, you don't like the constant on the left hand side?

Yes.

Is that prohibited by CodingStyle or similar?

I don't think so. It's just a personal taste. I think it's based on
things learned in primary school, they teach to write (comparisons)
X = 4 instead of the other way around, and my brain seems to shock
a bit on the opposite.

I got into the habit of writing it that way to prevent a typo like:

	if (irq = NO_IRQ)

I see. Unfortunately it doesn't prevent typos like this when the right
side isn't a constant. Anyway gcc warns about them, even when both sides
are variable.

quoted

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.

Well, these two underscores mainly mean it's arch-dependent, but so are
the ixp4xx drivers. Using the __raw_read* directly is the preferred
method (or, perhaps, in such case, it's the only way).

Actually, I was thinking about changing the macros some time ago, and it
may eventually happen. But we'll fix all the code using them then.
-- 
Krzysztof Halasa