Re: [PATCH] iio: imu: inv_icm42600: refactor accel/gyro configuration paths

[Jonathan Cameron <jic23@kernel.org>]

On Sun,  3 May 2026 00:12:36 -0300
João Fernandes <joaovictor.fernandes@usp.br> wrote:

> Refactor accel and gyro configuration paths to remove duplicated
> logic and improve code structure.
> Introduce a generic helper, inv_icm42600_set_sensor_conf(), to handle
> common register programming and state updates.
> 
> Keep sensor-specific handling in inv_icm42600_set_accel_conf() and
> inv_icm42600_set_gyro_conf() wrappers. Also extract configuration
> sanitization into a dedicated helper inv_icm42600_sanitize_conf()
> to further reduce repetition.
> 
> Signed-off-by: João Fernandes <joaovictor.fernandes@usp.br>
> Co-developed-by: Otavio Capobianco <gcotavio@usp.br>
> Signed-off-by: Otavio Capobianco <gcotavio@usp.br>
> ---
>  .../iio/imu/inv_icm42600/inv_icm42600_core.c  | 125 +++++++++---------

When you get a patch to 'remove duplicated' code and it adds 1 line,
then start questioning if it is a good idea.  I think on balance, not
quite worth it here. Some feedback inline.

I haven't broken it out but maybe the sanitize section is a sensible
small deduplication patch?

Comments mostly about too much complexity.

Thanks,

Jonathan



>  1 file changed, 63 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 76eb22488..e326e0aae 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -289,15 +289,51 @@ static int inv_icm42600_set_pwr_mgmt0(struct inv_icm42600_state *st,
>  	return 0;
>  }
>  
> -int inv_icm42600_set_accel_conf(struct inv_icm42600_state *st,
> -				struct inv_icm42600_sensor_conf *conf,
> -				unsigned int *sleep_ms)
> +static int inv_icm42600_set_sensor_conf(struct inv_icm42600_state *st,
> +			      struct inv_icm42600_sensor_conf *target_conf,
> +			      struct inv_icm42600_sensor_conf *accel_conf,
> +			      struct inv_icm42600_sensor_conf *gyro_conf,
> +			      struct inv_icm42600_sensor_conf *oldconf,

It's a bit odd to have 3 with _conf and one with no _
Maybe old_config?  Given you are updating it, is it really old?
Maybe just call it conf?


> +			      unsigned int reg_config0,

The number of parameters is what got me first wondering if this was
worth doing.  To me it feels of marginal benefit.

> +			      unsigned int target_config0_val,
> +			      unsigned int *sleep_ms)
>  {
> -	struct inv_icm42600_sensor_conf *oldconf = &st->conf.accel;
>  	unsigned int val;
>  	int ret;
>  
> -	/* Sanitize missing values with current values */
> +	/* set target sensor's CONFIG0 register */
> +	if (target_conf->fs != oldconf->fs || target_conf->odr != oldconf->odr) {
> +		ret = regmap_write(st->map, reg_config0, target_config0_val);
> +		if (ret)
> +			return ret;
> +
> +		oldconf->fs = target_conf->fs;
> +		oldconf->odr = target_conf->odr;
> +	}
> +
> +	/* set GYRO_ACCEL_CONFIG0 register */

Code makes that obvious. So I'd drop the comment.

> +	if (target_conf->filter != oldconf->filter) {
> +		val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(accel_conf->filter) |
> +		      INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(gyro_conf->filter);
> +
> +		ret = regmap_write(st->map,
> +				   INV_ICM42600_REG_GYRO_ACCEL_CONFIG0,
> +				   val);
> +		if (ret)
> +			return ret;
> +
> +		oldconf->filter = target_conf->filter;
> +	}
> +
> +	/* set PWR_MGMT0 register */
> +	return inv_icm42600_set_pwr_mgmt0(st, gyro_conf->mode, accel_conf->mode,
> +					  st->conf.temp_en, sleep_ms);
> +}
> +
> +static void inv_icm42600_sanitize_conf(
> +	struct inv_icm42600_sensor_conf *conf,
> +	const struct inv_icm42600_sensor_conf *oldconf)

Maybe go a bit long on this one and use this style:
static void inv_icm42600_sanitize_conf(struct inv_icm42600_sensor_conf *conf,
				       const struct inv_icm42600_sensor_conf *oldconf)

