lkml.org 
[lkml]   [2017]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH 3/4] net: macb: Add hardware PTP support
    Date
    > From: Richard Cochran [mailto:richardcochran@gmail.com]
    > Sent: 14 kwietnia 2017 20:29
    > To: Rafal Ozieblo <rafalo@cadence.com>
    > Cc: David Miller <davem@davemloft.net>; nicolas.ferre@atmel.com;
    > netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
    > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
    > harinikatakamlinux@gmail.com; harini.katakam@xilinx.com;
    > Andrei.Pistirica@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

    \
     
     \ /
      Last update: 2017-05-02 15:57    [W:3.581 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site