Tdk invensense icm42x70 support 3axis #83498

afontaine-invn · 2025-01-02T09:49:15Z

The icm42370-p is a 3-axis MEMS accelerometer.
https://invensense.tdk.com/products/motion-tracking/3-axis/icm-42370-p/
To support icm42370-p sensor, we renamed icm42670 to icm42x70 as the driver is similar to ICM-42670-P/S in hal_tdk module.
Add icm42x70 device tree file, it includes the common properties for icm42370-p, icm42670-p, and icm42670-s. Add icm42370-p device tree files for SPI and I2C interface.

Signed-off-by: Aurelie Fontaine [email protected]

zephyrbot · 2025-01-02T09:50:12Z

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_tdk	zephyrproject-rtos/hal_tdk@`e0ade95`	zephyrproject-rtos/hal_tdk@`29fcd7a`	zephyrproject-rtos/[email protected]

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

afontaine-invn · 2025-01-07T11:05:01Z

FYI @kartben , as you suggested in a previous closed PR#69781 we proposed to support both icm-42670 6-axis and icm-42370 3-axis in the same driver.

kartben

Great work!
Haven't had a deep look from an actual driver perspective but my initial comment would be that you need to please describe in the Zephyr 4.1 migration guide the changes users of the existing driver/compatible will have to make to their codebase since things will "break" for them due to some of the refactoring -- it looks like it might be pretty minimal (so kudos on that!) but you should e.g. describe things like the fact that the public header for sensor custom attribute has been renamed include/zephyr/drivers/sensor/icm42670.h → include/zephyr/drivers/sensor/icm42x70.h ; things like that.

modules/hal_tdk/Kconfig

my comments have been addressed - over to sensor maintainers and collaborators for more in-depth review :)

afontaine-invn · 2025-01-27T10:41:03Z

Hi @MaureenHelm ,
Could you have a look at this pull request?
I added this change in release note document as requested and I've got conflicts to resolve many times lately that's why I needed to repsuh several times but nothing else changed.

yperess

I have some minor nits about the way #ifdef is used where I think it would be much easier to read if we used if (IS_ENABLED(...)) instead, but this PR is migrating existing code that's already been reviewed so I wouldn't require any cleanup.

yperess · 2025-01-27T16:45:19Z

drivers/sensor/tdk/icm42x70/icm42x70.c

@@ -377,7 +381,8 @@ static int icm42670_accel_config(struct icm42670_data *drv_data, enum sensor_att
 	return 0;
 }

-static int icm42670_set_gyro_odr(struct icm42670_data *drv_data, const struct sensor_value *val)
+#if CONFIG_USE_EMD_ICM42670


Suggestion, to make the code more readable add these functions to icm42670.h and include it from icm42x70.h, then move the definitions to an icm42670.c file and guard the inclusion of it in the cmake file. I generally find that it's easier to read the code that way.

Thanks for this suggestion. Indeed, it lightweight the icm42x70.c file, it's easy to read. And icm42670 dedicated code is identified. I did the change.

yperess · 2025-01-27T16:49:47Z

drivers/sensor/tdk/icm42x70/icm42x70.c

 	    (chan == SENSOR_CHAN_ACCEL_X) || (chan == SENSOR_CHAN_ACCEL_Y) ||
-	    (chan == SENSOR_CHAN_ACCEL_Z) || (chan == SENSOR_CHAN_GYRO_XYZ) ||
-	    (chan == SENSOR_CHAN_GYRO_X) || (chan == SENSOR_CHAN_GYRO_Y) ||
-	    (chan == SENSOR_CHAN_GYRO_Z) || (chan == SENSOR_CHAN_DIE_TEMP)) {
-#ifdef CONFIG_ICM42670_TRIGGER
-		status = icm42670_fetch_from_fifo(dev);
+	    (chan == SENSOR_CHAN_ACCEL_Z)


nit: prefer sensor.h's SENSOR_CHANNEL_IS_ACCEL(chan) and SENSOR_CHANNEL_IS_GYRO(chan) for readability.

Thanks for this suggestion also, it's more elegant. I did the change

The icm42370-p is a 3-axis MEMS accelermeter. https://invensense.tdk.com/products/motion-tracking/3-axis/icm-42370-p/ Signed-off-by: Aurelie Fontaine <[email protected]>

To support icm42370-p sensor, we renamed icm42670 to icm42x70 as the driver is similar to ICM-42670-P/S in hal_tdk module. Keep icm42670 source and header for dedicated code. Signed-off-by: Aurelie Fontaine <[email protected]>

Add icm42x70 device tree file, it includes the common properties for icm42370-p, icm42670-p, and icm42670-s. Add icm42370-p device tree files for SPI and I2C interface. Signed-off-by: Aurelie Fontaine <[email protected]>

Rename ICM42670 define to ICM42X70 to be aligned with updated driver to support icm42370-p Signed-off-by: Aurelie Fontaine <[email protected]>

MaureenHelm

Most of the changes look fine, but the #if CONFIG_USE_EMD_ICM42670 parts need to be reworked to check on a per-device basis.

MaureenHelm · 2025-01-31T14:52:43Z

drivers/sensor/tdk/icm42x70/icm42x70.c

@@ -490,6 +397,7 @@ static int icm42670_turn_on_sensor(const struct device *dev)
 	}
 	LOG_DBG("Set accel full scale to: %d G", data->accel_fs);

+#if CONFIG_USE_EMD_ICM42670


This won't work if there is both an icm42670 and an icm42370 in the system. You need to check if this device is an icm42670, not if any device is an icm42670.

