Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588 support functions

["Mcnamara, John" <john.mcnamara@intel.com>]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, November 10, 2015 11:58 AM
> To: Mcnamara, John
> Cc: Mrzyglod, DanielX T; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 1/7] ethdev: add additional ieee1588
> support functions
> 
> 2015-11-10 11:36, Mcnamara, John:
> > From: Thomas Monjalon
> > > I'll try to fix it now to be sure it will be one of the first series
> > > ready for the 2.3 cycle.
> >
> > These comments are minor and could be fixed now.
> 
> After having a closer look in the drivers change, it seems to be
> restricted to the PTP functions of the Intel drivers.
> So you can ask to the Intel validation team if they are OK to add it in
> RC2.
> I think it would be a wrong idea because we need to stop moving the ethdev
> and drivers code, and focus on other DPDK areas for the RC2.

Hi Thomas,

Ok. I'll ask the validation team to evaluate the effect of the patches.


> 
> > > > +extern int rte_eth_timesync_time_get(uint8_t port_id,
> > > > +	      struct timespec *time);
> > >
> > > How is it different from rte_eth_timesync_read_rx_timestamp() and
> > > rte_eth_timesync_read_tx_timestamp()?
> > >
> > > Why repeating the word time? Why not rte_eth_timesync_get()?
> >
> > In the context of PTP there is a difference between the time (os or NIC)
> and the timestamp (either in the mbuf, a register or as part of the
> payload).
> 
> Do you think we can make it clear in the definition of these functions?

Yes. I think that could be made clearer. I'll fix that and the others.


> 
> More wording comments:
> - rte_eth_timesync_time_get
> - rte_eth_timesync_read_rx_timestamp
> Why is it "get" in a case and "read" in another?
> Why the verb is at the end in the first and before the complement in the
> latter?

My preference would be for verb_noun but I think noun_verb was used for consistency with the rest of the Ethdev API (although that isn't quite consistent either). And yes, read() would be more consistent with the timesync API while get() is more consistent with the rest of the Ethdev API. I think it would be best, in this case, to try maintain self-consistency and use rte_eth_timesync_read_time().


John.
--