-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
tests: adc_api: Use new DT_LABEL macro for getting device name #24623
Conversation
Replace dts_fixup.h based DT_ADC_0_NAME defines with DT_LABEL as we phase out dts_fixup.h Signed-off-by: Kumar Gala <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Atmel SAM
@@ -29,7 +29,7 @@ | |||
#elif defined(CONFIG_BOARD_NRF51DK_NRF51422) | |||
|
|||
#include <hal/nrf_adc.h> | |||
#define ADC_DEVICE_NAME DT_ADC_0_NAME | |||
#define ADC_DEVICE_NAME DT_LABEL(DT_INST(0, nordic_nrf_adc)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we be sure that DT_INST(0, nordic_nrf_adc)
is ADC_0
? Shouldn't we use DT_NODELABEL()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing in this case its know that ADC is a singleton, since the driver uses DT_INST.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was more of a general comment to the conversions performed in this PR. Some of the coversions convert from ADC_1 to DT_INST(0, ...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the driver is multi-instance at all, the INST ordering should be assumed to be basically random, so this is indeed an issue in general (though for this particular line of source it's a singleton).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbolivar-nordic I might have been a bit too trigger-happy with this one. @henrikbrixandersen please feel free to continue the conversation here and then we can create a separate PR if necessary to tweak whatever is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think the INST conversions should be removed and replaced with either NODELABEL or ALIAS.
Edit: in cases where ambiguity is possible, not this particular line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer NODELABEL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or in this case, for a lot of these PATH would make sense since they are board specific ifdefs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either NODELABEL or PATH seems fine as way to resolve this issue
Replace dts_fixup.h based DT_ADC_0_NAME defines with DT_LABEL as we
phase out dts_fixup.h
Signed-off-by: Kumar Gala [email protected]