Re: [PATCH 3/4] iio: imu: inv_icm42600: simplify watermark computation by using gcd

[Jean-Baptiste Maneyrol]

>
>
>
>________________________________________
>From: Jonathan Cameron <jic23@xxxxxxxxxx>
>Sent: Friday, August 21, 2026 02:14
>To: Jean-Baptiste Maneyrol via B4 Relay
>Cc: Jean-Baptiste Maneyrol; David Lechner; Nuno Sá; Andy Shevchenko; linux-iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jean-Baptiste Maneyrol
>Subject: Re: [PATCH 3/4] iio: imu: inv_icm42600: simplify watermark computation by using gcd
>
>On Thu, 20 Aug 2026 21: 02: 39 +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> > > The watermark computation
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>
>On Thu, 20 Aug 2026 21:02:39 +0200
>Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@xxxxxxxxxx> wrote:
>
>> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>>
>> The watermark computation was in fact resulting in computing the gcd
>
>Maybe GCD if we are referring to it by acronym rather than talking
>about the function that implements it.

Hello Jonathan,

no problem, will fix that in V2.

Thanks,
JB

>
>> of the latencies when both sensors are on. Gcd is required because
>> of the IIO buffer watermark. We need to use a divider of IIO buffer
>> watermark, otherwise we will overflow the requested watermark.
>>
>> Move to use gcd and update documentation accordingly.
>
>gcd()
>as this is about the function I think.
>
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>> ---
>>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 47 ++++++++++------------
>>  1 file changed, 21 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> index 043ae9deee65..1428f18408ce 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> @@ -5,6 +5,7 @@
>>
>>  #include <linux/delay.h>
>>  #include <linux/device.h>
>> +#include <linux/gcd.h>
>>  #include <linux/kernel.h>
>>  #include <linux/minmax.h>
>>  #include <linux/mutex.h>
>> @@ -172,15 +173,14 @@ static unsigned int inv_icm42600_wm_truncate(unsigned int watermark,
>>   *
>>   * FIFO watermark threshold is computed based on the required watermark values
>>   * set for gyro and accel sensors. Since watermark is all about acceptable data
>> - * latency, use the smallest setting between the 2. It means choosing the
>> - * smallest latency but this is not as simple as choosing the smallest watermark
>> - * value. Latency depends on watermark and ODR. It requires several steps:
>> - * 1) compute gyro and accel latencies and choose the smallest value.
>> - * 2) adapt the chosen latency so that it is a multiple of both gyro and accel
>> - *    ones. Otherwise it is possible that you don't meet a requirement. (for
>> - *    example with gyro @100Hz wm 4 and accel @100Hz with wm 6, choosing the
>> - *    value of 4 will not meet accel latency requirement because 6 is not a
>> - *    multiple of 4. You need to use the value 2.)
>> + * latency, we should need to use the smallest latency value. But it is not as
>> + * simple as choosing the smallest watermark value. Latency depends on watermark
>> + * and ODR and IIO buffer watermark adds another requirement. The required steps:
>> + * 1) compute gyro and accel periods and latencies
>> + * 2) Use the smallest period and the gcd of the latencies. Gcd is required
>
>GCD here as well.
>
>> + *    because of the IIO buffer watermark that will prevent send of data if not
>> + *    crossed. Thus accel and gyro watermarks must be a multiple of the watermark
>> + *    value. Computing the gcd gives us the biggest value that meets this criteria.
>>   * 3) Since all periods are multiple of each others, watermark is computed by
>>   *    dividing this computed latency by the smallest period, which corresponds
>>   *    to the FIFO frequency. Beware that this is only true because we are not
>> @@ -190,7 +190,7 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
>>  {
>>       size_t packet_size, wm_size;
>>       unsigned int wm_gyro, wm_accel, watermark;
>> -     u32 period_gyro, period_accel;
>> +     u32 period_gyro, period_accel, period;
>>       u32 latency_gyro, latency_accel, latency;
>>       bool restore;
>>       __le16 raw_wm;
>> @@ -218,22 +218,17 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
>>               watermark = wm_gyro;
>>               st->fifo.watermark.eff_gyro = wm_gyro;
>>       } else {
>> -             /* compute the smallest latency that is a multiple of both */
>> -             if (latency_gyro <= latency_accel)
>> -                     latency = latency_gyro - (latency_accel % latency_gyro);
>> -             else
>> -                     latency = latency_accel - (latency_gyro % latency_accel);
>> -             /* all this works because periods are multiple of each others */
>> -             watermark = latency / min(period_gyro, period_accel);
>> -             if (watermark < 1)
>> -                     watermark = 1;
>> -             /* update effective watermark */
>> -             st->fifo.watermark.eff_gyro = latency / period_gyro;
>> -             if (st->fifo.watermark.eff_gyro < 1)
>> -                     st->fifo.watermark.eff_gyro = 1;
>> -             st->fifo.watermark.eff_accel = latency / period_accel;
>> -             if (st->fifo.watermark.eff_accel < 1)
>> -                     st->fifo.watermark.eff_accel = 1;
>> +             /*
>> +              * In case of both accel and gyro enabled, we need to use the
>> +              * shortest period and the gcd of the latencies. Gcd is required
>> +              * because of the IIO buffer watermark that will prevent data
>> +              * sending if we are not crossing the watermark level.
>> +              */
>> +             period = min(period_gyro, period_accel);
>> +             latency = gcd(latency_gyro, latency_accel);
>> +             watermark = max(latency / period, 1);
>> +             st->fifo.watermark.eff_gyro = max(latency / period_gyro, 1);
>> +             st->fifo.watermark.eff_accel = max(latency / period_accel, 1);
>>       }
>>
>>       /* compute watermark value in bytes */
>>
>