Message-ID:
 <FR2PPF4571F02BCCFD984FDD99C69CAE7298C00A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Thu, 4 Sep 2025 13:01:32 +0000
From: Remi Buisson <Remi.Buisson@....com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
CC: Jonathan Cameron <jic23@...nel.org>,
        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: Andy Shevchenko <andriy.shevchenko@...el.com> 
>Sent: Thursday, August 21, 2025 11:21 AM
>To: Remi Buisson <Remi.Buisson@....com>
>Cc: Jonathan Cameron <jic23@...nel.org>; 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, Aug 20, 2025 at 02:24:21PM +0000, Remi Buisson via B4 Relay wrote:
>> 
>> Add FIFO control functions.
>> Support hwfifo watermark by multiplexing gyro and accel settings.
>> Support hwfifo flush.
>
Thanks again for the review.
>...
>
>> +#define INV_ICM45600_SENSOR_CONF_KEEP_VALUES {U8_MAX, U8_MAX, U8_MAX, U8_MAX, }
>
>When one line, no need to have inner trailing comma, besides that missed space.
The trailing comma was a request from Jonathan Cameron on the v2 of patchset.
Let me know, if you disagree with him and I'll fix.

And I'll add a space before first element.

>
>...
>
>> +{
>> +	unsigned int mask, val;
>> +	int ret;
>> +
>> +	/* Update only FIFO EN bits. */
>> +	mask = INV_ICM45600_FIFO_CONFIG3_GYRO_EN |
>> +	       INV_ICM45600_FIFO_CONFIG3_ACCEL_EN;
>
>> +	val = 0;
>
>Why not to put it under else branch?

Agreed.
>
>> +	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);
>> +
>> +	ret = regmap_update_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG3, mask, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	st->fifo.en = fifo_en;
>> +	inv_icm45600_buffer_update_fifo_period(st);
>> +
>> +	return 0;
>> +}
>
>...
>
>> +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. fifo_period is in ns */
>> +	grace_samples = (20U * 1000000U) / fifo_period;
>
>We have NSEC_PER_MSEC, other similar constants, check respective time.h and units.h.

Sure.
>
>> +	if (grace_samples < 1)
>> +		grace_samples = 1;
>> +
>> +	watermark_max = INV_ICM45600_FIFO_SIZE_MAX / packet_size;
>> +	watermark_max -= grace_samples;
>> +
>> +	return min(watermark, watermark_max);
>
>Where is the header for min()?

