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

[Satyam Gupta <sgsatyam@google.com>]

Hi Tzung-Bi,

Thanks for the review!

On Tue, Jul 28, 2026 at 10:15:52AM +0000, Tzung-Bi Shih wrote:
> > This CL makes 5 specific changes to the clock drift estimation algorithm
> 
> Since these are 5 distinct logical changes, shouldn't they be separated into
> a 5-patch series?

Although the v3 commit description listed the adjustments as 5 bullet points,
they are tightly coupled parts of a single fix to support sensor batching in
the timestamp filter without breaking intermediate states:

- Extending TS_HISTORY_BORED_US from 0.5s to 2.0s so the filter survives 1-2s
  batch flush intervals requires the 10x dx ODR/mode-change reset and slope
  clamping; otherwise, transitions between active streaming and batching (which
  previously exceeded the 0.5s timeout) mix into the same filter history.
- The 10x dx check (which reads state->x_history[1] when history_len > 1)
  depends directly on the loop start index fix: previously, with
  `i = state->history_len - 1`, the loop did not run when `history_len == 1`,
  leaving `x_history[1]` unpopulated when `history_len` became 2.
- Dropping strictly past timestamps instead of clamping them to `last_ts`
  prevents post-reset backward ticks from being mistaken for same-timestamp
  batches by `cros_ec_sensor_ring_spread_add()`.

I have reworded the commit message in v4 to explain how these pieces work
together as a single cohesive change rather than a numbered list of unrelated
items.

> > and timestamp filtering to resolve monotonic backward skews and OOO errors
>                                                                   ^^^
> Please spell out the acronym (i.e., Out Of Order) for clarity.
> 
> > during batching:
> > 
> > 1. Clamp the calculated drift slope (m) to a realistic maximum bound
> >    (+/- 200 PPM). Huge jitter spikes previously produced astronomical
> 
> Same here: P..? Per Million?
> 
> > Signed-off-by: SATYAM GUPTA <sgsatyam@google.com>
> 
> Please fix the capitalization of your name to match the author line.

Will spell out Out Of Order (OOO) and Parts Per Million (PPM), and fix the
Signed-off-by capitalization in v4.

> > +#define TS_HISTORY_BORED_US 2000000 // 2 seconds
> 
> The inline comment is redundant.
> 
> > +	start = (state->history_len == CROS_EC_SENSORHUB_TS_HISTORY_SIZE) ?
> > +		state->history_len - 1 : state->history_len;
> 
> How about:
>     min_t(int, state->history_len, CROS_EC_SENSORHUB_TS_HISTORY_SIZE - 1);
> 
> > -static void
> > +static bool
> >  cros_ec_sensor_ring_check_for_past_timestamp(struct cros_ec_sensorhub
> >  					     *sensorhub,
> >  					     struct cros_ec_sensors_ring_sample
> 
> It'd be better to rename the function to make its intent clear.  E.g.:
> 
>     cros_ec_sensor_ring_is_past_timestamp()
> 
> And have it return true if it's a past timestamp, adjusting the caller
> logic accordingly.

Ack on all three. Updated in v4.

> On a related note, is [1] still relevant?  Should we disregard it?
> 
> [1] https://lore.kernel.org/all/20260720110405.2127011-1-sgsatyam@google.com

Yes, [1] ("platform/chrome: cros_ec_sensorhub: Fix AP/EC TS wrap") is still
relevant — it addresses 32-bit EC timestamp rollover across deep AP suspend.

Best regards,
Satyam Gupta