Re: [PATCH V7 03/11] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Overriding `fwnode_irq_get_byname` return value breaks probe deferral.
- [Medium] Permissive WHOAMI check allows successful probe of non-existent devices on floating SPI busses.
--

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

iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

This commit adds the core component of the new inv_icm42607 driver. It
includes setup functions and the full register definition in the header file.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> new file mode 100644
> index 0000000000000..9784709319b92
> --- /dev/null
> +++ 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;
> +
> +	ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != st->hw->whoami)
> +		dev_warn(dev, "invalid whoami %#02x expected %#02x (%s)\n",
> +			 val, st->hw->whoami, st->hw->name);

Should the WHOAMI check fail the probe if the device returns completely
invalid values like 0xff or 0x00?

If the device is not present and the SPI MISO line is floating high,
regmap_read() will return 0xff for the WHOAMI register. Because this check
only issues a warning, the driver continues the probe sequence.

> +
> +	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
> +			   INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(1000);
> +
> +	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);

Later, when polling for the reset completion here, the condition
(val & INV_ICM42607_INT_STATUS_RESET_DONE) checks for BIT(4). A floating bus
returning 0xff will immediately evaluate to true, allowing the probe to
succeed. This could result in a phantom IIO device being created that only
returns garbage data.

[ ... ]

> +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 overriding the error code from fwnode_irq_get_byname() break probe
deferral?

If the interrupt controller is not yet ready, fwnode_irq_get_byname() will
return -EPROBE_DEFER. By unconditionally squashing the return value into
-EINVAL when calling dev_err_probe(), the kernel will permanently fail to
probe the device instead of deferring it. Could we propagate the actual irq
error code instead?

[ ... ]

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