Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: sensor: temperature: Add i.MX RT die temperature sensor #83880

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

anedergaard
Copy link
Contributor

  • Added dts binding for die temperature sensor for i.MX RT117X and i.MX RT118X
  • Added driver for die temperature readings on i.MX RT117X and i.MX RT118X
  • Added overlay file for RT1170-EVK in temp_sensor test for validation

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 13, 2025

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

Name Old Revision New Revision Diff
hal_nxp zephyrproject-rtos/hal_nxp@b4e6f88 zephyrproject-rtos/hal_nxp#494 zephyrproject-rtos/hal_nxp#494/files

DNM label due to: 1 project with PR revision

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

@zephyrbot zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Jan 13, 2025
@anedergaard anedergaard force-pushed the driver-tmpsns-imxrt11xx branch from 353c9e1 to ab9fb98 Compare January 13, 2025 08:24
Copy link
Collaborator

@jeppenodgaard jeppenodgaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality LGTM.
I think some naming alignment is needed. There's tmpsns, temp, temperature.

drivers/sensor/nxp/nxp_imxrt_temp/temp_imxrt.c Outdated Show resolved Hide resolved
drivers/sensor/nxp/nxp_imxrt_temp/temp_imxrt.c Outdated Show resolved Hide resolved
drivers/sensor/nxp/nxp_imxrt_temp/temp_imxrt.c Outdated Show resolved Hide resolved
@anedergaard
Copy link
Contributor Author

Functionality LGTM. I think some naming alignment is needed. There's tmpsns, temp, temperature.

Yes, you are right, I'll align the naming once @DerekSnell has suggested a compatible string.

@DerekSnell
Copy link
Contributor

Hi @anedergaard ,
Thank you for this contribution, and for getting guidance on the naming. As @decsny stated, NXP does not like to use SOC or SOC family names in the bindings and drivers, because the hardware IP is frequently re-used across the portfolio on other familes.

The IP name for this HW peripheral is TMPSNS, which is also the name used in the SOC reference manual, and in the HAL driver fsl_tempsensor.c. So I think the best name for this binding is nxp,tmpsns.

Thank you

@anedergaard anedergaard force-pushed the driver-tmpsns-imxrt11xx branch from ab9fb98 to 42f9cdc Compare January 14, 2025 20:47
This commit points nxp_hal to the PR which enables mcux
driver for die temperature for i.MX RT117X and i.MX RT118X
devices.

Signed-off-by: Anders Bjørn Nedergaard <[email protected]>
Added die temperature binding for i.MX RT 117X and i.MX RT 118X

Signed-off-by: Anders Bjørn Nedergaard <[email protected]>
@anedergaard anedergaard force-pushed the driver-tmpsns-imxrt11xx branch from 42f9cdc to 9b87ba0 Compare January 14, 2025 20:55
@anedergaard
Copy link
Contributor Author

Hi @DerekSnell,
Thank you for the feedback on naming and the reasoning behind it - I appreciate it!

Updated 1

  • Addressed driver naming, renamed binding to nxp,tmpsns
  • Rebased to address conflicts

Added driver for i.MX RT117X and i.MX RT118X die temperature sensor

Signed-off-by: Anders Bjørn Nedergaard <[email protected]>
Added temp_sensor overlay for i.MX RT1170-EVK

Signed-off-by: Anders Bjørn Nedergaard <[email protected]>
@anedergaard anedergaard force-pushed the driver-tmpsns-imxrt11xx branch from 9b87ba0 to b891f14 Compare January 14, 2025 21:07
@anedergaard
Copy link
Contributor Author

Update 2:

  • Addressed compliance issue on Kconfig source order.

depends on SOC_SERIES_IMXRT11XX || SOC_SERIES_IMXRT118X
default y
help
Enable temperature measurement for NXP TMPSNS sensor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Enable temperature measurement for NXP TMPSNS sensor
Enable temperature measurement for NXP TMPSNS sensor

}
}

static const struct sensor_driver_api tmpsns_driver_api = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static const struct sensor_driver_api tmpsns_driver_api = {
static DEVICE_API(sensor, tmpsns_driver_api) = {

@@ -0,0 +1,3 @@
temp_sensor: &temp {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File needs a license/copyright header

Copy link
Collaborator

@jeppenodgaard jeppenodgaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming looks good :)

const struct nxp_tmpsns_data *data = dev->data;

switch (chan) {
case SENSOR_CHAN_ALL:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this line.
get cannot support SENSOR_CHAN_ALL since there is only one val reference.

Copy link
Contributor

@DerekSnell DerekSnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anedergaard ,

Thank you for contributing this. I am blocking to address this build warning when I build the test for the MIMXRT1170-EVK. The test does build and runs well on my board. But can you improve the DTS to avoid this warning?

CMake Warning at 
/zephyrproject/zephyr/cmake/modules/dts.cmake:425 (message):
  dtc raised one or more warnings:
 
  /build/zephyr/zephyr.dts:4051.27-4054.5:
  Warning (simple_bus_reg): /soc/temp: missing or empty reg/ranges property

Otherwise, the naming changes you made look good to me.

I see your PR got caught in a known CI issue with the RT1180, which should be resolved when #83827 merges.

A very minor nit, but since you will be updating - there is a typo in commit message "termerature"

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Sensors Sensors DNM This PR should not be merged (Do Not Merge) manifest manifest-hal_nxp platform: NXP Drivers NXP Semiconductors, drivers platform: NXP NXP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants