Re: [PATCH v5 2/9] iio: imu: inv_icm45600: add new inv_icm45600 driver

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Aug 20, 2025 at 02:24:20PM +0000, Remi Buisson via B4 Relay wrote:
> 
> 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.

...

> +#ifndef INV_ICM45600_H_
> +#define INV_ICM45600_H_
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/iio/common/inv_sensors_timestamp.h>
> +#include <linux/iio/iio.h>
> +#include <linux/types.h>

Please, follow IWYU principle. Also, it's better to split out the IIO group as
it's part of the subsystem this driver is for.

#include <linux/bitfield.h>
#include <linux/bits.h>
#include <linux/types.h>

#include <linux/iio/common/inv_sensors_timestamp.h>
#include <linux/iio/iio.h>

(but again, the list of the headers seems incorrect / incomplete).

...

> +struct inv_icm45600_state {
> +	struct mutex lock;

No header for this.

> +	struct regmap *map;

No forward declaration.

> +	struct regulator *vddio_supply;

Ditto.

> +	struct iio_mount_matrix orientation;



> +	struct iio_dev *indio_gyro;
> +	struct iio_dev *indio_accel;
> +	const struct inv_icm45600_chip_info *chip_info;
> +	struct {
> +		s64 gyro;
> +		s64 accel;
> +	} timestamp;
> +	union {
> +		u8 buff[2];
> +		__le16 u16;
> +		u8 ireg[3];
> +	} buffer __aligned(IIO_DMA_MINALIGN);
> +};

...

> +#define INV_ICM45600_FIFO_SIZE_MAX			(8 * 1024)

SZ_8K from sizes.h ?

...

> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#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>

As per above, please double check for IWYU principle.

...

> +static int inv_icm45600_ireg_read(struct regmap *map, unsigned int reg,
> +				   u8 *data, size_t count)
> +{
> +	const struct device *dev = regmap_get_device(map);
> +	struct inv_icm45600_state *st = dev_get_drvdata(dev);
> +	unsigned int d;

> +	ssize_t i;

Why signed? Same comment for all similar cases.

> +	int ret;
> +
> +	st->buffer.ireg[0] = FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg);
> +	st->buffer.ireg[1] = FIELD_GET(INV_ICM45600_REG_ADDR_MASK, reg);
> +
> +	/* Burst write address. */
> +	ret = regmap_bulk_write(map, INV_ICM45600_REG_IREG_ADDR, st->buffer.ireg, 2);
> +	/* Wait while the device is busy processing the address. */
> +	fsleep(INV_ICM45600_IREG_DELAY_US);
> +	if (ret)
> +		return ret;
> +
> +	/* Read the data. */
> +	for (i = 0; i < count; i++) {
> +		ret = regmap_read(map, INV_ICM45600_REG_IREG_DATA, &d);
> +		/* Wait while the device is busy processing the data. */
> +		fsleep(INV_ICM45600_IREG_DELAY_US);
> +		if (ret)
> +			return ret;
> +		data[i] = d;
> +	}
> +
> +	return 0;
> +}

...

> +	if (FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg) == 0)

Why not using positive conditional?

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

	if (FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg))
		return inv_icm45600_ireg_read(map, reg, val_buf, val_size);

Ditto for other similar cases.

...

> +static int inv_icm45600_write(void *context, const void *data,
> +				   size_t count)

This is perfectly 1 line, please, check that the code utilises exactly 80 limit
when there is a room. It's probably a wrapping done by the (mis)configured editor.

...

> +static const struct regmap_config inv_icm45600_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,

No cache?

> +};

...

