Skip to content

Conversation

@Raymond0225
Copy link
Contributor

On NXP RT1170 SOC, ADC ETC exists but it can not be enabled because of dependency on HAS_MCUX_ADC_ETC. The expectation is when CONFIG_ADC=y, ADC ETC should be enabled automatically if it exists. Fixes:#81466

@zephyrbot zephyrbot added platform: NXP NXP platform: NXP Drivers NXP Semiconductors, drivers area: ADC Analog-to-Digital Converter (ADC) labels Nov 15, 2024
@zephyrbot zephyrbot added the size: XS A PR changing only a single line of code label Nov 15, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this depend on the LPADC? Parts like the RT1064 which do not have an LPADC have the ADC_ETC IP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing that out. You are right , RT10xx did not use LPADC IP, it seems ADC_MCUX_12B1MSPS_SAR is used.
why I put dependency here it that I had a talk with some experts about ETC IP, it is an IP which can not be used standalone, which means in app , it must be combined with other "ADC" IPs. I had a look at the Kconfig.mcux for adc, 4 "ADC" IPs are listed: ADC_MCUX_ADC12 , ADC_MCUX_ADC16 , ADC_MCUX_12B1MSPS_SAR , ADC_MCUX_LPADC. I am thinking maybe we should only define ETC when any one of these IP defined. How do you think about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about depends on ADC? That needs the ADC driver included, but does not depend on specific IP.

Copy link
Contributor

@danieldegrasse danieldegrasse Nov 18, 2024

Choose a reason for hiding this comment

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

I think that is ok. I'll also note that the ADC_ETC IP isn't really supported in Zephyr right now- all this Kconfig does is enables a user to include the HAL driver in their application. So we should generally be less restrictive with dependencies, because a user enabling this Kconfig is using the HAL driver directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the dependency. if any ADC IP is defined, ETC is defined.

@DerekSnell DerekSnell linked an issue Nov 18, 2024 that may be closed by this pull request
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 @Raymond0225 ,
This is minor feedback, but if you push an update, you could improve the commit message subject. "driver: adc: kconfig" is very generic, and does not convey the change here. I think a better subject is

soc: nxp: imxrt11xx: select CONFIG_HAS_MCUX_ADC_ETC

@Raymond0225 Raymond0225 changed the title driver: adc: kconfig soc: nxp: imxrt11xx: select CONFIG_HAS_MCUX_ADC_ETC Nov 18, 2024
@Raymond0225
Copy link
Contributor Author

Hi @Raymond0225 , This is minor feedback, but if you push an update, you could improve the commit message subject. "driver: adc: kconfig" is very generic, and does not convey the change here. I think a better subject is

soc: nxp: imxrt11xx: select CONFIG_HAS_MCUX_ADC_ETC

thanks, done

danieldegrasse
danieldegrasse previously approved these changes Nov 18, 2024
DerekSnell
DerekSnell previously approved these changes Nov 18, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

For the time been, only ADC_MCUX_12B1MSPS_SAR (for RT10xx devices) and ADC_MCUX_LPADC (for RT11xx devices) use with ADC_MCUX_ETC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I know. Anyway, ADC_MUCX_ETC also depends on HAS_MCUX_ADC_ETC

Copy link
Contributor

@mmahadevan108 mmahadevan108 left a comment

Choose a reason for hiding this comment

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

Delete the HAS_MCUX_ADC_ETC config

Copy link
Member

@decsny decsny left a comment

Choose a reason for hiding this comment

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

the proper fix would be to make a DT binding and depend on that, do not use this legacy config

On NXP RT1170 SOC, ADC ETC exists but it can not be enabled because
of dependency on HAS_MCUX_ADC_ETC.
Also, ADC ETC should only work with ADC together, there is no use
case to run it standalone.
Fixes:zephyrproject-rtos#81466

Signed-off-by: Raymond Lei <[email protected]>
@Raymond0225
Copy link
Contributor Author

Raymond0225 commented Nov 21, 2024

the proper fix would be to make a DT binding and depend on that, do not use this legacy config

To my understanding, What I am doing is to fix the bug because of which adc etc driver can not be included. when it comes to how to remove legacy config, it is another issue, you can make a proposal in another PR.

@Raymond0225
Copy link
Contributor Author

Delete the HAS_MCUX_ADC_ETC config

This config is used not only for RT11xx but also RT10xx SOCs. What I do here is to fix the bug: etc driver can not be included on RT11xx SOCs. you can create another PR on how to delete this config.

Comment on lines +40 to +46
if ADC_MCUX_12B1MSPS_SAR || ADC_MCUX_LPADC
config ADC_MCUX_ETC
bool "MCUX ADC ETC driver"
depends on HAS_MCUX_ADC_ETC
help
Enable the MCUX ADC ETC driver.
endif
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
if ADC_MCUX_12B1MSPS_SAR || ADC_MCUX_LPADC
config ADC_MCUX_ETC
bool "MCUX ADC ETC driver"
depends on HAS_MCUX_ADC_ETC
help
Enable the MCUX ADC ETC driver.
endif
config ADC_MCUX_ETC
bool "MCUX ADC ETC driver"
depends on DT_HAS_NXP_ADC_ETC_ENABLED
help
Enable the MCUX ADC ETC driver.

and then just make a one-line binding file with the compat string, and add a small node to rt11xx.dtsi and rt10xx.dtsi that just looks like this basically:

adc_etc@deadbeef {
    compatible = "nxp,adc-etc";
};

can just leave status okay in soc level since the kconfig is n by default, if dont want to change all the board dts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why remove "if ADC_MCUX_12B1MSPS_SAR || ADC_MCUX_LPAD"? ETC can not work standalone

@fabiobaltieri fabiobaltieri assigned dleach02 and unassigned anangl Dec 6, 2024
@kartben kartben merged commit 0db1c07 into zephyrproject-rtos:main Dec 7, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ADC Analog-to-Digital Converter (ADC) platform: NXP Drivers NXP Semiconductors, drivers platform: NXP NXP size: XS A PR changing only a single line of code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config HAS_MCUX_ADC_ETC does not be included for RT11xx SOCs

10 participants