Re: [PATCH V7 04/11] iio: imu: inv_icm42607: Add I2C and SPI For icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Masking `-EPROBE_DEFER` and strict IRQ naming breaks device probe lifecycle and standard devicetrees.
- [High] `regcache_mark_dirty()` usage causes the hardware state to desync, skipping required bus configuration after soft reset.
- [High] SPI 3-wire mode configuration uses a read-modify-write cycle, reading garbage data and corrupting device registers.
- [Medium] Valid error codes from regmap operations are discarded and replaced with -EIO.
--

commit 7dbec8ad22b33274f815169a6c802cbf63590cea
Author: Chris Morgan <macromorgan@hotmail.com>

iio: imu: inv_icm42607: Add I2C and SPI For icm42607

This commit adds I2C and SPI driver support for the InvenSense
ICM-42607 motion tracking device.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c
> new file mode 100644
> index 0000000000000..49438fa6f8673
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c

[ ... ]

> +static int inv_icm42607_spi_bus_setup(struct inv_icm42607_state *st)
> +{
> +	const struct device *dev = regmap_get_device(st->map);
> +	const struct spi_device *spi = to_spi_device(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	if (spi->mode & SPI_3WIRE)
> +		ret = regmap_clear_bits(st->map, INV_ICM42607_REG_DEVICE_CONFIG,
> +					INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE);

Since the device powers on in 4-wire mode, won't this initial
read-modify-write fail for a 3-wire configured host?

The host expects data on MOSI but the device drives MISO, which would result
in reading garbage data and permanently corrupting the rest of the
configuration bits in DEVICE_CONFIG when written back.

Should a blind regmap_write() be used here instead to safely establish
3-wire mode?

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 9784709319b92..1088c5c7076f8 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> +static int inv_icm42607_setup(struct inv_icm42607_state *st,
> +			      inv_icm42607_bus_setup bus_setup)
> +{
> +	const struct device *dev = regmap_get_device(st->map);
> +	unsigned int val;
> +	int ret;
> +
> +	/*
> +	 * Setup the bus first in case we need to set the SPI mode or
> +	 * change the slew rate in order.
> +	 */
> +	ret = bus_setup(st);
> +	if (ret)
> +		return ret;

[ ... ]

> +	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
> +			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
> +	if (ret)
> +		return ret;
>  
> -	fsleep(1000);
> +	/*
> +	 * In order to confirm a reset is complete, we need to read the reset
> +	 * bit, but in certain circumstances we need to set the bus up before
> +	 * we can do a read. So we should wait the required amount of time
> +	 * per the datasheet first, then set the bus up again, then read to
> +	 * ensure the reset status is done. Invalidate the regmap cache since
> +	 * we're doing a hardware reset.
> +	 */
> +	regcache_mark_dirty(st->map);
>  
> -	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 * 1000);
> -	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "reset error, reset done bit not set\n");
> +	fsleep(INV_ICM42607_RESET_TIME_MS * 1000);
>  
>  	ret = bus_setup(st);

Because regcache_mark_dirty() only flags the cache as needing a sync but does
not clear the cached values, will regmap_update_bits() in this second
bus_setup() call silently skip the hardware writes?

The cache would still show the bits as set from the first bus_setup(), leaving
the hardware in its default reset state.

Would regcache_sync() or regcache_drop_region() be more appropriate here to
force the actual hardware writes?

>  	if (ret)
>  		return ret;
>  
> +	ret = regmap_read(st->map, INV_ICM42607_REG_INT_STATUS, &val);
> +	if (ret || (!(val & INV_ICM42607_INT_STATUS_RESET_DONE)))
> +		return dev_err_probe(dev, -EIO,
> +				     "reset error, reset done bit not set\n");

If the underlying bus read fails (e.g., returning -ENXIO or -ETIMEDOUT), does
it make sense to mask the actual error code with a hardcoded -EIO?

Could the original error code be propagated to help debug underlying bus
subsystem issues?

[ ... ]

> +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 fwnode_handle *fwnode = dev_fwnode(dev);
> +	struct inv_icm42607_state *st;
> +	int irq;
> +	int ret;
> +
> +	irq = fwnode_irq_get_byname(fwnode, "INT1");
> +	if (!(irq > 0))
> +		return dev_err_probe(dev, -EINVAL, "Unable to get INT1 interrupt\n");

Does this prevent probe deferral if the interrupt controller isn't ready yet?

Since fwnode_irq_get_byname() would return -EPROBE_DEFER, converting it to
-EINVAL causes a permanent probe failure instead of retrying.

Additionally, if the device tree binding specifies interrupt-names as optional,
will this strict name enforcement cause a hard failure for valid devicetrees
that omit it?

Should the driver fall back to an index-based lookup or use the
framework-provided client->irq / spi->irq if the name is missing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515130018.237378-1-macroalpha82@gmail.com?part=4