Message-ID: <aKbk9WYtfb5L5la4@smile.fi.intel.com>
Date: Thu, 21 Aug 2025 12:20:53 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: 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.

...

> +#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.

...

> +{
> +	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?

> +	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.

> +	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()?

> +}

...

> +/**
> + * 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.

> + * 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.

> +	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.

> +	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.

...

> +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

> +	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;

> +	/* 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?

> +		/* 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?

> +			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.

> +		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()

> +	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.

...

> +/* 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?

...

> +#define INV_ICM45600_DATA_INVALID		S16_MIN

limits.h?

...

> +static inline s16 inv_icm45600_fifo_get_sensor_data(__le16 d)
> +{
> +	return le16_to_cpu(d);

asm/byteorder.h ?

> +}

...

> +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.

> +}

...

> +/**
> + * 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...

> + */

...

> +	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.

...

> -	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.

>  		/* Restore sensors state. */
>  		ret = inv_icm45600_set_pwr_mgmt0(st, st->suspended.gyro,

> -						st->suspended.accel, NULL);
> +						 st->suspended.accel, NULL);

Stray change.

> +		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.

> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists