> From: Richard Cochran [mailto:richardcochran@xxxxxxxxx] > Sent: 14 kwietnia 2017 20:29 > To: Rafal Ozieblo <rafalo@xxxxxxxxxxx> > Cc: David Miller <davem@xxxxxxxxxxxxx>; nicolas.ferre@xxxxxxxxx; > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > harinikatakamlinux@xxxxxxxxx; harini.katakam@xxxxxxxxxx; > Andrei.Pistirica@xxxxxxxxxxxxx > Subject: Re: [PATCH 3/4] net: macb: Add hardware PTP support > > On Thu, Apr 13, 2017 at 02:39:23PM +0100, Rafal Ozieblo wrote: (...) > > +static int gem_tsu_get_time(struct macb *bp, struct timespec64 *ts) > > +{ > > + long first, second; > > + u32 secl, sech; > > + unsigned long flags; > > + > > + if (!bp || !ts) > > + return -EINVAL; > > Useless test. Sorry for me being too carefulness, I'll remove all tests. > (...) > > +static int gem_ptp_gettime(struct ptp_clock_info *ptp, struct timespec64 > *ts) > > +{ > > + struct macb *bp = container_of(ptp, struct macb, ptp_clock_info); > > + > > + if (!ptp || !ts) > > + return -EINVAL; > > Useles test. > > What is the point of this wrapper function anyhow? Please remove it. gem_ptp_gettime() is assigned in ptp_clock_info and it has to have ptp_clock_info pointer as first parameter. gem_tsu_get_time() is used in the source code but with macb pointer. Do you want me to do something like: gem_ptp_gettime(macb->ptp, ts); and first would be getting macb pointer from ptp ? struct macb *bp = container_of(ptp, struct macb, ptp_clock_info); > > > + > > + gem_tsu_get_time(bp, ts); > > + return 0; > > +} > > + > > +static int gem_ptp_settime(struct ptp_clock_info *ptp, > > + const struct timespec64 *ts) > > +{ > > + struct macb *bp = container_of(ptp, struct macb, ptp_clock_info); > > + > > + if (!ptp || !ts) > > + return -EINVAL; > > Another useless function. ditto > > > + gem_tsu_set_time(bp, ts); > > + return 0; > > +} > > + > > +static int gem_ptp_enable(struct ptp_clock_info *ptp, > > + struct ptp_clock_request *rq, int on) > > +{ > > + struct macb *bp = container_of(ptp, struct macb, ptp_clock_info); > > + > > + if (!ptp || !rq) > > + return -EINVAL; > > Sigh. > > > + > > + switch (rq->type) { > > + case PTP_CLK_REQ_EXTTS: /* Toggle TSU match interrupt */ > > + if (on) > > + macb_writel(bp, IER, MACB_BIT(TCI)); > > No locking to protect IER and IDE? There is no need. > > > + else > > + macb_writel(bp, IDR, MACB_BIT(TCI)); > > + break; > > + case PTP_CLK_REQ_PEROUT: /* Toggle Periodic output */ > > + return -EOPNOTSUPP; > > + /* break; */ > > + case PTP_CLK_REQ_PPS: /* Toggle TSU periodic (second) > interrupt */ > > + if (on) > > + macb_writel(bp, IER, MACB_BIT(SRI)); > > + else > > + macb_writel(bp, IDR, MACB_BIT(SRI)); > > + break; > > + default: > > + break; > > + } > > + return 0; > > +} > > + (...) > > -- > > 2.4.5 > > > > Thanks, > Richard -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
