| 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: <20250825114205.65ec79bc@jic23-huawei>
Date: Mon, 25 Aug 2025 11:42:05 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org>
Cc: 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.
> 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.
> +
> + 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.
> +}
> +
> +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.
> +
> + 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