Re: [PATCH v6 5/6] iio: imu: inv_icm42607: Implement MREGx register access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] build: SPI driver fails to link when I2C is disabled due to missing regmap_config
- [High] iio: resume failure due to regcache_sync() attempting bulk writes on MREG registers
- [Low] iio: INV_ICM42607_REG_ZG_ST_DATA is defined as max_register but is not readable
--

commit 402c5b10cd3776a6da15b9bcc49baac45dadfdbb
Author: Kanak Shilledar <kanak.shilledar@axis.com>

iio: imu: inv_icm42607: Implement MREGx register access

The device supports indirect register access to different banks. A
specific routine needs to be followed when accessing the registers in
another bank as documented in the datasheet (section 13). This is
required for accessing registers configured via the user and
implementing buffer support.

Link: https://www.invensense.tdk.com/en-us/products/3-axis/icm-42370-p
Link: https://www.lcsc.com/product-detail/C5129967.html

> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -18,63 +18,255 @@

[ ... ]

> +static int inv_icm42607_mreg_write(struct regmap *map, unsigned int reg,
> +				   const u8 *data, size_t count)
> +{
> +	bool idle_set;
> +	int ret, ret2;
> +	u8 blk_sel;
> +
> +	/* MREG access is one byte per transaction, no burst support. */
> +	if (count != 1)
> +		return -EINVAL;

[Severity: High]
Does this strict size check cause system resume to fail?

If a user reads or writes USER0 through USER8 registers via debugfs, they are
cached and can become dirty. During system resume, inv_icm42607_resume() calls
regcache_mark_dirty() followed by regcache_sync(). Because the regcache uses
REGCACHE_MAPLE and the config below omits .use_single_write = true,
regcache_sync() will group contiguous dirty MREG registers into a single bulk
write. This bulk write is then passed to inv_icm42607_mreg_write(), which will
return -EINVAL, causing regcache_sync() and the resume process to fail.

[ ... ]

> +static bool inv_icm42607_is_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case INV_ICM42607_REG_MCLK_RDY ... INV_ICM42607_REG_INT_CONFIG:
> +	case INV_ICM42607_REG_TEMP_DATA1 ... INV_ICM42607_REG_TMST_FSYNCL:
> +	case INV_ICM42607_REG_APEX_DATA4 ... INV_ICM42607_REG_INTF_CONFIG1:
> +	case INV_ICM42607_REG_INT_STATUS_DRDY ... INV_ICM42607_REG_FIFO_DATA:
> +	case INV_ICM42607_REG_WHOAMI:
> +	case INV_ICM42607_REG_OFFSET_USER0 ... INV_ICM42607_REG_OFFSET_USER8:
>  		return true;
>  	}
>  
>  	return false;
>  }

[ ... ]

> +static const struct regmap_config inv_icm42607_virt_regmap_config = {
> +	.name = "banks",
> +	.reg_bits = 16,
>  	.val_bits = 8,
>  	.writeable_reg = inv_icm42607_is_writeable_reg,
>  	.readable_reg = inv_icm42607_is_readable_reg,
>  	.volatile_reg = inv_icm42607_is_volatile_reg,
> -	.max_register = INV_ICM42607_REG_WHOAMI,
> +	.max_register = INV_ICM42607_REG_ZG_ST_DATA,
>  	.cache_type = REGCACHE_MAPLE,
>  };

[Severity: Low]
Does this prevent INV_ICM42607_REG_ZG_ST_DATA from actually being accessed?

While INV_ICM42607_REG_ZG_ST_DATA is defined as .max_register, it is missing
from the switch statements in both inv_icm42607_is_readable_reg() and
inv_icm42607_is_writeable_reg(). Will this cause the regmap core to block any
attempts to read or write it, returning an error like -EIO?

> -EXPORT_SYMBOL_NS_GPL(inv_icm42607_regmap_config, "IIO_ICM42607");

> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c
> @@ -12,6 +12,12 @@
>  
>  #include "inv_icm42607.h"
>  
> +const struct regmap_config inv_icm42607_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +EXPORT_SYMBOL_NS_GPL(inv_icm42607_regmap_config, "IIO_ICM42607");
> +

[Severity: High]
Does this break the build when the SPI driver is compiled without I2C support?

Because inv_icm42607_regmap_config was moved exclusively to the I2C-specific
file, compiling the kernel with CONFIG_INV_ICM42607_SPI=y and
CONFIG_INV_ICM42607_I2C=n will skip compiling inv_icm42607_i2c.c. Since the SPI
driver still references this configuration via devm_regmap_init_spi(), will it
cause an undefined reference linker error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-b4-inv_icm42370p-v6-0-e53884f185ff@axis.com?part=5