Re: [PATCH v5 6/6] iio: imu: inv_icm42607: Add accelerometer calibbias support

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

On 10/02, Kanak Shilledar wrote:
> Expose IIO_CHAN_INFO_CALIBBIAS on the accelerometer channels. The
> registers are stored in MREG1. The calibration bias is written to
> OFFSET_USER4 to OFFSET_USER8 registers in MREG1. Reject the out of
> limit calibbias values instead of clamping it.
> 
> Note: The accelerometer functionality is tested with Invensense,
> ICM42370-P development board.
Good to know, but this should probably go below the '---'. Not within the commit
message.

> 
> Datasheet: https://www.invensense.tdk.com/en-us/products/3-axis/icm-42370-p
> Datasheet: https://www.lcsc.com/product-detail/C5129967.html
> Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com>
> ---
Here

>  drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c | 194 +++++++++++++++++++++-
>  1 file changed, 192 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> index 9a3ace3e7fcf9..98cef0058b64c 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> @@ -13,6 +13,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/types.h>
> +#include <linux/units.h>
>  
...
> +	/* 12 bits signed value */
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +	case IIO_MOD_Z:
> +		offset = sign_extend32(((lo_val & 0xF0) << 4) | hi_val, 11);
> +		break;
> +	case IIO_MOD_Y:
> +		offset = sign_extend32(((hi_val & 0x0F) << 8) | lo_val, 11);
Can the mask and shifting be done with some combination of FIELD_PREP/_GET/_MODIFY?
See comment below.

...
> +static int inv_icm42607_accel_write_offset(struct iio_dev *indio_dev,
> +					   struct iio_chan_spec const *chan,
> +					   int val, int val2)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	s32 min, max;
> +	s16 offset;
> +	s64 val64;
> +	int ret;
> +
> +	if (chan->type != IIO_ACCEL)
> +		return -EINVAL;
> +
> +	/* inv_icm42607_accel_calibbias: min - step - max in micro */
> +	min = inv_icm42607_accel_calibbias[0] * (long)MEGA -
> +	      inv_icm42607_accel_calibbias[1];
> +	max = inv_icm42607_accel_calibbias[4] * (long)MEGA +
> +	      inv_icm42607_accel_calibbias[5];
> +
> +	val64 = val * (s64)MEGA;
> +	if (val >= 0)
> +		val64 += val2;
> +	else
> +		val64 -= val2;
> +
> +	if (val64 < min || val64 > max)
> +		return -EINVAL;
> +
> +	/*
> +	 * Convert m/s² to g then to raw value
> +	 * m/s² to g: 1 / 9.806650
> +	 * g to raw 12 bits signed, step 0.5mg: 10000 / 5
> +	 * val in micro (1000000)
> +	 * val * 10000 / (9.806650 * 1000000 * 5)
> +	 */
> +	val64 *= 10000LL;
> +
> +	/* For rounding, add + or - divisor (9806650 * 5) divided by 2 */
> +	if (val64 >= 0)
> +		val64 += 9806650 * 5 / 2;
> +	else
> +		val64 -= 9806650 * 5 / 2;
> +	offset = div_s64(val64, 9806650 * 5);
> +
> +	/* Value is limited to 12 bits signed, return -EINVAL if out of range */
> +	if (offset < -2048 || offset > 2047)
> +		return -EINVAL;
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +		/* OFFSET_USER4 upper nibble is shared. */
> +		ret = regmap_update_bits(st->map, INV_ICM42607_REG_OFFSET_USER4,
> +					 GENMASK(7, 4), (offset & 0xF00) >> 4);
> +		if (ret)
> +			return ret;
> +
> +		return regmap_write(st->map, INV_ICM42607_REG_OFFSET_USER5,
> +				    offset & 0xFF);
> +	case IIO_MOD_Y:
> +		/* OFFSET_USER7 lower nibble is shared. */
> +		ret = regmap_update_bits(st->map, INV_ICM42607_REG_OFFSET_USER7,
> +					 GENMASK(3, 0), (offset & 0xF00) >> 8);
> +		if (ret)
> +			return ret;
> +
> +		return regmap_write(st->map, INV_ICM42607_REG_OFFSET_USER6,
> +				    offset & 0xFF);
> +	case IIO_MOD_Z:
> +		/* OFFSET_USER7 upper nibble is shared. */
> +		ret = regmap_update_bits(st->map, INV_ICM42607_REG_OFFSET_USER7,
> +					 GENMASK(7, 4), (offset & 0xF00) >> 4);
I see the register map for this chip is a bit odd. The above bit masking and
shifting can be easy to write once one is familiar with the part being
supported. Though, this really looks like one of the cases where we can declare
a bitmask and use FIELD_PREP (or even FIELD_MODIFY). Maybe something like
INV_ICM42607_REG_OFFSET_HIGH	GENMASK(7, 4)
INV_ICM42607_REG_OFFSET_LOW	GENMASK(3, 0)

INV_ICM42607_ACCEL_OFFSET_HB	GENMASK(15, 8)
INV_ICM42607_ACCEL_OFFSET_LB	GENMASK(7, 0)

reg_val = FIELD_PREP(INV_ICM42607_REG_OFFSET_HIGH,
		     FIELD_GET(INV_ICM42607_ACCEL_OFFSET_HB, offset));

Please, also take this suggestion to the other similar mask and shifting above.

> +		if (ret)
> +			return ret;
> +
> +		return regmap_write(st->map, INV_ICM42607_REG_OFFSET_USER8,
> +				    offset & 0xFF);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
...
> @@ -266,6 +454,8 @@ static int inv_icm42607_accel_write_raw_get_fmt(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT_PLUS_NANO;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_CALIBBIAS:
Minor neat, if we move the line above up one line we can drop the line below.
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
>  	}