[PATCH V13 8/9] iio: imu: inv_icm42607: Add Gyroscope to icm42607

[Chris Morgan]

On Tue, Jun 16, 2026 at 01:13:03PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 15, 2026 at 12:25:51PM -0500, Chris Morgan wrote:
> 
> > Add gyroscope functions to the icm42607 driver.
> 
> ...
> 
> > +int inv_icm42607_set_gyro_conf(struct inv_icm42607_state *st,
> > +			       struct inv_icm42607_sensor_conf *conf,
> > +			       unsigned int *sleep_ms)
> > +{
> > +	struct inv_icm42607_sensor_conf *oldconf = &st->conf.gyro;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	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;
> 
> Same comment as per previous patch. But looking at this, can you rather have
> a helper that answers the below two questions? Something like
> 
> void _assign_conf(..., bool *write_odr, bool *write_filter)
> {
> 	...
> }
> EXPORT_...
> 
> in the core driver? But wight both approaches and choose either existing one
> (as in this patch series) or what I suggested.

In the core driver, I was going to do something like this:

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;

        if (conf->filter != oldconf->filter)
                *config1 = true;
}

So the step of copying stuff for sanity checking as well as checking
if I need to update config0 or config1 registers is handled here, and
I can simply check the values of config0 and config1 with an if to
determine if I need to call to write to the registers.

> 
> > +	if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> > +		val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG0_FS_SEL_MASK,
> > +				 conf->fs);
> > +		val |= FIELD_PREP(INV_ICM42607_GYRO_CONFIG0_ODR_MASK,
> > +				  conf->odr);
> > +		ret = regmap_write(st->map, INV_ICM42607_REG_GYRO_CONFIG0, val);
> > +		if (ret)
> > +			return ret;
> > +		oldconf->fs = conf->fs;
> > +		oldconf->odr = conf->odr;
> > +	}
> > +
> > +	if (conf->filter != oldconf->filter) {
> > +		val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG1_FILTER_MASK,
> > +				 conf->filter);
> > +		ret = regmap_update_bits(st->map, INV_ICM42607_REG_GYRO_CONFIG1,
> > +					 INV_ICM42607_GYRO_CONFIG1_FILTER_MASK, val);
> > +		if (ret)
> > +			return ret;
> > +		oldconf->filter = conf->filter;
> > +	}
> > +
> > +	return inv_icm42607_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode,
> > +					  st->conf.temp_en, sleep_ms);
> > +}
> 
> ...
> 
> > +{
> > +	unsigned int odr;
> > +	unsigned int i;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	odr = st->conf.gyro.odr;
> > +
> > +	for (i = 5; i < ARRAY_SIZE(inv_icm42607_gyro_odr); ++i) {
> 
> Same comment, why pre-increment?

I could not brain on this day, I had the dumb.
I'll fix it. :-)

> 
> > +		if (i == odr)
> > +			break;
> > +	}
> > +	if (i >= ARRAY_SIZE(inv_icm42607_gyro_odr))
> > +		return -EINVAL;
> > +
> > +	*val = inv_icm42607_gyro_odr[i][0];
> > +	*val2 = inv_icm42607_gyro_odr[i][1];
> > +
> > +	return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +static int inv_icm42607_gyro_write_odr(struct iio_dev *indio_dev,
> > +				       int val, int val2)
> > +{
> > +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> > +	struct device *dev = regmap_get_device(st->map);
> > +	unsigned int idx;
> > +	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> > +	int ret;
> > +
> > +	for (idx = 5; idx < ARRAY_SIZE(inv_icm42607_gyro_odr); ++idx) {
> 
> Ditto.
> 
> > +		if (val == inv_icm42607_gyro_odr[idx][0] &&
> > +		    val2 == inv_icm42607_gyro_odr[idx][1])
> > +			break;
> > +	}
> > +	if (idx >= ARRAY_SIZE(inv_icm42607_gyro_odr))
> > +		return -EINVAL;
> > +
> > +	conf.odr = 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_gyro_conf(st, &conf, NULL);
> > +}
> 
> Can be some of the code deduplicated between gyro and accel?

Probably a fair amount, but the deduplication will likely need to be
undone somewhat if we get buffer, WoM or apex support added back
(I don't have any devices with such functionality, so if anyone will
do it then it won't be me). I can refactor more if you want, or we
can keep it split like this to make it easy if someone else wants to
tackle the buffers/IRQs stuff later? Your call.

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Thank you,
Chris