Re: [PATCH v2 3/8] iio: imu: inv_icm45600: add buffer support in iio devices

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 11 Aug 2025 14:13:54 +0000
Remi Buisson <Remi.Buisson@tdk.com> wrote:

> >
> >
> >From: Jonathan Cameron <jic23@kernel.org> 
> >Sent: Thursday, July 17, 2025 4:34 PM
> >To: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@kernel.org>
> >Cc: Remi Buisson <Remi.Buisson@tdk.com>; David Lechner <dlechner@baylibre.com>; Nuno Sá <nuno.sa@analog.com>; Andy Shevchenko <andy@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; devicetree@vger.kernel.org
> >Subject: Re: [PATCH v2 3/8] iio: imu: inv_icm45600: add buffer support in iio devices
> >On Thu, 10 Jul 2025 08:57:58 +0000
> >Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@kernel.org> wrote:
> >  
> >> From: Remi Buisson <remi.buisson@tdk.com>
> >> 
> >> Add FIFO control functions.
> >> Support hwfifo watermark by multiplexing gyro and accel settings.
> >> Support hwfifo flush.
> >> 
> >> Signed-off-by: Remi Buisson <remi.buisson@tdk.com>  
> >Hi Remi,
> >
> >Sorry for delay - hectic week.
> >
> >Jonathan  
> No problem, thanks for the review ! (and sorry for my late reply)
> >  
> >> ---
> >>  drivers/iio/imu/inv_icm45600/Makefile              |   1 +
> >>  drivers/iio/imu/inv_icm45600/inv_icm45600.h        |   4 +
> >>  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 +++++-  
> >We used to do the buffer / core split a lot but it often ends up more trouble
> >that it is worth and we no longer make buffer support a build time option (which was
> >what motivated the separate files)  Consider how much simplification you'd get by squashing them into
> >one file.    
> I understand the point.
> However merging files will allow to remove 5 lines at most,
> while the length of the core file will increase a lot.
> I'm not sure of the benefit in the end, but 
> please let me know if you really want me to proceed with the merge.

It's only a combined 1.5k. That would be fine even if the savings are fairly small.

It's not something I care that much about though.





> >> +const struct iio_buffer_setup_ops inv_icm45600_buffer_ops = {
> >> +	.preenable = inv_icm45600_buffer_preenable,
> >> +	.postenable = inv_icm45600_buffer_postenable,
> >> +	.predisable = inv_icm45600_buffer_predisable,
> >> +	.postdisable = inv_icm45600_buffer_postdisable,
> >> +};
> >> +
> >> +int inv_icm45600_buffer_fifo_read(struct inv_icm45600_state *st,
> >> +				  unsigned int max)  
> >What is max here?  Seems to be passed 0 in the only caller.  
> Function call with max > 0 is implemented later in the same patch
> (in 4/8, from inv_icm45600_buffer_hwfifo_flush).

Maybe push the parameter being introduced to ther.

> >> +{
> >> +	const ssize_t packet_size = INV_ICM45600_FIFO_2SENSORS_PACKET_SIZE;
> >> +	__le16 *raw_fifo_count;
> >> +	size_t fifo_nb, i;
> >> +	ssize_t size;
> >> +	const struct inv_icm45600_fifo_sensor_data *accel, *gyro;
> >> +	const __le16 *timestamp;
> >> +	const s8 *temp;
> >> +	unsigned int odr;
> >> +	int ret;
> >> +
> >> +	/* Reset all samples counters. */
> >> +	st->fifo.count = 0;
> >> +	st->fifo.nb.gyro = 0;
> >> +	st->fifo.nb.accel = 0;
> >> +	st->fifo.nb.total = 0;
> >> +
> >> +	/* Read FIFO count value. */
> >> +	raw_fifo_count = &st->buffer.u16;
> >> +	ret = regmap_bulk_read(st->map, INV_ICM45600_REG_FIFO_COUNT,
> >> +			       raw_fifo_count, sizeof(*raw_fifo_count));  
> >
> >For IIO drivers at least we still operated under some guidance the regmap maintainer
> >gave years ago to never assume regmap (for busses that otherwise require DMA safe
> >buffers) will always bounce the data.  So bulk reads with SPI buffers need
> >DMA safe buffers. Easiest is usually an __aligned(IIO_DMA_MINALIGN) buffer
> >(or set of buffers) at the end of st.
> >
> >In practice last time I checked regmap doesn't bother with the zero copy
> >optimization that would need this safety so you won't actually hit this
> >issue.  
> From my understanding, alignment of &st->buffer.u16 is correct because the union is aligned,
> 
> 	union {
> 		u8 buff[2];
> 		__le16 u16;
> 	} buffer __aligned(IIO_DMA_MINALIGN);
> 
> Please let me know if my answer is not correct.
Ah. I probably just missed that.  can't remember!

> >