Re: [PATCH V6 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 2 potential issue(s) to consider:
- [High] Probe incorrectly returns 0 (success) when the IRQ mapping fails, leading to an uninitialized but successfully bound driver.
- [High] SPI 3-wire mode initialization will fail to probe or corrupt the device configuration register due to a read-modify-write on a mismatched bus state.
--

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

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

Add I2C and SPI driver support for InvenSense ICM-42607 devices.
Add necessary Kconfig and Makefile to allow building of (incomplete)
driver.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 0baf4763b0a7d..6119379ca6a8b 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -154,7 +211,7 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
>  	int ret;
>  
>  	irq = fwnode_irq_get_byname(fwnode, "INT1");
> -	if (irq < 0)
> +	if (irq <= 0)
>  		return dev_err_probe(dev, irq, "error missing INT1 interrupt\n");

If fwnode_irq_get_byname() returns 0, could this lead to an uninitialized but
successfully bound driver?

Since dev_err_probe() returns the error code it is passed, passing 0
will cause inv_icm42607_core_probe() to immediately return 0. This bypasses
the driver data setup and hardware initialization, which might cause a NULL
pointer dereference later during suspend when dev_get_drvdata() is called.

Would it be better to pass an explicit negative error code like -EINVAL when
irq is 0?

> 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);

Does this read-modify-write correctly handle a device that powers up in
4-wire mode when the host expects 3-wire mode?

It appears inv_icm42607_setup() performs reads (like a WHOAMI check and
reset poll) before calling bus_setup(). If the chip is in 4-wire mode and
outputs on SDO while the host listens on MOSI/SISO, wouldn't these reads
return garbage?

If the reads fail or return garbage, wouldn't regmap_clear_bits() read
that garbage and write it back, corrupting other bits in the register
like SPI_MODE?

Should 3-wire mode be enabled using a direct regmap_write() prior to
any reads?

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