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

[Andy Shevchenko]

On Fri, Oct 02, 2026 at 01:54:29PM +0200, Kanak Shilledar 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.

...

> +	unsigned int val;
> +	int ret, ret2;
> +
> +	*idle_set = false;
> +
> +	ret = regmap_read(map, INV_ICM42607_REG_MCLK_RDY, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val & INV_ICM42607_MCLK_RDY_BIT)
> +		return 0;

Why not regmap_test_bits()?

> +	/*
> +	 * Clock isn't running: we're either in Sleep mode or in Accel LP mode
> +	 * running on WUOSC. Force the RC oscillator on via IDLE and wait for
> +	 * MCLK_RDY. Datasheet gives 10us (accel startup) to 200us (accel
> +	 * transition from OFF) for this to complete.
> +	 */
> +	ret = regmap_set_bits(map, INV_ICM42607_REG_PWR_MGMT0,
> +			      INV_ICM42607_PWR_MGMT0_IDLE);
> +	if (ret)
> +		return ret;
> +
> +	*idle_set = true;
> +
> +	ret = regmap_read_poll_timeout(map, INV_ICM42607_REG_MCLK_RDY, val,
> +				       val & INV_ICM42607_MCLK_RDY_BIT, 10, 200);
> +	if (ret) {
> +		ret2 = regmap_clear_bits(map, INV_ICM42607_REG_PWR_MGMT0,
> +					 INV_ICM42607_PWR_MGMT0_IDLE);
> +		if (ret2)
> +			dev_err(regmap_get_device(map),
> +					"failed to clear IDLE after MCLK timeout: %d\n", ret2);

Broken indentation.
Is it really important message?

> +		else
> +			*idle_set = false;
>  	}

...

> +static void inv_icm42607_mclk_put(struct regmap *map, bool idle_set)
>  {
> +	if (idle_set)
> +		regmap_clear_bits(map, INV_ICM42607_REG_PWR_MGMT0,
> +				  INV_ICM42607_PWR_MGMT0_IDLE);
> +}

What about

	if (!idle_set)
		return;

	regmap_clear_bits(map, INV_ICM42607_REG_PWR_MGMT0, INV_ICM42607_PWR_MGMT0_IDLE);

?

...

> +static int inv_icm42607_mreg_read(struct regmap *map, unsigned int reg,
> +				  u8 *data, size_t count)
> +{
> +	unsigned int val;
> +	bool idle_set;
> +	u8 blk_sel;
> +	int ret;
> +
> +	/* 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_R, blk_sel);
> +	if (ret)
> +		goto out;

So, can we use regmap ranges instead?

> +	ret = regmap_write(map, INV_ICM42607_REG_MADDR_R,
> +			   FIELD_GET(INV_ICM42607_REG_ADDR_MASK, reg));
> +	if (ret)
> +		goto out;
> +
> +	fsleep(INV_ICM42607_MREG_ACCESS_DELAY_US);
> +
> +	ret = regmap_read(map, INV_ICM42607_REG_M_R, &val);
> +	if (ret)
> +		goto out;
> +
> +	fsleep(INV_ICM42607_MREG_ACCESS_DELAY_US);
> +
> +	*data = val;
> +out:
> +	/* Restore direct access. */
> +	ret = regmap_write(map, INV_ICM42607_REG_BLK_SEL_R, 0);
> +	inv_icm42607_mclk_put(map, idle_set);
> +
> +	return ret;
>  }

...

> +static const struct regmap_config inv_icm42607_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,

No cache? Why?

> +};

...

> +static const struct regmap_config inv_icm42607_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};

Ditto. And why they can't be deduplicated?

-- 
With Best Regards,
Andy Shevchenko