| 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: <20250928094524.52d492a9@jic23-huawei>
Date: Sun, 28 Sep 2025 09:45:24 +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 v6 3/9] iio: imu: inv_icm45600: add buffer support in
iio devices
On Wed, 24 Sep 2025 09:23:56 +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>
Hi Remi,
A few trivial things in here as well.
Jonathan
> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600.h b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
> index 5f637e2f2ec8f1537459459dbb7e8a796d0ef7a6..aac8cd852c12cfba5331f2b7c1ffbbb2ed23d1c7 100644
> --- a/drivers/iio/imu/inv_icm45600/inv_icm45600.h
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
> @@ -5,6 +5,7 @@
> #define INV_ICM45600_H_
>
> #include <linux/bits.h>
> +#include <linux/limits.h>
Why this in the header? Should be only needed in some of the c files I think
so push the include down there.
> #include <linux/mutex.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> @@ -14,6 +15,8 @@
> #include <linux/iio/common/inv_sensors_timestamp.h>
> #include <linux/iio/iio.h>
>
> +#include "inv_icm45600_buffer.h"
> +
> #define INV_ICM45600_REG_BANK_MASK GENMASK(15, 8)
> #define INV_ICM45600_REG_ADDR_MASK GENMASK(7, 0)
>
> @@ -94,6 +97,8 @@ struct inv_icm45600_sensor_conf {
> u8 filter;
> };
>
> +#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;
> @@ -122,6 +127,7 @@ struct inv_icm45600_chip_info {
> * @indio_accel: accelerometer IIO device.
> * @chip_info: chip driver data.
> * @timestamp: interrupt timestamps.
> + * @fifo: FIFO management structure.
> * @buffer: data transfer buffer aligned for DMA.
> */
> struct inv_icm45600_state {
> @@ -138,6 +144,7 @@ struct inv_icm45600_state {
> s64 gyro;
> s64 accel;
> } timestamp;
> + struct inv_icm45600_fifo fifo;
> union {
> u8 buff[2];
> __le16 u16;
> @@ -190,6 +197,7 @@ struct inv_icm45600_sensor_state {
> #define INV_ICM45600_FIFO_CONFIG0_MODE_BYPASS 0
> #define INV_ICM45600_FIFO_CONFIG0_MODE_STREAM 1
> #define INV_ICM45600_FIFO_CONFIG0_MODE_STOP_ON_FULL 2
> +#define INV_ICM45600_FIFO_CONFIG0_FIFO_DEPTH_MASK GENMASK(5, 0)
> #define INV_ICM45600_FIFO_CONFIG0_FIFO_DEPTH_MAX 0x1F
>
> #define INV_ICM45600_REG_FIFO_CONFIG2 0x0020
> 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..f81a85c5c77a0b0bc729734a1256af0c896c8fbd
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c
> @@ -0,0 +1,486 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright (C) 2025 Invensense, Inc. */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/minmax.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +
> +#include <asm/byteorder.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/common/inv_sensors_timestamp.h>
> +#include <linux/iio/iio.h>
> +
> +#include "inv_icm45600_buffer.h"
> +#include "inv_icm45600.h"
> +
> +/* FIFO header: 1 byte */
> +#define INV_ICM45600_FIFO_EXT_HEADER BIT(7)
> +#define INV_ICM45600_FIFO_HEADER_ACCEL BIT(6)
> +#define INV_ICM45600_FIFO_HEADER_GYRO BIT(5)
> +#define INV_ICM45600_FIFO_HEADER_HIGH_RES BIT(4)
> +#define INV_ICM45600_FIFO_HEADER_TMST_FSYNC GENMASK(3, 2)
> +#define INV_ICM45600_FIFO_HEADER_ODR_ACCEL BIT(1)
> +#define INV_ICM45600_FIFO_HEADER_ODR_GYRO BIT(0)
> +
> +struct inv_icm45600_fifo_1sensor_packet {
> + u8 header;
> + struct inv_icm45600_fifo_sensor_data data;
> + s8 temp;
> +} __packed;
> +
> +struct inv_icm45600_fifo_2sensors_packet {
> + u8 header;
> + struct inv_icm45600_fifo_sensor_data accel;
> + struct inv_icm45600_fifo_sensor_data gyro;
> + s8 temp;
> + __le16 timestamp;
> +} __packed;
> +
> +int inv_icm45600_buffer_set_fifo_en(struct inv_icm45600_state *st,
> + unsigned int fifo_en)
> +{
> + unsigned int mask, val;
> + int ret;
> +
> + mask = INV_ICM45600_FIFO_CONFIG3_GYRO_EN |
> + INV_ICM45600_FIFO_CONFIG3_ACCEL_EN;
> +
> + if ((fifo_en & INV_ICM45600_SENSOR_GYRO) || (fifo_en & INV_ICM45600_SENSOR_ACCEL))
> + val = (INV_ICM45600_FIFO_CONFIG3_GYRO_EN | INV_ICM45600_FIFO_CONFIG3_ACCEL_EN);
maybe val = mask;?
> + else
> + val = 0;
> +
> + ret = regmap_update_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG3, mask, val);
Could use
ret = regmap_assign_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG3, mask,
(fifo_en & INV_ICM45600_SENSOR_GYRO) ||
(fifo_en & INV_ICM45600_SENSOR_ACCEL));
or something like.
if ((fifo_en & INV_ICM45600_SENSOR_GYRO) || (fifo_en & INV_ICM45600_SENSOR_ACCEL))
ret = regmap_set_bits(st->map, INV_IC..., mask);
else
ret = regmap_clear_bits();
Anyhow, up to you onthis one as none of the options look perfect to me.
> + if (ret)
> + return ret;
> +
> + st->fifo.en = fifo_en;
> + inv_icm45600_buffer_update_fifo_period(st);
> +
> + return 0;
> +}
> 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..0c8caa8287dd4373cf11bb6c7b913a6c49e9eee5
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.h
> +
> +/**
> + * struct inv_icm45600_fifo - FIFO state variables
> + * @on: reference counter for FIFO on.
> + * @en: bits field of INV_ICM45600_SENSOR_* for FIFO EN bits.
> + * @period: FIFO internal period.
> + * @watermark: watermark configuration values for accel and gyro.
Given the contents of this to me look like things to also document.e
* @watermark.gyro: ....
etc as well would be good to add
> + * @count: number of bytes in the FIFO data buffer.
> + * @nb: gyro, accel and total samples in the FIFO data buffer.
This is more obvious. Check if the kernel-doc script minds these subfields not
being defined. If it does, add a the trivial documentation just to squash warnings
and make it easier to spot real issues.
> + * @data: FIFO data buffer aligned for DMA (8kB)
> + */
> +struct inv_icm45600_fifo {
> + unsigned int on;
> + unsigned int en;
> + u32 period;
> + struct {
> + unsigned int gyro;
> + unsigned int accel;
> + unsigned int eff_gyro;
> + unsigned int eff_accel;
> + } watermark;
> + size_t count;
> + struct {
> + size_t gyro;
> + size_t accel;
> + size_t total;
> + } nb;
> + u8 *data;
> + unsigned int count);
> +
> +#endif
Powered by blists - more mailing lists