Re: [PATCH V5 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 5 potential issue(s) to consider:
- [High] Missing NULL check for match data leads to kernel panic.
- [Medium] Incorrect regmap ranges configuration forces an unnecessary dummy read of register 0 before every access.
- [Medium] The driver ignores a mismatched WHOAMI value due to a missing return statement.
- [Medium] The SPI bus setup unconditionally forces 4-wire SPI mode, breaking 3-wire configurations.
- [Low] IRQ mapping failures are treated as valid interrupts.
--

commit 52263283b912417c0c4ca749220464cd92114b9c
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 e84e84416dab5..8ff51711ab1fe 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -15,6 +15,57 @@
>  
>  #include "inv_icm42607.h"
>  
> +static const struct regmap_range_cfg inv_icm42607_regmap_ranges[] = {
> +	{
> +		.name = "user bank",
> +		.range_min = 0x0000,
> +		.range_max = 0x00FF,
> +		.window_start = 0,
> +		.window_len = 0x0100,
> +	},
> +};

Is this regmap range configuration missing a selector register?

Because selector_reg and selector_mask default to 0, and max_register is
0xFF, paging might be unnecessary here. During regmap access to registers
other than 0, the page check could evaluate to true and trigger a dummy read
on register 0 before every transaction.

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c
> new file mode 100644
> index 0000000000000..3859517f88c94
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c
> @@ -0,0 +1,84 @@
[ ... ]
> +static int inv_icm42607_probe(struct i2c_client *client)
> +{
> +	const struct inv_icm42607_hw *hw = i2c_get_match_data(client);
> +	struct regmap *regmap;

If the device is bound dynamically via sysfs without a matching device ID,
can i2c_get_match_data() return NULL? If so, could this lead to a kernel
panic when hw is unconditionally dereferenced later in
inv_icm42607_core_probe()?

[ ... ]

> 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..4a38a11f2baef
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_spi.c
> @@ -0,0 +1,91 @@
[ ... ]
> +static int inv_icm42607_spi_bus_setup(struct inv_icm42607_state *st)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_set_bits(st->map, INV_ICM42607_REG_DEVICE_CONFIG,
> +			      INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE);

Does this unconditionally force 4-wire SPI mode, even if the SPI controller
is configured for 3-wire mode?

If the hardware is physically connected using 3-wire SPI and configured with
spi-3wire in the device tree, will forcing the 4-wire bit break
communication? Should this check for SPI_3WIRE first?

> +	if (ret)
> +		return ret;
[ ... ]
> +static int inv_icm42607_probe(struct spi_device *spi)
> +{
> +	const struct inv_icm42607_hw *hw = spi_get_device_match_data(spi);
> +	struct regmap *regmap;

Similarly, can spi_get_device_match_data() return NULL here and cause a
kernel panic during probe if the device is bound dynamically?

[ ... ]

Could the WHOAMI mismatch warning be silently ignored?

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c:inv_icm42607_setup() {
    ...
	if (val != st->hw->whoami)
		dev_warn_probe(dev, -ENODEV,
			       "invalid whoami %#02x expected %#02x (%s)\n",
			       val, st->hw->whoami, st->hw->name);

	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
    ...
}

It looks like the driver attempts to abort by calling dev_warn_probe(), but
fails to return the result. Does this cause the -ENODEV error to be
discarded, allowing the driver to proceed configuring an unrecognized device?

Are IRQ mapping failures handled correctly during probe?

drivers/iio/imu/inv_icm42607/inv_icm42607_core.c:inv_icm42607_core_probe() {
    ...
	irq = fwnode_irq_get_byname(fwnode, "INT1");
	if (irq < 0)
		return dev_err_probe(dev, irq, "error missing INT1 interrupt\n");
    ...
}

fwnode_irq_get_byname() can return 0 if the interrupt exists but fails to be
mapped by the irq domain. By only checking if irq < 0, does this allow 0 to
pass as a valid IRQ, potentially failing later when requesting the threaded
IRQ?

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