Re: [PATCH V12 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Double application of FIELD_PREP on INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS.
- [Low] Wake-on-Motion threshold registers are omitted from regmap read/write permission access checks.
--

commit db0995d0ef167d77bbd7b68753044eb2b93adf1c
Author: Chris Morgan <macromorgan@hotmail.com>

iio: imu: inv_icm42607: Add inv_icm42607 Core Driver

Add the core component of a new inv_icm42607 driver. This includes
a few setup functions and the full register definition in the
header file, as well as the bits necessary to compile and probe the
device when used on an i2c bus.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 0000000000000..5d40f1ee53d6d
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -0,0 +1,285 @@

[ ... ]

> +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:
> +		return true;
> +	}
> +
> +	return false;
> +}

[Severity: Low]
Are the Wake-on-Motion threshold registers (0x4b - 0x4d) missing from this
allowed list in inv_icm42607_is_readable_reg()?

Registers like INV_ICM42607_REG_ACCEL_WOM_X_THR are defined in the header,
but since they are omitted here, any future attempt to read them will be
rejected by the regmap core with an -EIO error.

> +
> +static bool inv_icm42607_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case INV_ICM42607_REG_DEVICE_CONFIG ... INV_ICM42607_REG_INT_CONFIG:
> +	case INV_ICM42607_REG_PWR_MGMT0 ... INV_ICM42607_REG_INT_SOURCE4:
> +	case INV_ICM42607_REG_INTF_CONFIG0 ... INV_ICM42607_REG_INTF_CONFIG1:
> +		return true;
> +	}
> +
> +	return false;
> +}

[Severity: Low]
Similarly, are the Wake-on-Motion threshold registers missing from the switch
statement in inv_icm42607_is_writeable_reg()?

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c
> index 0000000000000..a1484f32c5d94
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_i2c.c
> @@ -0,0 +1,97 @@

[ ... ]

> +static int inv_icm42607_i2c_bus_setup(struct inv_icm42607_state *st)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_clear_bits(st->map, INV_ICM42607_REG_INTF_CONFIG1,
> +				INV_ICM42607_INTF_CONFIG1_I3C_DDR_EN |
> +				INV_ICM42607_INTF_CONFIG1_I3C_SDR_EN);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(INV_ICM42607_DRIVE_CONFIG2_I2C_MASK,
> +			 INV_ICM42607_SLEW_RATE_12_36NS);
> +	ret = regmap_update_bits(st->map, INV_ICM42607_REG_DRIVE_CONFIG2,
> +				 INV_ICM42607_DRIVE_CONFIG2_I2C_MASK, val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK,
> +			 INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS);

[Severity: Low]
Is this double-application of FIELD_PREP intentional?

The macro INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS is already defined
using FIELD_PREP in inv_icm42607.h:

#define INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS \
    FIELD_PREP(INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK, 2)

Passing it as the value argument to another FIELD_PREP in
inv_icm42607_i2c_bus_setup() is conceptually incorrect. It only evaluates
correctly here by coincidence because the bitmask GENMASK(1, 0) has a shift
of 0.

> +	return regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
> +				  INV_ICM42607_INTF_CONFIG0_UI_SIFS_CFG_MASK,
> +				  val);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611202607.85376-1-macroalpha82@gmail.com?part=3