Message-ID:
 <FR2PPF4571F02BCB51FC0779715F4C8AD828C2AA@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Wed, 13 Aug 2025 10:06:20 +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 v3 2/8] iio: imu: inv_icm45600: add new inv_icm45600
 driver

>
>
>From: Jonathan Cameron <jic23@...nel.org> 
>Sent: Thursday, July 24, 2025 5:56 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 v3 2/8] iio: imu: inv_icm45600: add new inv_icm45600 driver
>
>On Thu, 17 Jul 2025 13:25:54 +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>
>
>A few minor comments inline.
>
>Thanks,
>
>Jonathan
>
>
>> 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..59aed59b94ca2d4709b0c986ddeda80b33064f90
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
>
>> +};
>
>> +
>> +/* all sensor data are 16 bits (2 registers wide) in big-endian */
>> +#define INV_ICM45600_REG_TEMP_DATA			0x000C
>> +#define INV_ICM45600_REG_ACCEL_DATA_X			0x0000
>> +#define INV_ICM45600_REG_ACCEL_DATA_Y			0x0002
>> +#define INV_ICM45600_REG_ACCEL_DATA_Z			0x0004
>> +#define INV_ICM45600_REG_GYRO_DATA_X			0x0006
>> +#define INV_ICM45600_REG_GYRO_DATA_Y			0x0008
>> +#define INV_ICM45600_REG_GYRO_DATA_Z			0x000A
>> +#define INV_ICM45600_DATA_INVALID			-32768
>That's all Fs.  GENMASK maybe?
It's supposed to be 0x8000
I will use hexadecimal instead, it would be clearer.
>
>
>
>> +int inv_icm45600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
>> +			     unsigned int writeval, unsigned int *readval);
>> +
>> +int inv_icm45600_core_probe(struct regmap *regmap, const struct inv_icm45600_chip_info *chip_info,
>Very long line.  Try to keep to 80 chars unless readability badly hurt (then it's fine to go a little
>above Tthat)
Sure.
>
>> +				bool reset, inv_icm45600_bus_setup bus_setup);
>> +
>> +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);
>> +
>> +#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..b961f774e54d0ad109b4ed19eec1cd9b65803c96
>> --- /dev/null
>> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_core.c
>
>> +
>> +int inv_icm45600_set_gyro_conf(struct inv_icm45600_state *st,
>> +			       struct inv_icm45600_sensor_conf *conf,
>> +			       unsigned int *sleep_ms)
>> +{
>> +	struct inv_icm45600_sensor_conf *oldconf = &st->conf.gyro;
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	/* Sanitize missing values with current values. */
>> +	if (conf->mode == U8_MAX)
>> +		conf->mode = oldconf->mode;
>> +	if (conf->fs == U8_MAX)
>> +		conf->fs = oldconf->fs;
>> +	if (conf->odr == U8_MAX)
>> +		conf->odr = oldconf->odr;
>> +	if (conf->filter == U8_MAX)
>> +		conf->filter = oldconf->filter;
>
>Maybe worth factoring out this bit of code filling in current values as I think
>it's used twice.
It makes sense.
>
>> +
>> +	/* Force the power mode against ODR when sensor is on. */
>> +	if (conf->mode > INV_ICM45600_SENSOR_MODE_STANDBY) {
>> +		if (conf->odr >= INV_ICM45600_ODR_6_25HZ_LP) {
>> +			conf->mode = INV_ICM45600_SENSOR_MODE_LOW_POWER;
>> +			conf->filter = INV_ICM45600_GYRO_LP_AVG_SEL_8X;
>> +		} else {
>> +			conf->mode = INV_ICM45600_SENSOR_MODE_LOW_NOISE;
>> +		}
>> +	}
>> +
>> +	/* Set GYRO_CONFIG0 register (gyro fullscale & odr). */
>> +	if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
>> +		val = FIELD_PREP(INV_ICM45600_GYRO_CONFIG0_FS_MASK, conf->fs) |
>> +		      FIELD_PREP(INV_ICM45600_GYRO_CONFIG0_ODR_MASK, conf->odr);
>> +		ret = regmap_write(st->map, INV_ICM45600_REG_GYRO_CONFIG0, val);
>> +		if (ret)
>> +			return ret;
>> +		oldconf->fs = conf->fs;
>> +		oldconf->odr = conf->odr;
>> +	}
>> +
>> +	/* Set GYRO_LP_AVG_SEL register (gyro low-power average filter). */
>> +	if (conf->filter != oldconf->filter) {
>> +		val = FIELD_PREP(INV_ICM45600_IPREG_SYS1_170_GYRO_LP_AVG_MASK, conf->filter);
>> +		ret = regmap_update_bits(st->map, INV_ICM45600_IPREG_SYS1_REG_170,
>> +			INV_ICM45600_IPREG_SYS1_170_GYRO_LP_AVG_MASK, val);
>> +		if (ret)
>> +			return ret;
>> +		oldconf->filter = conf->filter;
>> +	}
>> +
>> +	/* Set PWR_MGMT0 register (gyro sensor mode). */
>> +	return inv_icm45600_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode,
>> +					  sleep_ms);
>> +}
>>
>
>> +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. */
>> +	usleep_range(3000, 4000);
>
>fsleep probably appropriate here.
Ok!
>
>> +
>> +	return 0;
>> +}
>> +
>> +static void inv_icm45600_disable_vddio_reg(void *_data)
>> +{
>> +	regulator_disable((struct regulator *) _data);
>> +}
>> +
>> +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);
>
>Better to introduce this irq type etc when you use them.  That's probably the next patch?
Yes it should be part of next patch, I'll move it.
> 
>
>> +
>> +	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 dev_err_probe(dev, ret, "Failed to get vdd regulator\n");
>> +
>> +	/* IMU start-up time. */
>> +	fsleep(100000);
>> +
>> +	ret = inv_icm45600_enable_regulator_vddio(st);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_add_action_or_reset(dev, inv_icm45600_disable_vddio_reg, st->vddio_supply);
>
>I suspect you'll get an underflow on the vddio regulator enabled count if you remove
>the driver in whilst runtime suspended.  Avoiding that may require a dance where
>you force it out of runtime suspend at time of disabling runtime pm. 
Ok, our proposal is to avoid enabling (and disabling) vddio regulator "manually".
We will only rely on pm_runtime_get/put functions.
>
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Setup chip registers. */
>I don't see the comment as adding much - so probably drop it.
Agreed.
>
>> +	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);
>> +
>> +	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.
>> + */
>> +static int inv_icm45600_suspend(struct device *dev)
>> +{
>> +	struct inv_icm45600_state *st = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	guard(mutex)(&st->lock);
>> +
>> +	st->suspended.gyro = st->conf.gyro.mode;
>> +	st->suspended.accel = st->conf.accel.mode;
>
>Can you use pm_runtime_force_suspend() here.  That basically calls the
>runtime suspend callback (safely) if we are not already runtime suspended.
>
>Pair it with pm_runtime_force_resume()
>
>You will need to be careful with locking though. Probably fine to just
>unlock the mutex before calling that.
>
Yes, thanks for the guidance.
>
>> +	if (pm_runtime_suspended(dev))
>> +		return 0;
>> +
>> +	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;
>> +}
>> +
>> +/*
>> + * 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 = 0;
>> +
>> +	guard(mutex)(&st->lock);
>> +
>> +	if (pm_runtime_suspended(dev))
>> +		return 0;
>> +
>> +	ret = inv_icm45600_enable_regulator_vddio(st);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* 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)
>> +};
>> +
>> +MODULE_AUTHOR("InvenSense, Inc.");
>> +MODULE_DESCRIPTION("InvenSense ICM-456xx device driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS("IIO_INV_SENSORS_TIMESTAMP");
>> 
>
>

Powered by blists - more mailing lists