| 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:
<FR2PPF4571F02BC17D1C2EFD573E55F9C428C2AA@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Wed, 13 Aug 2025 10:08:11 +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 v3 3/8] iio: imu: inv_icm45600: add buffer support in iio
devices
>
>
>From: Jonathan Cameron <jic23@...nel.org>
>Sent: Thursday, July 24, 2025 6:06 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 v3 3/8] iio: imu: inv_icm45600: add buffer support in iio devices
>
>On Thu, 17 Jul 2025 13:25:55 +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 minor things inline.
>
>> ---
>> drivers/iio/imu/inv_icm45600/Makefile | 1 +
>> drivers/iio/imu/inv_icm45600/inv_icm45600.h | 7 +
>> drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c | 514 +++++++++++++++++++++
>> drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.h | 99 ++++
>> drivers/iio/imu/inv_icm45600/inv_icm45600_core.c | 137 +++++-
>> 5 files changed, 757 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/imu/inv_icm45600/Makefile b/drivers/iio/imu/inv_icm45600/Makefile
>> index 4f442b61896e91647c7947a044949792bae06a30..19c521ffba17b0d108a8ecb45ecdea35dff6fd18 100644
>> --- a/drivers/iio/imu/inv_icm45600/Makefile
>> +++ b/drivers/iio/imu/inv_icm45600/Makefile
>> @@ -2,3 +2,4 @@
>>
>> obj-$(CONFIG_INV_ICM45600) += inv-icm45600.o
>> inv-icm45600-y += inv_icm45600_core.o
>> +inv-icm45600-y += inv_icm45600_buffer.o
>> \ No newline at end of file
>
>Fix this by adding one.
Ok.
>
>> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600.h b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
>> index 59aed59b94ca2d4709b0c986ddeda80b33064f90..5625c437b6f54336f6e652c2ae2e4ea8f88e2396 100644
>> --- a/drivers/iio/imu/inv_icm45600/inv_icm45600.h
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
>
>>
>> +#define INV_ICM45600_SENSOR_CONF_KEEP_VALUES {U8_MAX, U8_MAX, U8_MAX, U8_MAX, }
>> +
>> struct inv_icm45600_conf {
>> struct inv_icm45600_sensor_conf gyro;
>> struct inv_icm45600_sensor_conf accel;
>> @@ -127,6 +132,7 @@ extern const struct inv_icm45600_chip_info inv_icm45689_chip_info;
>> * @indio_gyro: gyroscope IIO device.
>> * @indio_accel: accelerometer IIO device.
>> * @timestamp: interrupt timestamps.
>> + * @fifo: FIFO management structure.
>> * @buffer: data transfer buffer aligned for DMA.
>> */
>> struct inv_icm45600_state {
>> @@ -143,6 +149,7 @@ struct inv_icm45600_state {
>> s64 gyro;
>> s64 accel;
>> } timestamp;
>> + struct inv_icm45600_fifo fifo;
>
>Having that 8 thousand element buffer in here seem unwise. Normally I encourage putting
>everything possible in here, but this maybe a case where a kzalloc() for that buffer is
>going to wasted less space.
Seems fine, I'll implement it using devm_kzalloc.
>
>> union {
>> u8 buff[2];
>> __le16 u16;
>> 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..b37607e52721097daf6362bac20001b0d4210f4b
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c
>> @@ -0,0 +1,514 @@
>
>>
>> +void inv_icm45600_buffer_update_fifo_period(struct inv_icm45600_state *st)
>> +{
>> + u32 period_gyro, period_accel, period;
>> +
>> + if (st->fifo.en & INV_ICM45600_SENSOR_GYRO)
>> + period_gyro = inv_icm45600_odr_to_period(st->conf.gyro.odr);
>> + else
>> + period_gyro = U32_MAX;
>> +
>> + if (st->fifo.en & INV_ICM45600_SENSOR_ACCEL)
>> + period_accel = inv_icm45600_odr_to_period(st->conf.accel.odr);
>> + else
>> + period_accel = U32_MAX;
>> +
>> + if (period_gyro <= period_accel)
>> + period = period_gyro;
>
> st->fifo.period = min(period_gyro, period_accel);
Ok.
>
>> + else
>> + period = period_accel;
>> +
>> + st->fifo.period = period;
>> +}
>
>> +
>> +static unsigned int inv_icm45600_wm_truncate(unsigned int watermark, size_t packet_size,
>> + unsigned int fifo_period)
>> +{
>> + size_t watermark_max, grace_samples;
>> +
>> + /* Keep 20ms for processing FIFO. */
>> + grace_samples = (20U * 1000000U) / fifo_period;
>
>I'mnot sure what the multipler here is but maybe 20U * MICRO
fifo_period is in ns, and the grace is in ms.
So I'm not sure using MICRO will ease the understanding here.
I could evetually use MEGA.
I will improve the above comment anyway.
>> + if (grace_samples < 1)
>> + grace_samples = 1;
>> +
>> + watermark_max = INV_ICM45600_FIFO_SIZE_MAX / packet_size;
>> + watermark_max -= grace_samples;
>> +
>> + if (watermark > watermark_max)
>> + watermark = watermark_max;
>
>min?
Yes!
>
>> +
>> + return watermark;
>> +}
>
>
>
Powered by blists - more mailing lists