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

[Kanak Shilledar <Kanak.Shilledar@axis.com>]

[-- Attachment #1: Type: text/plain, Size: 5541 bytes --]

Hi Andy,

On Fri, 2026-10-02 at 16:12 +0300, Andy Shevchenko wrote:
> 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()?

I will fix it.

> > +	/*
> > +	 * 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?

It is useful for indicating failure in MCLK, but I will lower the
priority to either _info or _debug and fix the indentation.

> > +		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);
> 
> ?

Will incorporate the suggestion.

> ...
> 
> > +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?

We can't use regmap ranges because the register accesses for different
banks guarded by a specific routine of writing the bank selector, the
address pointer and then finally accessing the value along with
checking for timings and clocks. There is also a limitation that,
accessing the registers in banks other than USER BANK 0 can only be
done serially. It doesn't support bulk reads. This is documented in
section 13 of the datasheet [1].

> > +	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?
As the virtual regmap config has some caching for USER BANK 0 registers
only. The indirect banks doesn't support caching [1].

> > +};
> 
> ...
> 
> > +static const struct regmap_config inv_icm42607_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +};
> 
> Ditto. And why they can't be deduplicated?

I will remove the duplication of these regmap_configs.

Thanks and Regards,
Kanak Shilledar

[1] Datasheet: https://www.lcsc.com/product-detail/C5129967.html

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]