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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Using regmap_write() instead of regmap_update_bits() in configuration functions unintentionally clobbers reserved or unhandled register fields.
- [Medium] Strict requirement for 'INT1' named interrupt breaks ACPI support and simple Device Trees.
- [Low] regcache_sync() is a no-op because regcache_mark_dirty() is not called after the hardware soft reset.
--

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

iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
    
Add the core component of a new inv_icm42607 driver. This includes
a few 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..4801ece280589
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> +static int inv_icm42607_set_conf(struct inv_icm42607_state *st,
> +				 const struct inv_icm42607_conf *conf)
> +{
[ ... ]
> +	val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG1_FILTER_MASK, conf->gyro.filter);
> +	ret = regmap_write(st->map, INV_ICM42607_REG_GYRO_CONFIG1, val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK, conf->accel.filter);
> +	ret = regmap_write(st->map, INV_ICM42607_REG_ACCEL_CONFIG1, val);

[Severity: High]
Does this regmap_write() unintentionally clobber the averaging
configuration bits (INV_ICM42607_ACCEL_CONFIG1_AVG_MASK)?

By writing the entire 8-bit register, any bits outside of the filter mask
are cleared to zero. This clobbers the averaging configuration (bits 6:4),
discarding the hardware default. Should this use regmap_update_bits()
instead to safely modify only the filter bits?

> +	if (ret)
> +		return ret;
> +
> +	st->conf = *conf;
> +
> +	return 0;
> +}
[ ... ]
> +static int inv_icm42607_setup(struct inv_icm42607_state *st,
> +			      inv_icm42607_bus_setup bus_setup)
> +{
[ ... ]
> +	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 * 10000);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "reset error, reset done bit not set\n");
> +
> +	/* Sync the regcache again after a reset. */
> +	ret = regcache_sync(st->map);

[Severity: Low]
Is this regcache_sync() a no-op?

It is called immediately after a software reset, but regcache_sync()
returns immediately if the map's cache_dirty flag is false. Since
regcache_mark_dirty() was never called after the hardware reset, the
cache is still considered clean.

> +	if (ret)
> +		return ret;
> +
> +	ret = bus_setup(st);
[ ... ]
> +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");

[Severity: Medium]
Will this strict requirement for an "INT1" named interrupt break probing on
systems with ACPI or simple single-interrupt Device Trees?

When an interrupt is provided without an "interrupt-names" property,
fwnode_irq_get_byname() fails. Would it be more compatible to fall back to
fwnode_irq_get(dev_fwnode(dev), 0) or use the irq provided by the I2C/SPI
client core before unconditionally aborting the probe?

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