Message-ID:
 <FR2PPF4571F02BCABEB6633903C3F49766C8C30A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Tue, 19 Aug 2025 12:58:22 +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 v4 3/9] iio: imu: inv_icm45600: add buffer support in iio
 devices

>
>
>From: Jonathan Cameron mailto:jic23@...nel.org 
>Sent: Saturday, August 16, 2025 2:00 PM
>To: Remi Buisson via B4 Relay mailto:devnull+remi.buisson.tdk.com@...nel.org
>Cc: Remi Buisson mailto:Remi.Buisson@....com; David Lechner mailto:dlechner@...libre.com; Nuno Sá mailto:nuno.sa@...log.com; Andy Shevchenko mailto:andy@...nel.org; Rob Herring mailto:robh@...nel.org; Krzysztof Kozlowski mailto:krzk+dt@...nel.org; Conor Dooley mailto:conor+dt@...nel.org; mailto:linux-kernel@...r.kernel.org; mailto:linux-iio@...r.kernel.org; mailto:devicetree@...r.kernel.org
>Subject: Re: [PATCH v4 3/9] iio: imu: inv_icm45600: add buffer support in iio devices
>
>On Thu, 14 Aug 2025 08:57:17 +0000
>Remi Buisson via B4 Relay mailto:devnull+remi.buisson.tdk.com@...nel.org wrote:
>
>> From: Remi Buisson mailto:remi.buisson@....com
>> 
>> Add FIFO control functions.
>> Support hwfifo watermark by multiplexing gyro and accel settings.
>> Support hwfifo flush.
>> 
>> Signed-off-by: Remi Buisson mailto:remi.buisson@....com
>
>Comments inline.
>
>Thanks,
>
>> 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..59415e9b1e4ee21a641781275e3654402cf6d0a8
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c
>
>> +/**
>> + * inv_icm45600_buffer_update_watermark - update watermark FIFO threshold
>> + * @st:	driver internal state
>> + *
>> + * Returns 0 on success, a negative error code otherwise.
>> + *
>> + * 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;
>> +	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 = latency / period;
>> +		if (watermark < 1)
>> +			watermark = 1;
>
>		watermark = max(latency/period, 1);
Okay.

>
>> +		/* Update effective watermark. */
>> +		st->fifo.watermark.eff_gyro = latency / period_gyro;
>> +		if (st->fifo.watermark.eff_gyro < 1)
>> +			st->fifo.watermark.eff_gyro = 1;
>
>		st->fifo.atermark.eff_gyro = max(latency / period_gyro, 1);
>
Yes.
>> +		st->fifo.watermark.eff_accel = latency / period_accel;
>> +		if (st->fifo.watermark.eff_accel < 1)
>> +			st->fifo.watermark.eff_accel = 1;
>		max()
Yes.
>> +	}
>> +
>> +
>> +	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));
>> +}
>> +
>> +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);
>
>Align immediately after (
Fine.
>
>
>> +	val |= INV_ICM45600_FIFO_CONFIG0_FIFO_DEPTH_MAX;
>> +	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);
>> +}
>
>
>> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c b/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c
>> index 0fdf86cdfe547357d2b74d9c97092e9a1e5722a8..cad632fb2e5158e9cd7cee66f77eb56fe101ecc3 100644
>> --- a/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c
>> @@ -17,6 +17,7 @@
>
>
>> +/**
>> + * 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.
>> + */
>> +static int inv_icm45600_irq_init(struct inv_icm45600_state *st, int irq,
>> +				 int irq_type, bool open_drain)
>> +{
>> +	struct device *dev = regmap_get_device(st->map);
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	/* Configure INT1 interrupt: default is active low on edge. */
>> +	switch (irq_type) {
>> +	case IRQF_TRIGGER_RISING:
>> +	case IRQF_TRIGGER_HIGH:
>> +		val = INV_ICM45600_INT1_CONFIG2_ACTIVE_HIGH;
>> +		break;
>> +	default:
>> +		val = INV_ICM45600_INT1_CONFIG2_ACTIVE_LOW;
>> +		break;
>> +	}
>> +
>> +	switch (irq_type) {
>> +	case IRQF_TRIGGER_LOW:
>> +	case IRQF_TRIGGER_HIGH:
>> +		val |= INV_ICM45600_INT1_CONFIG2_LATCHED;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	if (!open_drain)
>> +		val |= INV_ICM45600_INT1_CONFIG2_PUSH_PULL;
>> +
>> +	ret = regmap_write(st->map, INV_ICM45600_REG_INT1_CONFIG2, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	irq_type |= IRQF_ONESHOT;
>
>I'd do that in the call and avoid changing the meaning of the irq_type
>local variable in this function.
>
>> +	return devm_request_threaded_irq(dev, irq, inv_icm45600_irq_timestamp,
>> +					 inv_icm45600_irq_handler, irq_type,
>
>					 irq_type | IRQF_ONESHOT,
>
Thanks, clear.
>> +					 "inv_icm45600", st);
>> +}
>> +
>>  static int inv_icm45600_timestamp_setup(struct inv_icm45600_state *st)
>>  {
>>  	/* Enable timestamps. */
>> @@ -556,6 +646,8 @@ int inv_icm45600_core_probe(struct regmap *regmap, const struct inv_icm45600_chi
>>  	struct fwnode_handle *fwnode;
>>  	struct inv_icm45600_state *st;
>>  	struct regmap *regmap_custom;
>> +	int irq, irq_type;
>> +	bool open_drain;
>>  	int ret;
>>  
>>  	/* Get INT1 only supported interrupt. */
>> @@ -563,6 +655,17 @@ int inv_icm45600_core_probe(struct regmap *regmap, const struct inv_icm45600_chi
>>  	if (!fwnode)
>>  		return dev_err_probe(dev, -ENODEV, "Missing FW node\n");
>>  
>> +	irq = fwnode_irq_get_byname(fwnode, "INT1");
>> +	if (irq < 0) {
>> +		if (irq != -EPROBE_DEFER)
>> +			dev_err_probe(dev, irq, "Missing INT1 interrupt\n");
>dev_err_probe() doesn't print on defer anyway. (or -ENOMEM)
>
>What you've done here is stopped the debug logic for deferred probe reasons getting
>the info on it being because we are waiting for an interrupt.  If this was intentional
>then add a comment, otherwise just
>		return dev_err_probe()
>here
>
Ok thanks for the clarification.
>> +		return irq;
>> +	}
>
>> @@ -633,6 +748,23 @@ static int inv_icm45600_suspend(struct device *dev)
>>  
>>  		st->suspended.gyro = st->conf.gyro.mode;
>>  		st->suspended.accel = st->conf.accel.mode;
>> +
>> +		/* Disable FIFO data streaming. */
>> +		if (st->fifo.on) {
>> +			unsigned int val;
>> +
>> +			ret = regmap_clear_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG3,
>> +						INV_ICM45600_FIFO_CONFIG3_IF_EN);
>> +			if (ret)
>> +				return ret;
>> +			val = FIELD_PREP(INV_ICM45600_FIFO_CONFIG0_MODE_MASK,
>> +						INV_ICM45600_FIFO_CONFIG0_MODE_BYPASS);
>> +			ret = regmap_update_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG0,
>> +						INV_ICM45600_FIFO_CONFIG0_MODE_MASK, val);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>>  	}
>>  
>>  	return pm_runtime_force_suspend(dev);
>> @@ -653,10 +785,30 @@ static int inv_icm45600_resume(struct device *dev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	scoped_guard(mutex, &st->lock)
>> +	scoped_guard(mutex, &st->lock) {
>> +
>>  		/* Restore sensors state. */
>>  		ret = inv_icm45600_set_pwr_mgmt0(st, st->suspended.gyro,
>>  						st->suspended.accel, NULL);
>> +		if (ret)
>> +			return ret;
>> +
>> +		/* Restore FIFO data streaming. */
>Its a little odd to resume in a different order to how we took things down.
>Perhaps add a comment if there is a reason for that.  If not reorder it
>purely to make it easier to review.
I'll re-order the accel/gyro mode save during suspend to match the resume order.
And add a comment to the clear/set of INV_ICM45600_FIFO_CONFIG3_IF_EN.
>
>
>> +		if (st->fifo.on) {
>> +			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;
>> +			ret = regmap_set_bits(st->map, INV_ICM45600_REG_FIFO_CONFIG3,
>> +					INV_ICM45600_FIFO_CONFIG3_IF_EN);
>> +		}
>> +	}
>>  
>>  	return ret;
>>  }
>> 
>
>

Powered by blists - more mailing lists