| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<FR2PPF4571F02BCADA7BB96EF58E5AF67068C00A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Thu, 4 Sep 2025 13:05:35 +0000
From: Remi Buisson <Remi.Buisson@....com>
To: Jonathan Cameron <jic23@...nel.org>,
Remi Buisson via B4 Relay
<devnull+remi.buisson.tdk.com@...nel.org>
CC: David Lechner <dlechner@...libre.com>,
Nuno Sá
<nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring
<robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org"
<linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>
Subject: RE: [PATCH v5 3/9] iio: imu: inv_icm45600: add buffer support in iio
devices
>
>
>From: Jonathan Cameron <jic23@...nel.org>
>Sent: Monday, August 25, 2025 12:42 PM
>To: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org>
>Cc: Remi Buisson <Remi.Buisson@....com>; David Lechner <dlechner@...libre.com>; Nuno Sá <nuno.sa@...log.com>; Andy Shevchenko <andy@...nel.org>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; linux-kernel@...r.kernel.org; linux-iio@...r.kernel.org; devicetree@...r.kernel.org
>Subject: Re: [PATCH v5 3/9] iio: imu: inv_icm45600: add buffer support in iio devices
>
>On Wed, 20 Aug 2025 14:24:21 +0000
>Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org> wrote:
>
>> From: Remi Buisson <remi.buisson@....com>
>>
>> Add FIFO control functions.
>> Support hwfifo watermark by multiplexing gyro and accel settings.
>> Support hwfifo flush.
>>
>> Signed-off-by: Remi Buisson <remi.buisson@....com>
>
>A few really minor things inline.
Thanks!
>
>> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..50fd21a24e34decfbe10426946a51c61353eb6a9
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c
>
>> +
>> +/**
>> + * inv_icm45600_buffer_update_watermark - update watermark FIFO threshold
>> + * @st: driver internal state
>> + *
>> + * Returns 0 on success, a negative error code otherwise.
>> + *
>> + * 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.)
>> + * 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.
>> + */
>> +int inv_icm45600_buffer_update_watermark(struct inv_icm45600_state *st)
>> +{
>> + const size_t packet_size = sizeof(struct inv_icm45600_fifo_2sensors_packet);
>> + unsigned int wm_gyro, wm_accel, watermark;
>> + u32 period_gyro, period_accel, period;
>> + u32 latency_gyro, latency_accel, latency;
>> +
>> + /* Compute sensors latency, depending on sensor watermark and odr. */
>> + wm_gyro = inv_icm45600_wm_truncate(st->fifo.watermark.gyro, packet_size,
>> + st->fifo.period);
>> + wm_accel = inv_icm45600_wm_truncate(st->fifo.watermark.accel, packet_size,
>> + st->fifo.period);
>> + /* Use us for odr to avoid overflow using 32 bits values. */
>> + period_gyro = inv_icm45600_odr_to_period(st->conf.gyro.odr) / 1000UL;
>> + period_accel = inv_icm45600_odr_to_period(st->conf.accel.odr) / 1000UL;
>> + latency_gyro = period_gyro * wm_gyro;
>> + latency_accel = period_accel * wm_accel;
>> +
>> + /* 0 value for watermark means that the sensor is turned off. */
>> + if (wm_gyro == 0 && wm_accel == 0)
>> + return 0;
>> +
>> + if (latency_gyro == 0) {
>> + watermark = wm_accel;
>> + st->fifo.watermark.eff_accel = wm_accel;
>> + } else if (latency_accel == 0) {
>> + 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);
>> + /* Use the shortest period. */
>> + period = min(period_gyro, period_accel);
>> + /* All this works because periods are multiple of each others. */
>> + watermark = max(latency / period, 1);
>> + /* Update effective watermark. */
>> + st->fifo.watermark.eff_gyro = max(latency / period_gyro, 1);
>> + st->fifo.watermark.eff_accel = max(latency / period_accel, 1);
>> + }
>> +
>
>1 blank line is enough.
Ok.
>
>> +
>> + st->buffer.u16 = cpu_to_le16(watermark);
>> + return regmap_bulk_write(st->map, INV_ICM45600_REG_FIFO_WATERMARK,
>> + &st->buffer.u16, sizeof(st->buffer.u16));
>> +}
>>
>> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.h b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..da463461b5f2708014126f868fa6008db0520a4e
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.h
>
>
>> +
>> +static inline s16 inv_icm45600_fifo_get_sensor_data(__le16 d)
>> +{
>> + return le16_to_cpu(d);
>I'm not really seeing advantage of this wrapper vs just using le16_to_cpu()
>inline.
There is actually no reason, I'll simplify this.
>
>> +}
>> +
>> +static inline bool
>> +inv_icm45600_fifo_is_data_valid(const struct inv_icm45600_fifo_sensor_data *s)
>> +{
>> + s16 x, y, z;
>> +
>> + x = inv_icm45600_fifo_get_sensor_data(s->x);
>> + y = inv_icm45600_fifo_get_sensor_data(s->y);
>> + z = inv_icm45600_fifo_get_sensor_data(s->z);
> x = le16_to_cpu(s->x);
>
>is pretty obvious.
And far more straightforward.
>
>> +
>> + if (x == INV_ICM45600_DATA_INVALID &&
>> + y == INV_ICM45600_DATA_INVALID &&
>> + z == INV_ICM45600_DATA_INVALID)
>> + return false;
>> +
>> + return true;
>> +}
>
>
>
Powered by blists - more mailing lists