Re: [PATCH v4] platform/chrome: cros_ec_sensorhub: Modify clock drift estimation

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Tue, Oct 13, 2026 at 10:57:53AM +0000, Satyam Gupta wrote:
>  /* Precision of fixed point for the m values from the filter */
>  #define M_PRECISION BIT(23)
> +#define MAX_DRIFT_PPM 200LL
> +#define MAX_M ((s64)(MAX_DRIFT_PPM * M_PRECISION / 1000000))
...
> @@ -271,13 +277,36 @@ cros_ec_sensor_ring_ts_filter_update(struct cros_ec_sensors_ts_filter_state
>  		return; /* we already have this irq in the history */
>  	dy = (state->y_history[0] + state->y_offset) - y;
>  	m = div64_s64(dy * M_PRECISION, dx);
> +	/*
> +	 * Clamp the calculated slope to a realistic range (+/- 200 ppm).
> +	 * Temporary large jitter spikes can produce huge slopes (e.g. +/- 20000 ppm)
> +	 * that would otherwise corrupt the median filter history.
> +	 */
> +	m = clamp(m, -MAX_M, MAX_M);

[This is one independent patch.]

There is a unit mismatch here that makes MAX_M off by a factor of 1000,
clamping the drift slope to +/-0.2 PPM instead of the intended +/-200
PPM.

See cros_ec_sensor_ring_ts_filter_update():
- dx is in us; dy is in ns.
- m = dy/dx is actually off by a factor of 1000.

> +
> +	/*
> +	 * Detect ODR (Output Data Rate) or Mode changes (e.g., transition from
> +	 * active streaming to low-frequency batching). If the time delta (dx)
> +	 * changes by more than 10x in either direction, reset the filter history
> +	 * to prevent mixing noisy active samples with batching samples.
> +	 */
> +	if (state->history_len > 1) {
> +		s64 prev_dx = -state->x_history[1];
> +
> +		if (prev_dx > 0 && (abs(dx) > 10 * prev_dx || prev_dx > 10 * abs(dx))) {
> +			state->history_len = 0;
> +			m = 0;
> +		}
> +	}

[This is one independent patch.]

Setting `m = 0;` is unnecessary.  Setting `state->history_len = 0;` is
sufficient to reset the filter.

The code elsewhere uses `-dx`.  Using `abs(dx)` is inconsistent and would
treat backward timestamps as valid deltas.

>  
>  	/* Empty filter if we haven't seen any action in a while. */
>  	if (-dx > TS_HISTORY_BORED_US)
>  		state->history_len = 0;
>  
>  	/* Move everything over, also update offset to all absolute coords .*/
> -	for (i = state->history_len - 1; i >= 1; i--) {
> +	start = min_t(int, state->history_len,
> +		      CROS_EC_SENSORHUB_TS_HISTORY_SIZE - 1);
> +	for (i = start; i >= 1; i--) {
>  		state->x_history[i] = state->x_history[i - 1] + dx;
>  		state->y_history[i] = state->y_history[i - 1] + dy;
>  

[This is one independent patch.]

> -static void
> -cros_ec_sensor_ring_check_for_past_timestamp(struct cros_ec_sensorhub
> -					     *sensorhub,
> -					     struct cros_ec_sensors_ring_sample
> -					     *sample)
> +static bool
> +cros_ec_sensor_ring_is_past_timestamp(struct cros_ec_sensorhub *sensorhub,
> +				      struct cros_ec_sensors_ring_sample *sample)
>  {
>  	const u8 sensor_id = sample->sensor_id;
>  
> -	/* If this event is earlier than one we saw before... */
> +	/*
> +	 * If this event is strictly earlier than one we saw before, it is truly
> +	 * out-of-order and must be dropped to preserve monotonicity.
> +	 *
> +	 * We intentionally allow duplicate timestamps (==) to pass through here
> +	 * because the EC/ISH may report a batch of samples with the same raw
> +	 * timestamp. These duplicates are required by the downstream spreading
> +	 * logic (cros_ec_sensor_ring_spread_add) to interpolate unique,
> +	 * monotonic timestamps for each sample in the batch.
> +	 */
>  	if (sensorhub->batch_state[sensor_id].newest_sensor_event >
>  	    sample->timestamp)
> -		/* mark it for spreading. */
> -		sample->timestamp =
> -			sensorhub->batch_state[sensor_id].last_ts;
> -	else
> -		sensorhub->batch_state[sensor_id].newest_sensor_event =
> -			sample->timestamp;
> +		return true;
> +
> +	sensorhub->batch_state[sensor_id].newest_sensor_event =
> +		sample->timestamp;
> +	return false;
>  }
> 
>  /**
> @@ -547,8 +582,10 @@  cros_ec_sensor_ring_process_event(struct cros_ec_sensorhub *sensorhub,
>  	for (axis = 0; axis < 3; axis++)
>  		out->vector[axis] = in->data[axis];
> 
> -	if (sensorhub->tight_timestamps)
> -		cros_ec_sensor_ring_check_for_past_timestamp(sensorhub, out);
> +	if (sensorhub->tight_timestamps) {
> +		if (cros_ec_sensor_ring_is_past_timestamp(sensorhub, out))
> +			return false;
> +	}

[This is one independent patch.]

Dropping a sample without setting `batch_state[sensor_id].last_len = 0`
leaves the previous batch length intact.  The next batch will compute
`sample_period` across the gap using the stale `last_len`.