[PATCH V10 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

[Chris Morgan]

On Fri, Jun 05, 2026 at 07:08:35PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 04, 2026 at 03:18:25PM -0500, Chris Morgan wrote:
> 
> > Add the core component of a new inv_icm42607 driver. This includes
> > a few setup functions and the full register definition in the
> > header file.
> 
> ...
> 
> > +#ifndef INV_ICM42607_H_
> > +#define INV_ICM42607_H_
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> 
> I haven't found users for these two.
> 
> + bits.h // BIT() / GENMASK()

I'll correct this.

> 
> > +#include <linux/iio/iio.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> 
> 
> > +#include <linux/regulator/consumer.h>
> 
> No users for this one.
> 
> + types.h // for bool
> 
> ...
> 

And this.

> > +enum inv_icm42607_sensor_mode {
> > +	INV_ICM42607_SENSOR_MODE_OFF,
> > +	INV_ICM42607_SENSOR_MODE_STANDBY,
> > +	INV_ICM42607_SENSOR_MODE_LOW_POWER,
> > +	INV_ICM42607_SENSOR_MODE_LOW_NOISE,
> 
> Are those enums map 1:1 to HW bits or bitfields? If so, assign explicitly each
> of them.

I'll explicitly set every value in the enums to be clear.

> 
> > +	INV_ICM42607_SENSOR_MODE_NB
> 
> Is this a terminator like NUMBER_OF ?

This is a terminator, yes. I will not set these explicitly and keep
commas off of them to keep them easily identifable as terminators.

> 
> > +};
> 
> ...
> 
> > +/* ODR values */
> > +enum inv_icm42607_odr {
> > +	INV_ICM42607_ODR_1600HZ = 5,
> 
> See above. This one is problematic. No one should rely on Linux/C enums when
> it's about HW bits. All HW related stuff has to be explicit.
> 

I will explicitly set the values here.

> > +	INV_ICM42607_ODR_800HZ,
> > +	INV_ICM42607_ODR_400HZ,
> > +	INV_ICM42607_ODR_200HZ,
> > +	INV_ICM42607_ODR_100HZ,
> > +	INV_ICM42607_ODR_50HZ,
> > +	INV_ICM42607_ODR_25HZ,
> > +	INV_ICM42607_ODR_12_5HZ,
> > +	INV_ICM42607_ODR_6_25HZ_LP,
> > +	INV_ICM42607_ODR_3_125HZ_LP,
> > +	INV_ICM42607_ODR_1_5625HZ_LP,
> > +	INV_ICM42607_ODR_NB
> > +};
> 
> ...
> 
> > +struct inv_icm42607_sensor_conf {
> > +	int mode;
> > +	int fs;
> > +	int odr;
> > +	int filter;
> 
> All of them are supposed to be signed? Why?
> 
> > +};
> 

A later commit will use -1 for invalid values.

> ...
> 
> > +struct inv_icm42607_hw {
> > +	uint8_t whoami;
> 
> What's wrong with u8?

Nothing, I'll make the change.

> 
> > +	const char *name;
> > +	const struct inv_icm42607_conf *conf;
> > +};
> 
> ...
> 
> > +#include <linux/delay.h>
> > +#include <linux/dev_printk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> 
> IWYU, please.
> 
> ...
> 

This may sound like a dumb question, but what's the best way to invoke
iwyu for a kernel? None of the readmes or man pages seem to be getting
me anywhere...

> > +/**
> > + *  inv_icm42607_setup() - check and setup chip
> > + *  @st:	driver internal state
> > + *  @bus_setup:	callback for setting up bus specific registers
> > + *
> > + *  Returns 0 on success, a negative error code otherwise.
> 
> If you do kernel-doc, validate it. Return section is missing here.
> 

I'll just nuke it, most of the stuff is fairly self explanatory anyway.

> > + */
> 
> ...
> 
> > +{
> > +	const struct device *dev = regmap_get_device(st->map);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (val != st->hw->whoami)
> > +		dev_warn(dev, "Unknown whoami %#02x expected %#02x (%s)\n",
> > +			 val, st->hw->whoami, st->hw->name);
> 
> dev_warn_probe() ?
> 
> > +	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
> > +			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
> > +	if (ret)
> > +		return ret;
> 
> > +	fsleep(INV_ICM42607_RESET_TIME_MS * 1000);
> 
> USEC_PER_MSEC (needs time.h)

> 
> > +	ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
> > +				       val, val & INV_ICM42607_INT_STATUS_RESET_DONE,
> > +				       INV_ICM42607_RESET_TIME_MS * 100,
> > +				       INV_ICM42607_RESET_TIME_MS * 10000);
> 
> These are weird, as in the first case it's actually 1/10th of _RESET_TIME_MS.
> Perhaps you need to reconsider what you use as that constant. Personally I
> prefer to see just plain values with the multipliers (to convert to µs).
> 

I honestly don't know what a good wait period between polling should
be, so I guessed at 1/10th the timeout period. I will however change
the timeout to us.

> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "reset error, reset done bit not set\n");
> > +
> > +	/* Sync the regcache again after a reset. */
> > +	regcache_mark_dirty(st->map);
> > +	ret = regcache_sync(st->map);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = bus_setup(st);
> 
> Hmm... This is bad name with potential of name collision in the future (in case
> driver bus code wants to have the same name for the function).

Thanks, I'll rename the function.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_set_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
> > +			      INV_ICM42607_INTF_CONFIG0_SENSOR_DATA_ENDIAN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG1,
> > +				 INV_ICM42607_INTF_CONFIG1_CLKSEL_MASK,
> > +				 INV_ICM42607_INTF_CONFIG1_CLKSEL_PLL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return inv_icm42607_set_conf(st, st->hw->conf);
> > +}
> 
> ...
> 
> > +int inv_icm42607_core_probe(struct regmap *regmap,
> > +			    const struct inv_icm42607_hw *hw,
> > +			    inv_icm42607_bus_setup bus_setup)
> > +{
> > +	struct device *dev = regmap_get_device(regmap);
> > +	struct inv_icm42607_state *st;
> > +	int ret;
> > +
> > +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > +	if (!st)
> > +		return -ENOMEM;
> > +
> > +	ret = devm_mutex_init(dev, &st->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->hw = hw;
> > +	st->map = regmap;
> > +
> > +	ret = iio_read_mount_matrix(dev, &st->orientation);
> > +	if (ret)
> 
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to retrieve mounting matrix %d\n", ret);
> 
> Remove duplicate ret printing.
> 

Understood.

> > +
> > +	ret = devm_regulator_get_enable(dev, "vdd");
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to get vdd regulator\n");
> > +
> > +	st->vddio_supply = devm_regulator_get(dev, "vddio");
> > +	if (IS_ERR(st->vddio_supply))
> > +		return dev_err_probe(dev, PTR_ERR(st->vddio_supply),
> > +				     "Failed to get vddio regulator\n");
> > +
> > +	ret = inv_icm42607_enable_vddio_reg(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_add_action_or_reset(dev, inv_icm42607_disable_vddio_reg, st);
> > +	if (ret)
> > +		return ret;
> 
> > +	/* Setup chip registers (includes WHOAMI check, reset check, bus setup) */
> > +	ret = inv_icm42607_setup(st, bus_setup);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> Just
> 
> 	return inv_icm42607_setup(st, bus_setup);
> 
> ? Or is it going to be extended in the next changes?
> 
> > +}

It will get extended, so I'll keep this for now if that's okay.

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

Thank you for your feedback. And please let me know about the iwyu
tool, that seems like it would be useful.

- Chris