Re: [PATCH V3 6/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[David Lechner <dlechner@baylibre.com>]

On 4/28/26 9:01 AM, Chris Morgan wrote:
> On Fri, Apr 10, 2026 at 05:59:05PM -0500, David Lechner wrote:
>> On 3/30/26 2:58 PM, Chris Morgan wrote:
>>> From: Chris Morgan <macromorgan@hotmail.com>
>>>
>>> Add icm42607 accelerometer sensor for icm42607.
>>>
>>

...

>>
>> Usually we make these 2-D arrays for readability and then cast to int * if needed.
>>
>>> +static const int inv_icm42607_accel_scale[] = {
>>> +	/* +/- 16G => 0.004788403 m/s-2 */
>>> +	[2 * INV_ICM42607_ACCEL_FS_16G] = 0,
>>> +	[2 * INV_ICM42607_ACCEL_FS_16G + 1] = 4788403,
>>> +	/* +/- 8G => 0.002394202 m/s-2 */
>>> +	[2 * INV_ICM42607_ACCEL_FS_8G] = 0,
>>> +	[2 * INV_ICM42607_ACCEL_FS_8G + 1] = 2394202,
>>> +	/* +/- 4G => 0.001197101 m/s-2 */
>>> +	[2 * INV_ICM42607_ACCEL_FS_4G] = 0,
>>> +	[2 * INV_ICM42607_ACCEL_FS_4G + 1] = 1197101,
>>> +	/* +/- 2G => 0.000598550 m/s-2 */
>>> +	[2 * INV_ICM42607_ACCEL_FS_2G] = 0,
>>> +	[2 * INV_ICM42607_ACCEL_FS_2G + 1] = 598550,
>>> +};
>>> +
> 
> I've gone through and implemented all of the changes everyone suggested, though
> this is one of the few on which I had a question. Obviously this driver was
> cobbled together from 2 different sources and checked to the best of my ability
> and tested/validated against the data sheet, but there are a few bits I'm not
> fully clear on such as this.
> 
> What's the correct way to represent this data? Since it looks like one of the
> values is always 0, should I just assume it's always 0 and only represent the
> values that change in this scale?
> 

Picking a random driver, bma220 has the most usual way of doing it.

static const int bma220_scale_table[][2] = {
	{ 0, 623000 }, { 1, 248000 }, { 2, 491000 }, { 4, 983000 },
};

I do like using the enum to make it self documenting though (no
comments needed), so for this driver...

static const int inv_icm42607_accel_scale_nano[][2] = {
	[INV_ICM42607_ACCEL_FS_16G] = { 0, 4788403 },
	...
};


Then the read_avail callback in bma220 does:

	case IIO_CHAN_INFO_SCALE:
		*vals = (int *)bma220_scale_table;
		*type = IIO_VAL_INT_PLUS_MICRO;
		*length = ARRAY_SIZE(bma220_scale_table) * 2;
		return IIO_AVAIL_LIST;

Although the cast would be better as `(const int *)` and I assume
you would want to use NANO instead of MICRO.



>>
>>> +int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
>>> +				struct inv_icm42607_sensor_conf *conf,
>>> +				unsigned int *sleep_ms)
>>> +{
>>> +	struct inv_icm42607_sensor_conf *oldconf = &st->conf.accel;
>>> +	unsigned int val;
>>> +	int ret;
>>> +
>>> +	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;
>>> +
>>> +	if (conf->fs != oldconf->fs || conf->odr != oldconf->odr) {
>>
>> We could use the regmap cache feature to avoid having to manual keep
>> track of old values. Or just always write the same values anyway. I
>> find that is nice when debugging hardware with a logic analyzer. Unless
>> there is some measureable performance improvlment here?
> 
> This is another one I had a question on. I'm not entirely clear from the
> datasheet which reg values are volatile and which ones are safe to cache.
> Performance wise the 42607 series appears to be the *least* performant
> in their lineup, so I don't imagine we care much either way. Should I just
> not worry about the old values and always write? Do you think that would
> work?
> 
My personal preference is to always do extra SPI writes for anything
that is not performance critical. This makes debugging with a logic
analyzer much easier.

In cases where using caching makes sense, generally, "status" registers
are volatile because the chip can change the value when status changes.
"config" registers are not volatile because they don't change unless
written to.