Re: [PATCH 2/2] iio: invensense: improve period measurement by using a longer delay

[Jean-Baptiste Maneyrol]

>________________________________________
>From: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>
>Sent: Saturday, July 18, 2026 03:25
>To: Jean-Baptiste Maneyrol via B4 Relay
>Cc: Jean-Baptiste Maneyrol; David Lechner; Nuno Sá; Andy Shevchenko; linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH 2/2] iio: invensense: improve period measurement by using a longer delay
>
>On Fri, 17 Jul 2026 14: 39: 41 +0200 Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste. maneyrol. tdk. com@ kernel. org> wrote: > From: Jean-Baptiste Maneyrol <jean-baptiste. maneyrol@ tdk. com> > > Period measurement can
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>
>On Fri, 17 Jul 2026 14:39:41 +0200
>Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@xxxxxxxxxx> wrote:
>
>> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>>
>> Period measurement can be difficult when using high sampling
>> frequency where the jitter criteria is hard to meet because of the
>> system jitter.
>>
>> This new version is using the delta time between 2 distant interrupts
>> to measure an interval of at least 20ms. 20ms is a good compromise
>> between the mitigation of system jitter and the delay to update
>> period. This way we decorrelate the period measurement from the
>> interrupt timestamps syncing using only the 2 last interrupts.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260717-iio-common-inv-sensors-timestamp-rework-v1-0-d1afee2805cd*40tdk.com__;IyU!!FtrhtPsWDhZ6tw!GQDZDRdZZcOJgpTEH9_gv20w-fCFfDYpFwab-M0OXzd0CgGQoA-cLwvrZsKCmlhaPjFSWVwndREG4x-7BMrZ9knTBO-QVszCO8nNaH1iGA$[sashiko[.]dev]
>
>Some comments. To me looks like a mixed bag. Please take a look.
>
>> ---
>>  .../iio/common/inv_sensors/inv_sensors_timestamp.c | 31 +++++++++++++++++-----
>>  include/linux/iio/common/inv_sensors_timestamp.h   |  6 +++++
>>  2 files changed, 31 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
>> index 2c89e0edc87d..63c1d8a2e19a 100644
>> --- a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
>> +++ b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
>> @@ -16,6 +16,9 @@
>>  #define INV_SENSORS_TIMESTAMP_MAX(_val, _jitter)             \
>>       (((_val) * (1000 + (_jitter))) / 1000)
>>
>> +/* minimum timestamp delta between 2 interrupts for measuring period (20ms) */
>> +#define INV_SENSORS_MIN_IT_DELTA     (20 * NSEC_PER_MSEC)
>> +
>>  /* Add a new value inside an accumulator and update the estimate value */
>>  static void inv_update_acc(struct inv_sensors_timestamp_acc *acc, uint32_t val)
>>  {
>> @@ -121,10 +124,13 @@ static uint32_t inv_align_timestamp_it(struct inv_sensors_timestamp *ts,
>>  void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
>>                                    size_t sample_nb, int64_t timestamp)
>>  {
>> +     const int64_t delta_threshold =
>> +                     INV_SENSORS_TIMESTAMP_MIN(INV_SENSORS_MIN_IT_DELTA,
>> +                                               ts->chip.jitter);
>>       struct inv_sensors_timestamp_interval *it;
>>       int64_t delta, interval;
>>       uint32_t period;
>> -     bool valid = false;
>> +     bool valid;
>>
>>       if (sample_nb == 0)
>>               return;
>> @@ -136,16 +142,29 @@ void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
>>               ts->timestamp = timestamp - interval;
>>       }
>>
>> +     /* update delta timestamps and estimated period */
>> +     it = &ts->delta;
>> +     ts->delta_counter += sample_nb;
>> +     delta = timestamp - it->up;
>> +     if (delta >= delta_threshold) {
>> +             it->lo = it->up;
>> +             it->up = timestamp;
>> +             if (it->lo != 0) {
>> +                     /* compute period: delta time divided by number of samples */
>> +                     delta = it->up - it->lo;
>> +                     period = div_s64(delta, sample_nb);
>
>Sashiko asks if that should be delta_counter on the bottom.  Seems plausible
>to me.

Hello Jonathan,

Sashiko is right here, good catch. I will fix it in V2.

>
>
>> +                     inv_update_chip_period(ts, period);
>> +             }
>> +             ts->delta_counter = 0;
>> +     }
>> +
>>       /* update interrupt timestamp and compute chip and sensor periods */
>>       it = &ts->it;
>>       it->lo = it->up;
>>       it->up = timestamp;
>>       delta = it->up - it->lo;
>> -     if (it->lo != 0) {
>> -             /* compute period: delta time divided by number of samples */
>> -             period = div_s64(delta, sample_nb);
>> -             valid = inv_update_chip_period(ts, period);
>> -     }
>> +     period = div_s64(delta, sample_nb);
>> +     valid = inv_validate_period(ts, period);
>>
>>       /* if interrupt interval is valid, sync with interrupt timestamp */
>>       ts->period = valid ? inv_align_timestamp_it(ts, sample_nb) :
>> diff --git a/include/linux/iio/common/inv_sensors_timestamp.h b/include/linux/iio/common/inv_sensors_timestamp.h
>> index 8d506f1e9df2..bc04831f54c4 100644
>> --- a/include/linux/iio/common/inv_sensors_timestamp.h
>> +++ b/include/linux/iio/common/inv_sensors_timestamp.h
>> @@ -46,6 +46,8 @@ struct inv_sensors_timestamp_acc {
>>   * @min_period:              minimal acceptable clock period
>>   * @max_period:              maximal acceptable clock period
>>   * @it:                      interrupts interval timestamps
>> + * @delta:           interval timestamps between several interrupts
>> + * @delta_counter:   number of data samples in the delta interval
>>   * @timestamp:               store last timestamp for computing next data timestamp
>>   * @mult:            current internal period multiplier
>>   * @new_mult:                new set internal period multiplier (not yet effective)
>> @@ -57,6 +59,8 @@ struct inv_sensors_timestamp {
>>       uint32_t min_period;
>>       uint32_t max_period;
>>       struct inv_sensors_timestamp_interval it;
>> +     struct inv_sensors_timestamp_interval delta;
>> +     uint32_t delta_counter;
>>       int64_t timestamp;
>>       uint32_t mult;
>>       uint32_t new_mult;
>> @@ -88,6 +92,8 @@ static inline void inv_sensors_timestamp_reset(struct inv_sensors_timestamp *ts)
>>       const struct inv_sensors_timestamp_interval interval_init = {0LL, 0LL};
>>
>>       ts->it = interval_init;
>> +     ts->delta = interval_init;
>> +     ts->delta_counter = 0;
>>       ts->timestamp = 0;
>>  }
>>
>>

Sashiko remarks about possible timestamp overflowing interrupt timestamp is
also interesting. I will fix that by moving new_period to s64 type, thus
resulting negative value will be processed correctly by clamp.

Thanks,
JB