Message-ID: <20250713170017.3e4ad51c@jic23-huawei>
Date: Sun, 13 Jul 2025 17:00:17 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@...nel.org>
Cc: 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, Sean Nyekjaer
 <sean@...nix.com>
Subject: Re: [PATCH v2 2/8] iio: imu: inv_icm45600: add new inv_icm45600
 driver

On Thu, 10 Jul 2025 08:57:57 +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,

A few minor things inline.  Given Sean has been looking at a closely
related driver and you have some of the things he's been fixing up
I +CC Sean on off chance he has time to take a look at this.

> ---
>  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 | 734 +++++++++++++++++++++++
>  6 files changed, 1109 insertions(+)
>
> 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..d56de75ab7f2168f22e25b5816cb361bef457c0d
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
> @@ -0,0 +1,364 @@

> +struct inv_icm45600_sensor_conf {
> +	int mode;
> +	int fs;
> +	int odr;
> +	int filter;
> +};
> +#define INV_ICM45600_SENSOR_CONF_INIT		{-1, -1, -1, -1}

+#define INV_ICM45600_SENSOR_CONF_INIT		{ -1, -1, -1, -1, }

Though I'm not sure the define is that useful vs pushing this down into the code.


> 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..022eb9180b5a750e8fba4a1cd873041c27f59880
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c

> +static int inv_icm45600_read(void *context, const void *reg_buf, size_t reg_size,
> +			  void *val_buf, size_t val_size)
> +{
> +	unsigned int reg = (unsigned int) be16_to_cpup(reg_buf);

Why is the cast needed? It's a u16 so why not keep it like that?

> +	struct regmap *map = context;
> +
> +	if (FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg) == 0)
> +		return regmap_bulk_read(map, FIELD_GET(INV_ICM45600_REG_ADDR_MASK, reg),
> +						val_buf, val_size);
> +
> +	return inv_icm45600_ireg_read(map, reg, val_buf, val_size);
> +}
> +
> +static int inv_icm45600_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	u8 *d = (u8 *)data;
	const u8 *d = data;
should be fine.

> +	unsigned int reg = (unsigned int) be16_to_cpup(data);

u16

> +	struct regmap *map = context;
> +
> +	if (FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg) == 0)
> +		return regmap_bulk_write(map, FIELD_GET(INV_ICM45600_REG_ADDR_MASK, reg),
> +					d + 2, count - 2);
> +
> +	return inv_icm45600_ireg_write(map, reg, d + 2, count - 2);
> +}