> +{
>  	if (conf->mode < 0)
>  		conf->mode = oldconf->mode;
>  	if (conf->fs < 0)
> @@ -306,6 +342,17 @@ int inv_icm42600_set_accel_conf(struct inv_icm42600_state *st,
>  		conf->odr = oldconf->odr;
>  	if (conf->filter < 0)
>  		conf->filter = oldconf->filter;
> +}
> +
> +int inv_icm42600_set_accel_conf(struct inv_icm42600_state *st,
> +				struct inv_icm42600_sensor_conf *conf,
> +				unsigned int *sleep_ms)
> +{
> +	struct inv_icm42600_sensor_conf *oldconf = &st->conf.accel;
> +	unsigned int config0_val;
> +
> +	/* Sanitize missing values with current values */
> +	inv_icm42600_sanitize_conf(conf, oldconf);
>  
>  	/* force power mode against ODR when sensor is on */
>  	switch (conf->mode) {
> @@ -324,30 +371,12 @@ int inv_icm42600_set_accel_conf(struct inv_icm42600_state *st,
>  		break;
>  	}
>  
> -	/* set ACCEL_CONFIG0 register (accel fullscale & odr) */
> -	if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> -		val = INV_ICM42600_ACCEL_CONFIG0_FS(conf->fs) |
> +	config0_val = INV_ICM42600_ACCEL_CONFIG0_FS(conf->fs) |
>  		      INV_ICM42600_ACCEL_CONFIG0_ODR(conf->odr);
> -		ret = regmap_write(st->map, INV_ICM42600_REG_ACCEL_CONFIG0, val);
> -		if (ret)
> -			return ret;
> -		oldconf->fs = conf->fs;
> -		oldconf->odr = conf->odr;
> -	}
> -
> -	/* set GYRO_ACCEL_CONFIG0 register (accel filter) */
> -	if (conf->filter != oldconf->filter) {
> -		val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(conf->filter) |
> -		      INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(st->conf.gyro.filter);
> -		ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_ACCEL_CONFIG0, val);
> -		if (ret)
> -			return ret;
> -		oldconf->filter = conf->filter;
> -	}
>  
> -	/* set PWR_MGMT0 register (accel sensor mode) */
> -	return inv_icm42600_set_pwr_mgmt0(st, st->conf.gyro.mode, conf->mode,
> -					  st->conf.temp_en, sleep_ms);
> +	return inv_icm42600_set_sensor_conf(st, conf, conf, &st->conf.gyro, oldconf,
> +					    INV_ICM42600_REG_ACCEL_CONFIG0,
> +					    config0_val, sleep_ms);
>  }
>  
>  int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
> @@ -355,45 +384,17 @@ int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
>  			       unsigned int *sleep_ms)
>  {
>  	struct inv_icm42600_sensor_conf *oldconf = &st->conf.gyro;
> -	unsigned int val;
> -	int ret;
> +	unsigned int config0_val;
>  
> -	/* sanitize missing values with current values */
> -	if (conf->mode < 0)
> -		conf->mode = oldconf->mode;
> -	if (conf->fs < 0)
> -		conf->fs = oldconf->fs;
> -	if (conf->odr < 0)
> -		conf->odr = oldconf->odr;
> -	if (conf->filter < 0)
> -		conf->filter = oldconf->filter;
> +	/* Sanitize missing values with current values */
> +	inv_icm42600_sanitize_conf(conf, oldconf);
>  
> -	/* set GYRO_CONFIG0 register (gyro fullscale & odr) */
> -	if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
> -		val = INV_ICM42600_GYRO_CONFIG0_FS(conf->fs) |
> +	config0_val = INV_ICM42600_GYRO_CONFIG0_FS(conf->fs) |
>  		      INV_ICM42600_GYRO_CONFIG0_ODR(conf->odr);
> -		ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_CONFIG0, val);
> -		if (ret)
> -			return ret;
> -		oldconf->fs = conf->fs;
> -		oldconf->odr = conf->odr;
> -	}
>  
> -	/* set GYRO_ACCEL_CONFIG0 register (gyro filter) */
> -	if (conf->filter != oldconf->filter) {
> -		val = INV_ICM42600_GYRO_ACCEL_CONFIG0_ACCEL_FILT(st->conf.accel.filter) |
> -		      INV_ICM42600_GYRO_ACCEL_CONFIG0_GYRO_FILT(conf->filter);
> -		ret = regmap_write(st->map, INV_ICM42600_REG_GYRO_ACCEL_CONFIG0, val);
> -		if (ret)
> -			return ret;
> -		oldconf->filter = conf->filter;
> -	}
> -
> -	/* set PWR_MGMT0 register (gyro sensor mode) */
> -	return inv_icm42600_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode,
> -					  st->conf.temp_en, sleep_ms);
> -
> -	return 0;
> +	return inv_icm42600_set_sensor_conf(st, conf, &st->conf.accel, conf, oldconf,
> +					    INV_ICM42600_REG_GYRO_CONFIG0,
> +					    config0_val, sleep_ms);
>  }
>  
>  int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,