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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hi,

Sorry for not having followed closer this series.
It was submitted at the last minute and got too few comments.
I'll try to fix it now to be sure it will be one of the first series
ready for the 2.3 cycle.

2015-11-05 15:06, Daniel Mrzyglod:
> --- a/doc/guides/rel_notes/release_2_2.rst
> +++ b/doc/guides/rel_notes/release_2_2.rst
> @@ -222,6 +222,9 @@ API Changes
>  
>  * The devargs union field virtual is renamed to virt for C++ compatibility.
>  
> +* Add new functions in ethdev to support IEEE1588: rte_eth_timesync_time_adjust()
> +  rte_eth_timesync_time_get(), rte_eth_timesync_time_set()

No need to add an entry in API changes for new functions.

> +/**
> + * Read the time from the timesync clock on an Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param time
> + *   Pointer to the timespec struct.
> + *
> + * @return
> + *   - 0: Success.
> + */
> +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()?

Not related to this patch, but in rte_eth_timesync_read_rx_timestamp(),
the flags parameter breaks the API layer separation with drivers:
 * @param flags
 *   Device specific flags. Used to pass the RX timesync register index to
 *   i40e. Unused in igb/ixgbe, pass 0 instead.