lkml.org 
[lkml]   [2011]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V8 12/13] ptp: Added a clock driver for the IXP46x.
    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@gmail.com> 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


    \
     
     \ /
      Last update: 2011-01-07 18:11    [W:0.027 / U:0.424 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site