> + *  inv_icm45600_setup() - check and setup chip
> + *  @st:	driver internal state
> + *  @chip_info:	detected chip description
> + *  @reset:	define whether a reset is required or not
> + *  @bus_setup:	callback for setting up bus specific registers
> + *
> + *  Returns 0 on success, a negative error code otherwise.
> + */
> +static int inv_icm45600_setup(struct inv_icm45600_state *st,
> +				const struct inv_icm45600_chip_info *chip_info,
> +				bool reset, inv_icm45600_bus_setup bus_setup)
> +{
> +	const struct device *dev = regmap_get_device(st->map);
> +	unsigned int val;
> +	int ret;
> +
> +	/* Set chip bus configuration if specified. */
> +	if (bus_setup) {
> +		ret = bus_setup(st);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Check chip self-identification value. */
> +	ret = regmap_read(st->map, INV_ICM45600_REG_WHOAMI, &val);
> +	if (ret)
> +		return ret;
> +	if (val != chip_info->whoami) {
> +		dev_err(dev, "invalid whoami %#02x expected %#02x (%s)\n",

This should be relaxed to a dev_info and carry on anyway. The reasoning
for this is fallback compatibles in DT.  If a new compatible chip
with a different whoami value comes along we still want it to run
with older kernels so we use fallback compatibles.  Unfortunately the
ID won't match.

> +			val, chip_info->whoami, chip_info->name);
> +		return -ENODEV;
> +	}
> +
> +	st->chip_info = chip_info;
> +
> +	if (reset) {
> +		/* Reset to make sure previous state are not there. */
> +		ret = regmap_write(st->map, INV_ICM45600_REG_MISC2,
> +				INV_ICM45600_MISC2_SOFT_RESET);
> +		if (ret)
> +			return ret;
> +		/* IMU reset time: 1ms. */
> +		fsleep(1000);
> +
> +		if (bus_setup) {
> +			ret = bus_setup(st);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = regmap_read(st->map, INV_ICM45600_REG_INT_STATUS, &val);
> +		if (ret)
> +			return ret;
> +		if (!(val & INV_ICM45600_INT_STATUS_RESET_DONE)) {
> +			dev_err(dev, "reset error, reset done bit not set\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	return inv_icm45600_set_conf(st, chip_info->conf);
> +}

> +
> +static void inv_icm45600_disable_vddio_reg(void *_data)
> +{
> +	struct inv_icm45600_state *st = _data;
> +	const struct device *dev = regmap_get_device(st->map);
> +	int ret;
> +
> +	ret = regulator_disable(st->vddio_supply);
> +	if (ret)
> +		dev_err(dev, "failed to disable vddio error %d\n", ret);

As per Sean's set - is this useful given such failures are already noisy.


> +}
> +
> +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 irq, irq_type;
> +	bool open_drain;
> +	int ret;
> +
> +	/* Get INT1 only supported interrupt. */
> +	fwnode = dev_fwnode(dev);
> +	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");
> +		return irq;
> +	}
> +
> +	irq_type = irq_get_trigger_type(irq);
> +
> +	open_drain = device_property_read_bool(dev, "drive-open-drain");
> +
> +	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 ret;
> +	/* IMU start-up time. */
> +	msleep(100);
> +
> +	ret = inv_icm45600_enable_regulator_vddio(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, inv_icm45600_disable_vddio_reg, st);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup chip registers. */
> +	ret = inv_icm45600_setup(st, chip_info, reset, bus_setup);
> +	if (ret)
> +		return ret;
> +
> +	ret = inv_icm45600_timestamp_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	/* Setup runtime power management. */
> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_get_noresume(dev);
> +	/* Suspend after 2 seconds. */
> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put(dev);

See below. Sean also reworks this. + I suggested he look at
devm_pm_runtime_enable but the set active version seems even better.
 
I'm not setting the gets and puts I'd expect for working runtime PM though.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(inv_icm45600_core_probe, "IIO_ICM45600");

> +
> +/*
> + * 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);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = inv_icm45600_enable_regulator_vddio(st);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
Take a look at 
https://lore.kernel.org/all/20250709-icm42pmreg-v1-0-3d0e793c99b2@geanix.com/

It would be good if you an review Sean's series as well as applying anything
you agree with here.  For example he got rid of the 3 lines above.

> +
> +	/* Restore sensors state. */
> +	return inv_icm45600_set_pwr_mgmt0(st, st->suspended.gyro,
> +					 st->suspended.accel, NULL);
> +
> +}
> +
> +/* Runtime suspend will turn off sensors that are enabled by iio devices. */
> +static int inv_icm45600_runtime_suspend(struct device *dev)
> +{
> +	struct inv_icm45600_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* disable all sensors */
> +	ret = inv_icm45600_set_pwr_mgmt0(st, INV_ICM45600_SENSOR_MODE_OFF,
> +					 INV_ICM45600_SENSOR_MODE_OFF, NULL);
> +	if (ret)
> +		return ret;
> +
> +	regulator_disable(st->vddio_supply);
> +
> +	return 0;
> +}
> +
> +/* Sensors are enabled by iio devices, no need to turn them back on here. */
> +static int inv_icm45600_runtime_resume(struct device *dev)
> +{
> +	struct inv_icm45600_state *st = dev_get_drvdata(dev);
> +
> +	guard(mutex)(&st->lock);
> +
> +	return inv_icm45600_enable_regulator_vddio(st);
> +}
> +
> +EXPORT_NS_GPL_DEV_PM_OPS(inv_icm45600_pm_ops, IIO_ICM45600) = {
> +	SET_SYSTEM_SLEEP_PM_OPS(inv_icm45600_suspend, inv_icm45600_resume)
> +	SET_RUNTIME_PM_OPS(inv_icm45600_runtime_suspend,
> +			   inv_icm45600_runtime_resume, NULL)
> +};

Powered by blists - more mailing lists