zephyrbot added area: Sensors Sensors area: Samples Samples labels Jan 2, 2025

zephyrbot requested review from asemjonovs, avisconti, jeppenodgaard, kartben, MaureenHelm, nashif, teburd, tristan-google, ubieda and yperess January 2, 2025 09:50

zephyrbot assigned MaureenHelm Jan 2, 2025

zephyrbot added manifest manifest-hal_tdk DNM This PR should not be merged (Do Not Merge) labels Jan 2, 2025

afontaine-invn force-pushed the TDK_Invensense_icm42x70_support_3axis branch 4 times, most recently from 3843b70 to a9178fe Compare January 3, 2025 14:48

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Jan 3, 2025

kartben previously requested changes Jan 10, 2025

View reviewed changes

pdgendt reviewed Jan 10, 2025

View reviewed changes

modules/hal_tdk/Kconfig Show resolved Hide resolved

afontaine-invn force-pushed the TDK_Invensense_icm42x70_support_3axis branch from a9178fe to 36e1d18 Compare January 10, 2025 14:06

zephyrbot added the Release Notes To be mentioned in the release notes label Jan 10, 2025

zephyrbot requested a review from fabiobaltieri January 10, 2025 14:07

afontaine-invn requested a review from kartben January 10, 2025 14:10

afontaine-invn force-pushed the TDK_Invensense_icm42x70_support_3axis branch from 36e1d18 to dc6fc86 Compare January 10, 2025 14:17

afontaine-invn force-pushed the TDK_Invensense_icm42x70_support_3axis branch 3 times, most recently from 59d055c to 007341c Compare January 20, 2025 09:23

afontaine-invn force-pushed the TDK_Invensense_icm42x70_support_3axis branch 3 times, most recently from f6d46c4 to 76ec1d7 Compare January 27, 2025 09:05

yperess previously approved these changes Jan 27, 2025

View reviewed changes

afontaine-invn dismissed yperess’s stale review via 50b1368 January 31, 2025 10:35

afontaine-invn force-pushed the TDK_Invensense_icm42x70_support_3axis branch 3 times, most recently from a0367cd to 6651aab Compare January 31, 2025 11:08

afontaine-invn added 6 commits January 31, 2025 14:13

modules: hal_tdk: Add icm42370p support

72a55bb

The icm42370-p is a 3-axis MEMS accelermeter. https://invensense.tdk.com/products/motion-tracking/3-axis/icm-42370-p/ Signed-off-by: Aurelie Fontaine <[email protected]>

drivers: sensor: icm42670: add icm42370-P support

2c3b690

To support icm42370-p sensor, we renamed icm42670 to icm42x70 as the driver is similar to ICM-42670-P/S in hal_tdk module. Keep icm42670 source and header for dedicated code. Signed-off-by: Aurelie Fontaine <[email protected]>

dts: bindings: sensor: add icm42370-p support

2a41113

Add icm42x70 device tree file, it includes the common properties for icm42370-p, icm42670-p, and icm42670-s. Add icm42370-p device tree files for SPI and I2C interface. Signed-off-by: Aurelie Fontaine <[email protected]>

samples: sensor: 6dof_motion_drdy: update naming

a97a2ab

Rename ICM42670 define to ICM42X70 to be aligned with updated driver to support icm42370-p Signed-off-by: Aurelie Fontaine <[email protected]>

samples: sensor: tdk_apex: update naming

4ff9521

Rename ICM42670 define to ICM42X70 to be aligned with updated driver to support icm42370-p Signed-off-by: Aurelie Fontaine <[email protected]>

tests: drivers: sensor: update naming

4fcdb7e

Rename ICM42670 define to ICM42X70 to be aligned with updated driver to support icm42370-p Signed-off-by: Aurelie Fontaine <[email protected]>

afontaine-invn force-pushed the TDK_Invensense_icm42x70_support_3axis branch from 6651aab to 4fcdb7e Compare January 31, 2025 13:15

MaureenHelm requested changes Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tdk invensense icm42x70 support 3axis #83498

Tdk invensense icm42x70 support 3axis #83498

afontaine-invn commented Jan 2, 2025

zephyrbot commented Jan 2, 2025 •

edited

Loading

afontaine-invn commented Jan 7, 2025 •

edited

Loading

kartben left a comment

afontaine-invn commented Jan 27, 2025

yperess left a comment

yperess Jan 27, 2025

afontaine-invn Jan 31, 2025

yperess Jan 27, 2025

afontaine-invn Jan 31, 2025

MaureenHelm left a comment

MaureenHelm Jan 31, 2025

Tdk invensense icm42x70 support 3axis #83498

Are you sure you want to change the base?

Tdk invensense icm42x70 support 3axis #83498

Conversation

afontaine-invn commented Jan 2, 2025

zephyrbot commented Jan 2, 2025 • edited Loading

afontaine-invn commented Jan 7, 2025 • edited Loading

kartben left a comment

Choose a reason for hiding this comment

afontaine-invn commented Jan 27, 2025

yperess left a comment

Choose a reason for hiding this comment

yperess Jan 27, 2025

Choose a reason for hiding this comment

afontaine-invn Jan 31, 2025

Choose a reason for hiding this comment

yperess Jan 27, 2025

Choose a reason for hiding this comment

afontaine-invn Jan 31, 2025

Choose a reason for hiding this comment

MaureenHelm left a comment

Choose a reason for hiding this comment

MaureenHelm Jan 31, 2025

Choose a reason for hiding this comment

zephyrbot commented Jan 2, 2025 •

edited

Loading

afontaine-invn commented Jan 7, 2025 •

edited

Loading