Re: [PATCH V8 03/10] 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] Unsafe hardware reset polling loop with premature timeouts and missing delay.
- [High] Strict DT requirement for `interrupt-names` breaks compatibility with valid bindings.
--

commit 32920ed238e36be6e7fdd4fa2b84b2b710397641
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..b270d48335ba6
> --- /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)
> +{

[ ... ]

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

Could this polling loop fail prematurely if the device returns a read error
while resetting?

Since regmap_read_poll_timeout() evaluates the loop condition as
__ret || (cond), any read error will immediately abort the loop and
return the error. If the device is unresponsive on the bus during reset
(e.g. returning an I2C NACK), it will instantly defeat the polling mechanism.

Additionally, could the timeout of exactly 1ms cause spurious probe failures?

Since 1ms is the typical hardware reset time, this doesn't leave any margin
for scheduling variations or natural hardware differences.

Would it be better to add a mandatory sleep before polling, and increase the
timeout margin?

[ ... ]

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

Does this break compatibility with valid device trees that omit the
optional interrupt-names property?

If interrupt-names is not provided in the device tree, fwnode_irq_get_byname()
will return -EINVAL and abort the probe.

Could this be modified to fallback to fetching the interrupt by index
(e.g. fwnode_irq_get(dev_fwnode(dev), 0)) if the named lookup fails?

> +
> +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);

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