Re: [Linuxptp-devel] [PATCHv2 2/6] Refactor time stamp processing.

[Richard C. <ric...@gm...>]

Few gripes, and a question...

On Tue, Mar 17, 2015 at 11:28:21AM +0100, Miroslav Lichvar wrote:
> +struct tsproc {
> +	/* Current ratio between remote and local clock frequency */
> +	double clock_rate_ratio;
> +
> +	/* Latest down measurement */
> +	tmv_t t1, t2;
> +	/* Latest up measurement */
> +	tmv_t t3, t4;

One field per line, please.

> +
> +	/* Current filtered delay */
> +	tmv_t filtered_delay;
> +
> +	/* Delay filter */
> +	struct filter *delay_filter;
> +};

> +int tsproc_update_delay(struct tsproc *tsp, tmv_t *delay)
> +{
> +	tmv_t raw_delay;
> +
> +	if (!tsp->t1 || !tsp->t2 || !tsp->t3 || !tsp->t4)
> +		return -1;

Use tmv_zero() here ...

> +	raw_delay = get_raw_delay(tsp);
> +	tsp->filtered_delay = filter_sample(tsp->delay_filter, raw_delay);
> +
> +	pr_debug("delay   filtered %10" PRId64 "   raw %10" PRId64,
> +		 tsp->filtered_delay, raw_delay);
> +
> +	if (delay)
> +		*delay = tsp->filtered_delay;
> +
> +	return 0;
> +}
> +
> +int tsproc_update_offset(struct tsproc *tsp, tmv_t *offset)
> +{
> +	tmv_t delay;
> +
> +	if (!tsp->t1 || !tsp->t2 || !tsp->t3 || !tsp->t4)
> +		return -1;

and here.

> +	if (!offset)
> +		return 0;

What is point of allowing 'offset' to be a null pointer?

> +	delay = tsp->filtered_delay;
> +
> +	/* offset = t2 - t1 - delay */
> +	*offset = tmv_sub(tmv_sub(tsp->t2, tsp->t1), delay);
> +
> +	return 0;
> +}

Thanks,
Richard