> +static const struct inv_icm45600_conf inv_icm45600_default_conf = {
> +	.gyro = {
> +		.mode = INV_ICM45600_SENSOR_MODE_OFF,
> +		.fs = INV_ICM45686_GYRO_FS_2000DPS,
> +		.odr = INV_ICM45600_ODR_800HZ_LN,
> +		.filter = INV_ICM45600_GYRO_LP_AVG_SEL_8X,
> +	},
> +	.accel = {
> +		.mode = INV_ICM45600_SENSOR_MODE_OFF,
> +		.fs = INV_ICM45686_ACCEL_FS_16G,
> +		.odr = INV_ICM45600_ODR_800HZ_LN,
> +		.filter = INV_ICM45600_ACCEL_LP_AVG_SEL_4X,
> +	},
> +};

Can you split the patch adding accel or gyro separately? I haven't checked all
the details, so it might be not worth it, just consider it.

...

> +u32 inv_icm45600_odr_to_period(enum inv_icm45600_odr odr)
> +{
> +	static const u32 odr_periods[INV_ICM45600_ODR_MAX] = {
> +		0, 0, 0,	/* reserved values */

Make it one per line as the rest.

> +		156250,		/* 6.4kHz */
> +		312500,		/* 3.2kHz */
> +		625000,		/* 1.6kHz */
> +		1250000,	/* 800kHz */
> +		2500000,	/* 400Hz */
> +		5000000,	/* 200Hz */
> +		10000000,	/* 100Hz */
> +		20000000,	/* 50Hz */
> +		40000000,	/* 25Hz */
> +		80000000,	/* 12.5Hz */
> +		160000000,	/* 6.25Hz */
> +		320000000,	/* 3.125Hz */
> +		640000000,	/* 1.5625Hz */

These seem to be times or so, can you use proper naming instead of _periods?

> +	};
> +
> +	return odr_periods[odr];
> +}

...

> +int inv_icm45600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> +			     unsigned int writeval, unsigned int *readval)
> +{
> +	struct inv_icm45600_state *st = iio_device_get_drvdata(indio_dev);

> +	int ret;

Useless, just return directly.

> +	guard(mutex)(&st->lock);

> +	if (readval)
> +		ret = regmap_read(st->map, reg, readval);
> +	else
> +		ret = regmap_write(st->map, reg, writeval);
> +
> +	return ret;
> +}

...

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

Please, run kernel-doc validator. It's not happy (Return section is missing)

> + */

...

> +	if (val != chip_info->whoami) {
> +		if (val == U8_MAX || val == 0)

Hmm... Perhaps in_range() ?

> +			return dev_err_probe(dev, -ENODEV,
> +					     "Invalid whoami %#02x expected %#02x (%s)\n",
> +					     val, chip_info->whoami, chip_info->name);

> +		else

Redundant 'else'.

> +			dev_warn(dev, "Unexpected whoami %#02x expected %#02x (%s)\n",
> +				 val, chip_info->whoami, chip_info->name);
> +	}

...

> +		ret = regmap_write(st->map, INV_ICM45600_REG_MISC2,
> +				   INV_ICM45600_MISC2_SOFT_RESET);
> +		if (ret)
> +			return ret;
> +		/* IMU reset time: 1ms. */
> +		fsleep(1000);

Use 1 * USEC_PER_MSEC and drop useless comment after that.
You will need time.h for it.

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

...

> +static int inv_icm45600_enable_regulator_vddio(struct inv_icm45600_state *st)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(st->vddio_supply);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait a little for supply ramp. */
> +	fsleep(3000);

As per above.

> +	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;
> +
> +	regmap_custom = devm_regmap_init(dev, &inv_icm45600_regmap_bus,
> +					 regmap, &inv_icm45600_regmap_config);

There is room for 'regmap' on the previous line.

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

100 * USEC_PER_MSEC

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

2 * MSEC_PER_SEC and drop yet another useless comment.

> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put(dev);
> +
> +	return 0;
> +}

...

> +static int inv_icm45600_resume(struct device *dev)
> +{
> +	struct inv_icm45600_state *st = dev_get_drvdata(dev);
> +	int ret = 0;

Why assignment?

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

With guard()() this whole construction will look better.

> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko