Re: [PATCH v4 4/5] iio: imu: inv_icm42607: Implement MREGx register access

[Marcelo Schmitt]

On 09/17, 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.
> 
> 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
> Signed-off-by: Kanak Shilledar <kanak.shilledar@xxxxxxxx>
> ---
> LLM was used to implement the mreg back switching support.
...
>  
> -/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
> +/*
> + * Virtual register addresses: bank id in the upper byte, register address in
> + * the lower byte. Bank 0 is directly addressable on the bus. MREG banks are
> + * reached through the BLK_SEL/MADDR/M indirect access window, one byte per
> + * transaction (datasheet section 13, no burst support).
> + *
> + * MREG1 programs BLK_SEL = 0x00, which collides with the direct bank, so it
> + * gets the distinct virtual id 0x01. MREG2 and MREG3 virtual ids match their
> + * BLK_SEL values.
> + */
> +#define INV_ICM42607_REG_BANK_MASK			GENMASK(15, 8)
> +#define INV_ICM42607_REG_ADDR_MASK			GENMASK(7, 0)
> +
> +#define INV_ICM42607_BANK0				0x0000
> +#define INV_ICM42607_MREG1				0x0100
> +#define INV_ICM42607_MREG2				0x2800
> +#define INV_ICM42607_MREG3				0x5000
> +
> +/* BLK_SEL value written to the device for each virtual bank id. */
> +#define INV_ICM42607_MREG1_BLK_SEL			0x00
> +#define INV_ICM42607_MREG2_BLK_SEL			0x28
> +#define INV_ICM42607_MREG3_BLK_SEL			0x50
> +
> +/*
> + * Datasheet section 13, Accessing MREGx Registers: no bus access for 10us
> + * after programming the indirect access window.
> + */
> +#define INV_ICM42607_MREG_ACCESS_DELAY_US		10
>  
>  /* Register Map for User Bank 0 */
It would help review if either the commit message or a comment here made it
clear Bank 0 register access is now going to be handled by the same routines
used by 16-bit virtual regmap_config. Replacing 0x00 by 0x0000 looks odd
otherwise.

> -#define INV_ICM42607_REG_MCLK_RDY			0x00
> +#define INV_ICM42607_REG_MCLK_RDY			0x0000
> +#define INV_ICM42607_MCLK_RDY_BIT			BIT(3)
>  
> -#define INV_ICM42607_REG_DEVICE_CONFIG			0x01
> +#define INV_ICM42607_REG_DEVICE_CONFIG			0x0001
>  #define INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE		BIT(2)
>  #define INV_ICM42607_DEVICE_CONFIG_SPI_MODE		BIT(0)
>  
...
> -static bool inv_icm42607_is_readable_reg(struct device *dev, unsigned int reg)
> +static int inv_icm42607_mclk_get(struct regmap *map, bool *idle_set)
>  {
> -	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:
> -		return true;
> +	unsigned int val;
> +	int ret;
> +
> +	*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;
> +
> +	/*
> +	 * 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) {
> +		regmap_clear_bits(map, INV_ICM42607_REG_PWR_MGMT0,
> +				  INV_ICM42607_PWR_MGMT0_IDLE);
> +		*idle_set = false;
To fully handle the error path, it should also check regmap_clear_bits() return.
E.g.
		ret2 = regmap_clear_bits(map, INV_ICM42607_REG_PWR_MGMT0,
					 INV_ICM42607_PWR_MGMT0_IDLE);
		if (ret2)
			dev_err(...);
		else
			*idle_set = false;

>  	}
>  
> -	return false;
> +	return ret;
>  }
>