I will review headers for this file as well
>
>> +}
>
>...
>
>> +/**
>> + * inv_icm45600_buffer_update_watermark - update watermark FIFO threshold
>> + * @st:	driver internal state
>
>> + * Returns 0 on success, a negative error code otherwise.
>
>Return section is missed and has to be the last one. Please, read how to create
>proper kernel-doc.

I'll fix.

>
>> + * 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;
>
>Replace 1000UL by the respective predefined constants.

I'll do it.
>
>> +	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);
>> +	}
>> +
>> +
>
>One blank line is enough.

I'll remove it.
>
>> +	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));
>> +}
>
>...
>
>...
>
>> +	if (sensor == INV_ICM45600_SENSOR_GYRO)
>> +		ret = inv_icm45600_set_gyro_conf(st, &conf, sleep);
>> +	else
>> +		ret = inv_icm45600_set_accel_conf(st, &conf, sleep);
>> +
>> +	return ret;
>
>Just return directly in the branches.

Ok.
>
>...
>
>> +int inv_icm45600_buffer_fifo_read(struct inv_icm45600_state *st)
>> +{
>> +	const ssize_t packet_size = sizeof(struct inv_icm45600_fifo_2sensors_packet);
>> +	__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));
>> +	if (ret)
>> +		return ret;
>
>+ blank line
Yes.
>
>> +	fifo_nb = le16_to_cpup(raw_fifo_count);
>> +
>> +	/* Check and limit number of samples if requested. */
>> +	if (fifo_nb == 0)
>> +		return 0;
>
>Better to reformat as
>
>	/* Check and limit number of samples if requested. */
>	fifo_nb = le16_to_cpup(raw_fifo_count);
>	if (fifo_nb == 0)
>		return 0;
>
Definitely better.
>> +	/* Try to read all FIFO data in internal buffer. */
>> +	st->fifo.count = fifo_nb * packet_size;
>> +	ret = regmap_noinc_read(st->map, INV_ICM45600_REG_FIFO_DATA,
>> +				st->fifo.data, st->fifo.count);
>> +	if (ret == -ENOTSUPP || ret == -EFBIG) {
>
>Strictly speaking this is a bit of layering issue, do we have other means to
>check the support before even trying?

No unfortunately, we can't with current I3C regmap implementation.
I2C and SPI regmaps are able to split transfers according to max_read_len.
But for I3C, it is left to the controller driver, which usually only returns an error.

>
>> +		/* Read full fifo is not supported, read samples one by one. */
>> +		ret = 0;
>> +		for (i = 0; i < st->fifo.count && ret == 0; i += packet_size)
>> +			ret = regmap_noinc_read(st->map, INV_ICM45600_REG_FIFO_DATA,
>> +						&st->fifo.data[i], packet_size);
>> +	}
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < st->fifo.count; i += size) {
>> +		size = inv_icm45600_fifo_decode_packet(&st->fifo.data[i], &accel, &gyro,
>> +						       &temp, &timestamp, &odr);
>> +		if (size <= 0)
>
>Doesn't size < 0 is an error condition that should be returned?
size<0 means that we reached end of FIFO.
I'll add a comment to clarify this.

>
>> +			break;
>> +		if (gyro != NULL && inv_icm45600_fifo_is_data_valid(gyro))
>> +			st->fifo.nb.gyro++;
>> +		if (accel != NULL && inv_icm45600_fifo_is_data_valid(accel))
>> +			st->fifo.nb.accel++;
>
>Drop ' != NULL' parts.
Clear.

>
>> +		st->fifo.nb.total++;
>> +	}
>> +
>> +	return 0;
>> +}
>
>...
>
>> +int inv_icm45600_buffer_init(struct inv_icm45600_state *st)
>> +{
>> +	int ret;
>> +	unsigned int val;
>> +
>> +	st->fifo.watermark.eff_gyro = 1;
>> +	st->fifo.watermark.eff_accel = 1;
>> +
>> +	/* Disable all FIFO EN bits. */
>> +	ret = regmap_write(st->map, INV_ICM45600_REG_FIFO_CONFIG3, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Disable FIFO and set depth. */
>> +	val = FIELD_PREP(INV_ICM45600_FIFO_CONFIG0_MODE_MASK,
>> +			 INV_ICM45600_FIFO_CONFIG0_MODE_BYPASS);
>> +	val |= INV_ICM45600_FIFO_CONFIG0_FIFO_DEPTH_MAX;
>
>FIELD_MODIFY()
Ok, great.

>
>> +	ret = regmap_write(st->map, INV_ICM45600_REG_FIFO_CONFIG0, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Enable only timestamp in fifo, disable compression. */
>> +	ret = regmap_write(st->map, INV_ICM45600_REG_FIFO_CONFIG4,
>> +			   INV_ICM45600_FIFO_CONFIG4_TMST_FSYNC_EN);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Enable FIFO continuous watermark interrupt. */
>> +	return regmap_set_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG2,
>> +			       INV_ICM45600_REG_FIFO_CONFIG2_WM_GT_TH);
>> +}
>
>...
>
>> +#ifndef INV_ICM45600_BUFFER_H_
>> +#define INV_ICM45600_BUFFER_H_
>> +
>> +#include <linux/bits.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/types.h>
>
>IWYU, please.
Yes again.
>
>...
>
>> +/* FIFO data packet */
>> +struct inv_icm45600_fifo_sensor_data {
>> +	__le16 x;
>> +	__le16 y;
>> +	__le16 z;
>> +} __packed;
>
>Why __packed? Do you expect this to be on unaligned addresses?
Agreed, it is useless in that case.

>
>...
>
>> +#define INV_ICM45600_DATA_INVALID		S16_MIN
>
>limits.h?
Yes.
>
>...
>
>> +static inline s16 inv_icm45600_fifo_get_sensor_data(__le16 d)
>> +{
>> +	return le16_to_cpu(d);
>
>asm/byteorder.h ?
Yes.
Is linux/byteorder/generic.h OK?

>
>> +}
>
>...
>
>> +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);
>> +
>> +	if (x == INV_ICM45600_DATA_INVALID &&
>> +	    y == INV_ICM45600_DATA_INVALID &&
>> +	    z == INV_ICM45600_DATA_INVALID)
>> +		return false;
>> +
>> +	return true;
>
>'if' can be avoided. But it's up to you. All depends on the readability of the
>end result.
It definitely makes sense. 
>
>> +}
>
>...
>
>> +/**
>> + * inv_icm45600_irq_init() - initialize int pin and interrupt handler
>> + * @st:		driver internal state
>> + * @irq:	irq number
>> + * @irq_type:	irq trigger type
>> + * @open_drain:	true if irq is open drain, false for push-pull
>> + *
>> + * Returns 0 on success, a negative error code otherwise.
>
>kernel-doc validation...
Yes again.
>
>> + */
>
>...
>
>> +	st->fifo.data = devm_kzalloc(dev, 8192, GFP_KERNEL);
>> +	if (!st->fifo.data)
>> +		return dev_err_probe(dev, -ENOMEM, "Cannot allocate fifo memory\n");
>
>HAve you checked what will happen in this case? Please, take a look and update
>the code accordingly.
Agreed this call is useless, I'll simplify the return code.

