> From: Richard Cochran [mailto:richardcoch...@gmail.com]
> Sent: 14 kwietnia 2017 20:29
> To: Rafal Ozieblo <raf...@cadence.com>
> Cc: David Miller <da...@davemloft.net>; nicolas.fe...@atmel.com;
> net...@vger.kernel.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> harinikatakamli...@gmail.com; harini.kata...@xilinx.com;
> andrei.pistir...@microchip.com
> 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
