Message-ID:
 <FR2PPF4571F02BC5DC4FDF00364C1A306EC8C30A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Tue, 19 Aug 2025 12:56:39 +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 2/9] iio: imu: inv_icm45600: add new inv_icm45600
 driver

>
>
>From: Jonathan Cameron <jic23@...nel.org> 
>Sent: Saturday, August 16, 2025 1:47 PM
>To: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org>
>Cc: Remi Buisson <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 v4 2/9] iio: imu: inv_icm45600: add new inv_icm45600 driver
>
>On Thu, 14 Aug 2025 08:57:16 +0000
>Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org> wrote:
>
>> From: Remi Buisson <remi.buisson@....com>
>> 
>> Core component of a new driver for InvenSense ICM-45600 devices.
>> It includes registers definition, main probe/setup, and device
>> utility functions.
>> 
>> ICM-456xx devices are latest generation of 6-axis IMU,
>> gyroscope+accelerometer and temperature sensor. This device
>> includes a 8K FIFO, supports I2C/I3C/SPI, and provides
>> intelligent motion features like pedometer, tilt detection,
>> and tap detection.
>> 
>> Signed-off-by: Remi Buisson <remi.buisson@....com>
>Hi Remi,
>
>Coming together well.  A few additional comments inline from a fresh read.
>
>Jonathan
Thanks for your support on this !
Remi
>
>> ---
>>  drivers/iio/imu/Kconfig                          |   1 +
>>  drivers/iio/imu/Makefile                         |   1 +
>>  drivers/iio/imu/inv_icm45600/Kconfig             |   5 +
>>  drivers/iio/imu/inv_icm45600/Makefile            |   4 +
>>  drivers/iio/imu/inv_icm45600/inv_icm45600.h      | 364 ++++++++++++
>>  drivers/iio/imu/inv_icm45600/inv_icm45600_core.c | 702 +++++++++++++++++++++++
>>  6 files changed, 1077 insertions(+)
>
>
>> diff --git a/drivers/iio/imu/inv_icm45600/Makefile b/drivers/iio/imu/inv_icm45600/Makefile
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..4f442b61896e91647c7947a044949792bae06a30
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/Makefile
>> @@ -0,0 +1,4 @@
>> +# SPDX-License-Identifier: GPL-2.0-or-later
>> +
>> +obj-$(CONFIG_INV_ICM45600) += inv-icm45600.o
>> +inv-icm45600-y += inv_icm45600_core.o
>> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600.h b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..e0304f35d32a078d4b9c260b2c6c29601583a429
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
>> @@ -0,0 +1,364 @@
>
>> +struct iio_dev *inv_icm45600_gyro_init(struct inv_icm45600_state *st);
>> +
>> +int inv_icm45600_gyro_parse_fifo(struct iio_dev *indio_dev);
>> +
>> +struct iio_dev *inv_icm45600_accel_init(struct inv_icm45600_state *st);
>> +
>> +int inv_icm45600_accel_parse_fifo(struct iio_dev *indio_dev);
>
>Some of this stuff isn't defined yet. Move the definitions to the patch where they are.
Sure!
>
>> +
>> +#endif
>> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c b/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..0fdf86cdfe547357d2b74d9c97092e9a1e5722a8
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c
>> @@ -0,0 +1,702 @@
>> +// 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/iio/iio.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>
>Bring headers in as they are used during the patch set.
>Not seeing an irqs yet.
Sure!
>
>> +#include <linux/limits.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/types.h>
>> +
>> +#include "inv_icm45600.h"
>> +
>> +static int inv_icm45600_ireg_read(struct regmap *map, unsigned int reg,
>> +				   u8 *data, size_t count)
>> +{
>> +	int ret;
>> +	u8 addr[2];
>> +	ssize_t i;
>> +	unsigned int d;
>> +
>> +	addr[0] = FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg);
>> +	addr[1] = FIELD_GET(INV_ICM45600_REG_ADDR_MASK, reg);
>> +
>> +	/* Burst write address. */
>> +	ret = regmap_bulk_write(map, INV_ICM45600_REG_IREG_ADDR, addr, sizeof(addr));
>
>addr is on the stack, so not DMA safe for the bulk write.  I haven't checked
>though if you have it bounced in the regmap implementation.
I agree it's safer to make sure from the driver that the memory is DMA-capable for bulk access.
As a sidenote:
The implementation of regmap_bulk_write in drivers/base/regmap/regmap.c
uses a kmemdup_array (I assume it is to avoid reading data from an unsuitable memory like stack)
Unless I missed it, I don't see regmap_bulk_read taking same precaution.
>
>> +	/* Wait while the device is busy processing the address. */
>> +	fsleep(INV_ICM45600_IREG_DELAY_US);
>> +	if (ret)
>> +		return ret;
>
>
>
>> +u32 inv_icm45600_odr_to_period(enum inv_icm45600_odr odr)
>> +{
>> +	static u32 odr_periods[INV_ICM45600_ODR_MAX] = {
>> +		/* reserved values */
>> +		0, 0, 0,
>> +		/* 6.4kHz */
>
>Given very short comments, perhaps
>		0, 0, 0, /* reserved */
>		156250, /* 6.4kHz */
>etc, just to reduce screen scrolling.
>
>Also, might as well mark it const.
Agreed on both comments.
>
>
>> +		156250,
>> +		/* 3.2kHz */
>> +		312500,
>> +		/* 1.6kHz */
>> +		625000,
>> +		/* 800kHz */
>> +		1250000,
>> +		/* 400Hz */
>> +		2500000,
>> +		/* 200Hz */
>> +		5000000,
>> +		/* 100Hz */
>> +		10000000,
>> +		/* 50Hz */
>> +		20000000,
>> +		/* 25Hz */
>> +		40000000,
>> +		/* 12.5Hz */
>> +		80000000,
>> +		/* 6.25Hz */
>> +		160000000,
>> +		/* 3.125Hz */
>> +		320000000,
>> +		/* 1.5625Hz */
>> +		640000000,
>> +	};
>> +
>> +	return odr_periods[odr];
>> +}
>> +
>> +static int inv_icm45600_set_pwr_mgmt0(struct inv_icm45600_state *st,
>> +				      enum inv_icm45600_sensor_mode gyro,
>> +				      enum inv_icm45600_sensor_mode accel,
>> +				      unsigned int *sleep_ms)
>> +{
>> +	enum inv_icm45600_sensor_mode oldgyro = st->conf.gyro.mode;
>> +	enum inv_icm45600_sensor_mode oldaccel = st->conf.accel.mode;
>> +	unsigned int sleepval;
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	/* if nothing changed, exit */
>> +	if (gyro == oldgyro && accel == oldaccel)
>> +		return 0;
>> +
>> +	val = FIELD_PREP(INV_ICM45600_PWR_MGMT0_GYRO_MODE_MASK, gyro) |
>> +	      FIELD_PREP(INV_ICM45600_PWR_MGMT0_ACCEL_MODE_MASK, accel);
>> +	ret = regmap_write(st->map, INV_ICM45600_REG_PWR_MGMT0, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	st->conf.gyro.mode = gyro;
>> +	st->conf.accel.mode = accel;
>> +
>> +	/* Compute the required wait time for sensors to stabilize. */
>> +	sleepval = 0;
>> +
>> +	/* Accel startup time. */
>> +	if (accel != oldaccel && oldaccel == INV_ICM45600_SENSOR_MODE_OFF) {
>> +		if (sleepval < INV_ICM45600_ACCEL_STARTUP_TIME_MS)
>> +			sleepval = INV_ICM45600_ACCEL_STARTUP_TIME_MS;
>
>		sleepval = max(sleepval, INV_ICM...
>
>> +	}
>> +	if (gyro != oldgyro) {
>> +		/* Gyro startup time. */
>> +		if (oldgyro == INV_ICM45600_SENSOR_MODE_OFF) {
>> +			if (sleepval < INV_ICM45600_GYRO_STARTUP_TIME_MS)
>> +				sleepval = INV_ICM45600_GYRO_STARTUP_TIME_MS;
>similar
>> +		/* Gyro stop time. */
>> +		} else if (gyro == INV_ICM45600_SENSOR_MODE_OFF) {
>> +			if (sleepval < INV_ICM45600_GYRO_STOP_TIME_MS)
>> +				sleepval =  INV_ICM45600_GYRO_STOP_TIME_MS;
>and here as well.
>
Agreed all 3 above comments. 
>> +		}
>> +	}
>> +
>> +	/* Deferred sleep value if sleep pointer is provided or direct sleep */
>> +	if (sleep_ms)
>> +		*sleep_ms = sleepval;
>> +	else if (sleepval)
>> +		msleep(sleepval);
>> +
>> +	return 0;
>> +}
>
>
>> +int inv_icm45600_core_probe(struct regmap *regmap, const struct inv_icm45600_chip_info *chip_info,
>> +				bool reset, inv_icm45600_bus_setup bus_setup)
>> +{
>> +	struct device *dev = regmap_get_device(regmap);
>> +	struct fwnode_handle *fwnode;
>> +	struct inv_icm45600_state *st;
>> +	struct regmap *regmap_custom;
>> +	int ret;
>> +
>> +	/* Get INT1 only supported interrupt. */
>
>Not seeing relevance of comment to this code.
Sorry, wrong place, wrong patch.
I'll move it to relevant location.
>
>> +	fwnode = dev_fwnode(dev);
>> +	if (!fwnode)
>
>Why do you need to check this here, rather than just letting it fail later?
OK + this should come in next patch.
 
>
>> +		return dev_err_probe(dev, -ENODEV, "Missing FW node\n");
>> +
>> +	regmap_custom = devm_regmap_init(dev, &inv_icm45600_regmap_bus,
>> +					 regmap, &inv_icm45600_regmap_config);
>> +	if (IS_ERR(regmap_custom))
>> +		return dev_err_probe(dev, PTR_ERR(regmap_custom), "Failed to register regmap\n");
>> +
>> +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>> +	if (!st)
>> +		return dev_err_probe(dev, -ENOMEM, "Cannot allocate memory\n");
>> +
>> +	dev_set_drvdata(dev, st);
>> +
>> +	ret = devm_mutex_init(dev, &st->lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	st->map = regmap_custom;
>> +
>> +	ret = iio_read_mount_matrix(dev, &st->orientation);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to retrieve mounting matrix\n");
>> +
>> +	st->vddio_supply = devm_regulator_get(dev, "vddio");
>> +	if (IS_ERR(st->vddio_supply))
>> +		return PTR_ERR(st->vddio_supply);
>> +
>> +	ret = devm_regulator_get_enable(dev, "vdd");
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to get vdd regulator\n");
>> +
>> +	/* IMU start-up time. */
>> +	fsleep(100000);
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>
>You may need to enforce runtime PM being enabled / built or add some fallback code.
>I think right now vddio will never be turned on if we have runtime PM disabled.
>
>That then leads into annoyingly fiddly code to turn it off again as we have
>to verify it isn't already off due to runtime pm in the path that is only there
>for non runtime pm.
>
I think we get it: as we can't fully rely on PM I'll revert to previous implementation for VDDIO.
And I will align with Sean's patch checking the device state when disabling device,
to avoid underflow when in suspend.
Please comment if you think it's still not ok.
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = inv_icm45600_setup(st, chip_info, reset, bus_setup);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = inv_icm45600_timestamp_setup(st);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Suspend after 2 seconds. */
>> +	pm_runtime_set_autosuspend_delay(dev, 2000);
>> +	pm_runtime_use_autosuspend(dev);
>> +	pm_runtime_put(dev);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(inv_icm45600_core_probe, "IIO_ICM45600");
>> +
>> +/*
>> + * Suspend saves sensors state and turns everything off.
>> + * Check first if runtime suspend has not already done the job.
>No explicit 'check' any more.  force suspend is dealing with that
>for you, so probably drop this doc or maybe update it.
Ok
>> + */
>> +static int inv_icm45600_suspend(struct device *dev)
>> +{
>> +	struct inv_icm45600_state *st = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	scoped_guard(mutex, &st->lock) {
>> +
>drop this blank line.
Yes.
>
>> +		st->suspended.gyro = st->conf.gyro.mode;
>> +		st->suspended.accel = st->conf.accel.mode;
>> +	}
>> +
>> +	return pm_runtime_force_suspend(dev);
>> +}
>> +
>> +/*
>> + * System resume gets the system back on and restores the sensors state.
>> + * Manually put runtime power management in system active state.
>> + */
>> +static int inv_icm45600_resume(struct device *dev)
>> +{
>> +	struct inv_icm45600_state *st = dev_get_drvdata(dev);
>> +	struct inv_icm45600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
>> +	struct inv_icm45600_sensor_state *accel_st = iio_priv(st->indio_accel);
>
>Bring these in when they are needed.  Probably next patch but I haven't checked.
Sure.
>
>> +	int ret = 0;
>> +
>> +	ret = pm_runtime_force_resume(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	scoped_guard(mutex, &st->lock)
>> +		/* Restore sensors state. */
>> +		ret = inv_icm45600_set_pwr_mgmt0(st, st->suspended.gyro,
>> +						st->suspended.accel, NULL);
>> +
>> +	return ret;
>> +}
>
>

Powered by blists - more mailing lists