Re: [PATCH v7 6/7] iio: imu: inv_icm42607: Implement MREGx register access

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 26 Oct 2026 12:02:54 +0100
Kanak Shilledar <kanak.shilledar@axis.com> wrote:

> 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. The implementation is inspired from the
> icm45600 driver.
> 
> The banks are defined based on their initial bank access code which are
> written to the BLK_SEL_* regs. However, there is a variation for MREG1
> as User Bank 0 and MREG1 both share the same 0x00, so User Bank 1 has
> 0x00 and MREG1 has 0x01. When MREG1 register is accessed, then BLK_SEL_*
> is written with 0x00 instead of 0x01. All the register access goes
> through the 16 bit virtual regmap layered on top of the 8bit bus regmap.
> Bank id in the upper byte and the register address in the lower byte.
> Bank 0 accesses are forwarded to the bus regmap with the bank field
> stripped, while MREG accesses are forwarded to bank switching sequence.
> 
> Due to the specific bank switching routine, it is not possible to use
> regmap-ranges. Only User Bank 0 supports bulk read/writes whereas other
> banks can only be accessed serially. Each time some data in the other
> bank needs to be accessed, it should go through the complete bank
> switching sequence.
> 
> Datasheet: https://www.invensense.tdk.com/en-us/products/3-axis/icm-42370-p
> Datasheet: https://www.lcsc.com/product-detail/C5129967.html
> Assisted-by: LLM

Drop this assisted by. The rules changes again :(  Now the plan is
just to have the LLM usage bit you have below the --- and
keep nothing in the git log.

> Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com>
> ---
> LLM was used to implement the mreg back switching support.
> ---
>  drivers/iio/imu/inv_icm42607/inv_icm42607.h      | 166 ++++++++++------
>  drivers/iio/imu/inv_icm42607/inv_icm42607_core.c | 242 +++++++++++++++++++++--
>  2 files changed, 329 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> index 674a9896fabb3..eecf4c46cbdb0 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h


> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 018a7506ad50c..414642f8ec930 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

> -	return false;
> +static int inv_icm42607_mreg_read(struct regmap *map, unsigned int reg,
> +				  u8 *data, size_t count)
> +{
> +	int ret, ret2, ret3;
> +	unsigned int val;
> +	bool idle_set;
> +	u8 blk_sel;
> +
> +	/* MREG access is one byte per transaction, no burst support. */
> +	if (count != 1)
> +		return -EINVAL;
> +
> +	ret = inv_icm42607_blk_sel(reg, &blk_sel);
> +	if (ret)
> +		return ret;
> +
> +	ret = inv_icm42607_mclk_get(map, &idle_set);
> +	if (ret)
If this fails, why don't we need to undo the blk select?
> +		return ret;
> +
> +	ret = regmap_write(map, INV_ICM42607_REG_BLK_SEL_R, blk_sel);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_write(map, INV_ICM42607_REG_MADDR_R,
> +			   FIELD_GET(INV_ICM42607_REG_ADDR_MASK, reg));
> +	fsleep(INV_ICM42607_MREG_ACCESS_DELAY_US);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_read(map, INV_ICM42607_REG_M_R, &val);
> +	fsleep(INV_ICM42607_MREG_ACCESS_DELAY_US);
> +	if (ret)
> +		goto out;
> +
> +	*data = val;
> +out:
> +	/* Restore direct access. */
> +	ret2 = regmap_write(map, INV_ICM42607_REG_BLK_SEL_R, 0);
> +	ret3 = inv_icm42607_mclk_put(map, idle_set);
> +
> +	return ret ?: ret2 ?: ret3;
From a readability point of view, this is getting a bit too creative.
I think I'd just separate the good and bad paths and burn a few more
lines to do it.  The lack of balance between get and release also
concerns me.

If we to do blk select then mclk manipulation in the same order for
both, then break it out to helpers where you can easily document why.

static int inv_icm42607_switch_to_mreg()
{
	int ret;

	ret = inv_icm42607_blk_sel(reg, &blk_sel);
	if (ret)
		return ret;

	ret = inv_icm42607_mclk_get(map, &idle_set);
	if (ret)
		//Can't undo the blk select?   Add comment here.
		return ret;

	return 0;
}

static int inv_icm42607_switch_from_mreg()
{
	int ret;

	/* Restore direct access - Say why ordering */
	ret = regmap_write(map, INV_ICM42607_REG_BLK_SEL_R, 0);
	if (ret) {
		inv_icm42607_mclk_put(map, idle_set);
		return ret;
	}
	return inv_icm42607_mclk_put(map, idle_set);
}

Even with this, split the inv_icm42607_mreg_read()
ending into a good path and error path to avoid the ret2 type
dance.



>  
> +static int inv_icm42607_mreg_write(struct regmap *map, unsigned int reg,
> +				   const u8 *data, size_t count)
> +{
> +	int ret, ret2, ret3;
> +	bool idle_set;
> +	u8 blk_sel;
> +
> +	/* MREG access is one byte per transaction, no burst support. */
> +	if (count != 1)
> +		return -EINVAL;
> +
> +	ret = inv_icm42607_blk_sel(reg, &blk_sel);
> +	if (ret)
> +		return ret;
> +
> +	ret = inv_icm42607_mclk_get(map, &idle_set);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(map, INV_ICM42607_REG_BLK_SEL_W, blk_sel);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_write(map, INV_ICM42607_REG_MADDR_W,
> +			   FIELD_GET(INV_ICM42607_REG_ADDR_MASK, reg));
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_write(map, INV_ICM42607_REG_M_W, *data);
> +	fsleep(INV_ICM42607_MREG_ACCESS_DELAY_US);
> +
> +out:
> +	/* Restore direct access. */
> +	ret2 = regmap_write(map, INV_ICM42607_REG_BLK_SEL_W, 0);
> +	ret3 = inv_icm42607_mclk_put(map, idle_set);
> +
> +	return ret ?: ret2 ?: ret3;

As above.

> +}
> +