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

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

On Thu, Jul 23, 2026 at 04:38:38PM +0000, Satyam Gupta 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?

> 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.

>  /*
>   * If we don't have any history entries for this long, empty the filter to
> - * make sure there are no big discontinuities.
> + * make sure there are no big discontinuities (e.g. after suspend or long idle).
> + *
> + * This is set to 2 seconds to accommodate normal batching flush intervals
> + * (which can be up to 1-2 seconds) without constantly resetting the filter,
> + * while still ensuring a reset occurs during actual long gaps or suspend.
>   */
> -#define TS_HISTORY_BORED_US 500000
> +#define TS_HISTORY_BORED_US 2000000 // 2 seconds
                                       ^^^^^^^^^^^^
The inline comment is redundant.

>  	/* Move everything over, also update offset to all absolute coords .*/
> -	for (i = state->history_len - 1; i >= 1; i--) {
> +	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.

---

On a related note, is [1] still relevant?  Should we disregard it?

[1] https://lore.kernel.org/all/20260720110405.2127011-1-sgsatyam@google.com