>
>...
>
>> -	scoped_guard(mutex, &st->lock)
>> +	scoped_guard(mutex, &st->lock) {
>
>Ah, nice. It should have been done in the first place and put a comment to that
>patch that scoped_guard() {} used specifically for limiting churn in the next
>changes.
If ok for you, I'll keep that as it is.
If I add a comment in previous patch, I'll anyway have to delete it this patch.

>
>>  		/* Restore sensors state. */
>>  		ret = inv_icm45600_set_pwr_mgmt0(st, st->suspended.gyro,
>
>> -						st->suspended.accel, NULL);
>> +						 st->suspended.accel, NULL);
>
>Stray change.
I'll fix in previous patch.

>
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* Restore FIFO data streaming. */
>> +		if (st->fifo.on) {
>> +			struct inv_icm45600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
>> +			struct inv_icm45600_sensor_state *accel_st = iio_priv(st->indio_accel);
>> +			unsigned int val;
>> +
>> +			inv_sensors_timestamp_reset(&gyro_st->ts);
>> +			inv_sensors_timestamp_reset(&accel_st->ts);
>> +			val = FIELD_PREP(INV_ICM45600_FIFO_CONFIG0_MODE_MASK,
>> +					 INV_ICM45600_FIFO_CONFIG0_MODE_STREAM);
>> +			ret = regmap_update_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG0,
>> +						 INV_ICM45600_FIFO_CONFIG0_MODE_MASK, val);
>> +			if (ret)
>> +				return ret;
>> +			/* FIFO_CONFIG3_IF_EN must only be set at end of FIFO the configuration */
>> +			ret = regmap_set_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG3,
>> +					      INV_ICM45600_FIFO_CONFIG3_IF_EN);
>
>This relies on the code elsewhere, much better to return here if an error
>condition happened.
Sure.
>
>> +		}
>> +	}
>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>
>

Powered by blists - more mailing lists