Message-ID:
 <FR2PPF4571F02BCC073F7740CBA818676388C00A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Thu, 4 Sep 2025 12:58:10 +0000
From: Remi Buisson <Remi.Buisson@....com>
To: Andy Shevchenko <andriy.shevchenko@...el.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-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 v5 2/9] iio: imu: inv_icm45600: add new inv_icm45600
 driver

>
>
>From: Andy Shevchenko <andriy.shevchenko@...el.com> 
>Sent: Thursday, August 21, 2025 11:03 AM
>To: Remi Buisson <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 2/9] iio: imu: inv_icm45600: add new inv_icm45600 driver
>
>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.
>
Hello Andy,

Thanks for this review.

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

Sure, I already did a header review once, but it looks like it still need a fix.

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

Correct
>
>> +	struct regmap *map;
>
>No forward declaration.

Correct again
>
>> +	struct regulator *vddio_supply;
>
>Ditto.

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

Definitely better this way.

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

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

My mistake.
>
>> +	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?

Sure, it will be easier to read.
>
>> +		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.

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

I probably did that one by hand... I'll fix.
>
>...
>
>> +static const struct regmap_config inv_icm45600_regmap_config = {
>> +	.reg_bits = 16,
>> +	.val_bits = 8,
>
>No cache?
>
If OK for you, we prefer to push this patch without cache.
And introduce it in another patchset.

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

Accel and Gyro code is added later in the patchset.
I move these definitions to the corresponding patchs.

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

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

It actually corresponds to periods = 1/frequency.
It's time for sure, but using "period" is a bit more specific IMO.

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

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

kernel-doc does not complain on this, on my side. 
I ran kernel-doc.py -v -none drivers/iio/imu/inv_icm45600/*
Is there any option I'm missing.
Anyway, I will add the missing colon and check the result.

>
>> + */
>
>...
>
>> +	if (val != chip_info->whoami) {
>> +		if (val == U8_MAX || val == 0)
>
>Hmm... Perhaps in_range() ?

Not sure of the benefit of this change.
I prefer to keep it this way if OK for you.

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

Right, I will change that one.
>
>> +			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.

Thanks for the tip, clear improvement.
>
>> +
>> +		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.
Yes.

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

Sure, I'll reformat.
>
>> +	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
Yes.
>
>> +	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.
Sure.
>
>> +	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?

No reason, I'll remove it.
>
>> +	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.

It's coming in later patch.
I thought it would better follow coding guidelines this way.
But let me know if it is not the case.

>
>> +	return ret;
>> +}
>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>
>

Powered by blists - more mailing lists