[PATCH v16 07/10] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[Andy Shevchenko]

On Mon, Jul 13, 2026 at 04:58:37PM -0500, Chris Morgan wrote:

> Add icm42607 accelerometer sensor for icm42607.

...

> +#define INV_ICM42607_ACCEL_CHAN(_modifier, _index, _ext_info)			\

> +{										\

You can also make it a compound literal, so it will be possible to use in the
assignments at run-time (maybe not needed).

(struct iio_chan_spec) {

But again, it's up to you.

> +	.type = IIO_ACCEL,							\
> +	.modified = 1,								\
> +	.channel2 = _modifier,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),				\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),			\
> +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +	.scan_index = _index,							\
> +	.scan_type = {								\
> +		.sign = 's',							\
> +		.realbits = 16,							\
> +		.storagebits = 16,						\
> +		.endianness = IIO_BE,						\
> +	},									\
> +	.ext_info = _ext_info,							\
> +}

...

> +static int inv_icm42607_accel_write_scale(struct iio_dev *indio_dev,
> +					  int val, int val2)
> +{
> +	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	size_t scales_len = ARRAY_SIZE(inv_icm42607_accel_scale_nano);
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int idx;
> +	int ret;
> +
> +	for (idx = 0; idx < scales_len; idx++) {
> +		if (val == inv_icm42607_accel_scale_nano[idx][0] &&
> +		    val2 == inv_icm42607_accel_scale_nano[idx][1])
> +			break;
> +	}
> +	if (idx >= scales_len)

'==' should suffice, otherwise it will be an interesting case.

> +		return -EINVAL;
> +
> +	conf.fs = idx;
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	return inv_icm42607_set_sensor_conf(st, &conf, IIO_ACCEL);
> +}

...

> +static int inv_icm42607_accel_read_odr(struct inv_icm42607_state *st,
> +				       int *val, int *val2)
> +{
> +	unsigned int odr;
> +	unsigned int i;
> +
> +	guard(mutex)(&st->lock);
> +
> +	odr = st->conf.accel.odr;
> +
> +	for (i = INV_ICM42607_ODR_1600HZ; i < ARRAY_SIZE(inv_icm42607_accel_odr); i++) {
> +		if (i == odr)
> +			break;
> +	}
> +	if (i >= ARRAY_SIZE(inv_icm42607_accel_odr))
> +		return -EINVAL;

Ditto. And so on.

> +	*val = inv_icm42607_accel_odr[i][0];
> +	*val2 = inv_icm42607_accel_odr[i][1];
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}

...

> +struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->map);
> +	struct inv_icm42607_sensor_state *accel_st;
> +	struct iio_dev *indio_dev;
> +	const char *name;
> +	int ret;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "%s-accel", st->hw->name);
> +	if (!name)
> +		return ERR_PTR(-ENOMEM);
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*accel_st));
> +	if (!indio_dev)
> +		return ERR_PTR(-ENOMEM);

> +	accel_st = iio_priv(indio_dev);
> +

Move this blank line to be above on the previous assignment.

> +	accel_st->power_mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
> +	accel_st->filter = INV_ICM42607_FILTER_BW_73HZ;

> +
> +	iio_device_set_drvdata(indio_dev, st);

And here I would rather move this line to be after the all assignments, so we
won't have the potential code that might use unfilled data in between.

> +	indio_dev->name = name;
> +	indio_dev->info = &inv_icm42607_accel_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = inv_icm42607_accel_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(inv_icm42607_accel_channels);
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return indio_dev;
> +}

...

> +static void inv_icm42607_update_config(struct inv_icm42607_sensor_conf *conf,
> +				       struct inv_icm42607_sensor_conf *oldconf,
> +				       bool *config0, bool *config1)
> +{
> +	if (conf->mode < 0)
> +		conf->mode = oldconf->mode;
> +	if (conf->fs < 0)
> +		conf->fs = oldconf->fs;
> +	if (conf->odr < 0)
> +		conf->odr = oldconf->odr;
> +	if (conf->filter < 0)
> +		conf->filter = oldconf->filter;

> +	if (conf->fs != oldconf->fs || conf->odr != oldconf->odr)
> +		*config0 = true;
> +	else
> +		*config0 = false;

Can be simply

	*config0 = (conf->fs != oldconf->fs) || (conf->odr != oldconf->odr);

(I added parentheses for better reading of this expression to make it clear
 that the recipient is a boolean.)

> +	if (conf->filter != oldconf->filter)
> +		*config1 = true;
> +	else
> +		*config1 = false;

In the similar way.

> +}

...

> +int inv_icm42607_read_sensor(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     s16 *val)
> +{
> +	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct inv_icm42607_sensor_state *sensor_st = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int reg;

> +	u8 data[2];

Define using the correct type. AFAICS it should be __be16 data;

> +	int ret;
> +
> +	if ((chan->type != IIO_ANGL_VEL) && (chan->type != IIO_ACCEL))
> +		return -EINVAL;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +		if (chan->type == IIO_ANGL_VEL)
> +			reg = INV_ICM42607_REG_GYRO_DATA_X1;
> +		else
> +			reg = INV_ICM42607_REG_ACCEL_DATA_X1;
> +		break;
> +	case IIO_MOD_Y:
> +		if (chan->type == IIO_ANGL_VEL)
> +			reg = INV_ICM42607_REG_GYRO_DATA_Y1;
> +		else
> +			reg = INV_ICM42607_REG_ACCEL_DATA_Y1;
> +		break;
> +	case IIO_MOD_Z:
> +		if (chan->type == IIO_ANGL_VEL)
> +			reg = INV_ICM42607_REG_GYRO_DATA_Z1;
> +		else
> +			reg = INV_ICM42607_REG_ACCEL_DATA_Z1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* enable sensor */
> +	conf.mode = sensor_st->power_mode;
> +	conf.filter = sensor_st->filter;
> +	ret = inv_icm42607_set_sensor_conf(st, &conf, chan->type);
> +	if (ret)
> +		return ret;
> +
> +	/* read sensor register data */
> +	ret = regmap_bulk_read(st->map, reg, data, sizeof(data));
> +	if (ret)
> +		return ret;
> +
> +	*val = get_unaligned_be16(data);
> +	if (*val == INV_ICM42607_DATA_INVALID)
> +		return -EINVAL;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko