Tdk invensense icp101xx pressure sensor #84081

rbuisson-invn · 2025-01-16T10:03:36Z

drivers: sensor: icp101xx: supports icp101xx sensors
dts: supports icp101xx invn pressure sensor
module: HAL TDK module

Adds official TDK Invensense Inc. driver in TDK HAL module for icp101xx sensor.
Replacing icp10125 unofficial driver
Validated with custom setup: nrf52dk_nrf52832 + icp10125 Piromoni board

Signed-off-by: Remi Buisson [email protected]

zephyrbot · 2025-01-16T10:04:34Z

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@`6727477` (`main`)	zephyrproject-rtos/[email protected]

✅ All manifest checks OK

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

JarmouniA · 2025-01-16T12:25:03Z

tests/drivers/build_all/sensor/i2c.dtsi

I think the renaming should happen in the same commit where the old binding is removed, so the history can be bisected.

Fixed, thanks for your review.

JarmouniA · 2025-01-16T12:33:13Z

drivers/sensor/tdk/icp101xx/icp101xx_drv.h

This is usually placed in the main source file.

Fixed, thanks for your review.

teburd

Please consider dropping float math, unclear what the tdk hal is providing here to be frank.

drivers/sensor/tdk/icp101xx/CMakeLists.txt

modules/hal_tdk/Kconfig

teburd · 2025-02-03T14:18:07Z

drivers/sensor/tdk/icp101xx/icp101xx_drv.h

There doesn't seem to be a good reason to split this into a .h file, unless there's multiple .c files needing this it should be in the same compliation unit to keep things simple

teburd · 2025-02-03T14:21:14Z

drivers/sensor/tdk/icp101xx/icp101xx_drv.c

I'm a bit confused by this call here, you are setting up a struct on the stack, then passing it in by reference, then its unused past this point. This seems wasteful if the result is copied in struct members into device data struct?

Totally agree, this is completely useless, removed.

teburd · 2025-02-03T14:23:04Z

drivers/sensor/tdk/icp101xx/icp101xx_drv.c

Is this call out to the tdk hal really needed? Isn't this simply reading a few registers?

This create a specific command sent over I2C.
It seems better to have a dedicated function.

teburd · 2025-02-03T14:23:33Z

drivers/sensor/tdk/icp101xx/icp101xx_drv.c

Isn't this equivalent to i2c_write() with a simple register write? Why is the TDK HAL needed here?

This creates a specific command sent over I2C (depending on selected mode).
It seems better to have a dedicated function.

teburd

reasonable answers/fixes given I think to my review comments

rbuisson-invn · 2025-02-03T16:52:36Z

reasonable answers/fixes given I think to my review comments

Thanks for the review and approval !

teburd

Refresh +1

drivers/sensor/tdk/icp101xx/icp101xx_drv.c

dts/bindings/sensor/invensense,icp10125.yaml

JarmouniA · 2025-02-05T12:17:22Z

modules/hal_tdk/Kconfig

Suggested change

bool "ICP101XX Barometric Pressure and Temperature Sensor"

bool "ICP101XX Barometric Pressure and Temperature Sensors"

JarmouniA · 2025-02-05T12:17:38Z

dts/bindings/sensor/invensense,icp101xx.yaml

Suggested change

description: ICP101xx High Accuracy, Low Power, Barometric Pressure and Temperature Sensor IC

description: ICP101xx High Accuracy, Low Power, Barometric Pressure and Temperature Sensors IC

JarmouniA · 2025-02-05T12:18:54Z

drivers/sensor/tdk/icp101xx/icp101xx_drv.c

Suggested change

#error "ICP101XX driver enabled without any devices"

#error "ICP101XX driver enabled without any compatible devices in devicetree"

JarmouniA · 2025-02-05T12:21:45Z

drivers/sensor/tdk/icp101xx/Kconfig

Suggested change

Enable driver for ICP101XX barometric pressure/temperature sensor.

Enable driver for ICP101XX barometric pressure/temperature sensors.

Thanks for the suggested changes, all taken into account.
Could you please approve if ok for you?

drivers/sensor/tdk/icp101xx/CMakeLists.txt

drivers/sensor/tdk/icp101xx/icp101xx_drv.c

drivers/sensor/tdk/icp101xx/icp101xx_drv.h

icp101xx are barometric pressure and temperature sensors. https://invensense.tdk.com/smartpressure Signed-off-by: Remi Buisson <[email protected]>

Use official TDK Invensense driver for icp101xx sensor in tdk_hal module. Signed-off-by: Remi Buisson <[email protected]>

Replace icp10125. tests: sensor: rename icp10125 to icp101xx Signed-off-by: Remi Buisson <[email protected]>

github-actions · 2025-02-07T06:38:57Z

Hi @rbuisson-invn!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

zephyrbot added the area: Sensors Sensors label Jan 16, 2025

zephyrbot requested review from MaureenHelm, asemjonovs, avisconti, jeppenodgaard, teburd, tristan-google, ubieda and yperess January 16, 2025 10:04

zephyrbot assigned MaureenHelm Jan 16, 2025

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

JarmouniA reviewed Jan 16, 2025

View reviewed changes

rbuisson-invn force-pushed the TDK_Invensense_icp101xx_pressure_sensor branch 4 times, most recently from c6b4f31 to 9ed1ee8 Compare January 17, 2025 14:11

rbuisson-invn force-pushed the TDK_Invensense_icp101xx_pressure_sensor branch from 9ed1ee8 to 3d3b1fd Compare February 3, 2025 09:07

zephyrbot requested a review from afontaine-invn February 3, 2025 09:08

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

afontaine-invn approved these changes Feb 3, 2025

View reviewed changes

teburd reviewed Feb 3, 2025

View reviewed changes

rbuisson-invn force-pushed the TDK_Invensense_icp101xx_pressure_sensor branch from 3d3b1fd to f4f01b0 Compare February 3, 2025 16:19

teburd previously approved these changes Feb 3, 2025

View reviewed changes

rbuisson-invn dismissed teburd’s stale review via 227b663 February 4, 2025 07:44

rbuisson-invn force-pushed the TDK_Invensense_icp101xx_pressure_sensor branch from f4f01b0 to 227b663 Compare February 4, 2025 07:44

teburd previously approved these changes Feb 4, 2025

View reviewed changes

MaureenHelm requested changes Feb 4, 2025

View reviewed changes

drivers/sensor/tdk/icp101xx/icp101xx_drv.c Outdated Show resolved Hide resolved

drivers/sensor/tdk/icp101xx/icp101xx_drv.c Outdated Show resolved Hide resolved

dts/bindings/sensor/invensense,icp10125.yaml Outdated Show resolved Hide resolved

rbuisson-invn dismissed teburd’s stale review via 3be0ef6 February 5, 2025 09:57

rbuisson-invn force-pushed the TDK_Invensense_icp101xx_pressure_sensor branch from 227b663 to 3be0ef6 Compare February 5, 2025 09:57

JarmouniA reviewed Feb 5, 2025

View reviewed changes

rbuisson-invn force-pushed the TDK_Invensense_icp101xx_pressure_sensor branch from 3be0ef6 to aec39dd Compare February 5, 2025 16:39

yperess requested changes Feb 5, 2025

View reviewed changes

drivers/sensor/tdk/icp101xx/CMakeLists.txt Outdated Show resolved Hide resolved

drivers/sensor/tdk/icp101xx/icp101xx_drv.c Outdated Show resolved Hide resolved

drivers/sensor/tdk/icp101xx/icp101xx_drv.h Outdated Show resolved Hide resolved

rbuisson-invn added 3 commits February 6, 2025 09:45

modules: hal_tdk: Add icp101xx support

4d26ef4

icp101xx are barometric pressure and temperature sensors. https://invensense.tdk.com/smartpressure Signed-off-by: Remi Buisson <[email protected]>

drivers: sensor: icp101xx: Add icp101xx support

4756302

Use official TDK Invensense driver for icp101xx sensor in tdk_hal module. Signed-off-by: Remi Buisson <[email protected]>

dts: bindings: sensor: Add invensense icp101xx properties

744541b

Replace icp10125. tests: sensor: rename icp10125 to icp101xx Signed-off-by: Remi Buisson <[email protected]>

rbuisson-invn force-pushed the TDK_Invensense_icp101xx_pressure_sensor branch from aec39dd to 744541b Compare February 6, 2025 08:45

zephyrbot added the Release Notes To be mentioned in the release notes label Feb 6, 2025

zephyrbot requested review from fabiobaltieri and kartben February 6, 2025 08:46

MaureenHelm approved these changes Feb 6, 2025

View reviewed changes

JarmouniA approved these changes Feb 6, 2025

View reviewed changes

yperess approved these changes Feb 7, 2025

View reviewed changes

kartben merged commit ad4ed55 into zephyrproject-rtos:main Feb 7, 2025
28 checks passed

rbuisson-invn mentioned this pull request Feb 11, 2025

Add Tdk invensense icp201xx pressure sensor support #83914

Merged

	bool "ICP101XX Barometric Pressure and Temperature Sensor"
	bool "ICP101XX Barometric Pressure and Temperature Sensors"

	description: ICP101xx High Accuracy, Low Power, Barometric Pressure and Temperature Sensor IC
	description: ICP101xx High Accuracy, Low Power, Barometric Pressure and Temperature Sensors IC

	#error "ICP101XX driver enabled without any devices"
	#error "ICP101XX driver enabled without any compatible devices in devicetree"

	Enable driver for ICP101XX barometric pressure/temperature sensor.
	Enable driver for ICP101XX barometric pressure/temperature sensors.

Tdk invensense icp101xx pressure sensor #84081

Tdk invensense icp101xx pressure sensor #84081

Uh oh!

Conversation

rbuisson-invn commented Jan 16, 2025

Uh oh!

zephyrbot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teburd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teburd left a comment

Choose a reason for hiding this comment

Uh oh!

rbuisson-invn commented Feb 3, 2025

Uh oh!

teburd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

zephyrbot commented Jan 16, 2025 •

edited

Loading