drivers: video: emul_rx: hotfix: force use of constant value #88890
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JarmouniA
approved these changes
Apr 22, 2025
JarmouniA
added
the
Hotfix
|
Trying on latest The hotfix is still expected to address the error as uses the value from devicetree rather than the previously declared |
I think you need to enable some specific compiler flag for it to appear. |
|
Can you add more details ? I couldn't reproduce the error on my side. |
|
@JarmouniA good point, different flags for west build and west twister it seems. |
|
It looks like the hotfix works: Before: After: |
kartben
previously approved these changes
Apr 22, 2025
ngphibang
requested changes
Apr 22, 2025
There was a problem hiding this comment.
Marked as request-changes for now as we needs to do the same for other drivers even if it's a needed fix. But I believe this is a clang issue and already fixed in LLVM newer versions as stated in #54826 unless someone proves that this is not.
In Clang 16 run with some flags, the compiler does not accept a static const variables as struct initializer. This caused build errors in only some contexts. Always use the devicetree macros to access the source device node as a workaround. Signed-off-by: Josuah Demangeon <[email protected]>
josuah
force-pushed
the
pr-hotfix-video-emul-imager
branch
from
e6dfdfe to
f2e8156
Compare
github-actions
bot
added
platform: ESP32
github-actions
bot
requested review from
dbaluta,
decsny,
dleach02,
LucasTambor,
manuargue and
marekmatej
github-actions
bot
requested review from
mmahadevan108,
Raymond0225,
sylvioalves and
uLipe
|
@josuah: Thanks for adding other drivers into the PR. But do you see it is fixed in newer LLVM versions (I don't have problem using Zephyr SDK) and think it is a clang issue ? |
JarmouniA
approved these changes
Apr 22, 2025
I confirm, it works with Testing on current Fails: Works: full logs |
sylvioalves
approved these changes
Apr 22, 2025
kartben
approved these changes
Apr 22, 2025
|
So, do we need to fix ? if yes, it's ok for me. |
|
So this fixes for clang-18 but keeps clang-16 failing, right? |
ngphibang
approved these changes
Apr 22, 2025
JarmouniA
reviewed
Apr 22, 2025
| @@ -282,6 +283,6 @@ int emul_rx_init(const struct device *dev) | |||
| DEVICE_DT_INST_DEFINE(n, &emul_rx_init, NULL, &emul_rx_data_##n, &emul_rx_cfg_##n, \ | |||
| POST_KERNEL, CONFIG_VIDEO_INIT_PRIORITY, &emul_rx_driver_api); \ | |||
| \ | |||
| VIDEO_DEVICE_DEFINE(emul_rx_##n, DEVICE_DT_INST_GET(n), emul_rx_cfg_##n.source_dev); | |||
| VIDEO_DEVICE_DEFINE(emul_rx_##n, DEVICE_DT_INST_GET(n), SOURCE_DEV(n)); | |||
There was a problem hiding this comment.
Regardless of this being an LLVM issue or not, to me it makes perfect sense to use the DT value here directly, instead of passing it with a struct field.
There was a problem hiding this comment.
No clean-up commit needed after it gets merged, then. 👍
Glad you asked, that was unclear.
|
ngphibang
reviewed
Apr 23, 2025
| #define NXP_VIDEO_SDMA_INIT(inst) \ | ||
| PINCTRL_DT_INST_DEFINE(inst); \ | ||
| const struct nxp_video_sdma_config sdma_config_##inst = { \ | ||
| .dma_dev = DEVICE_DT_GET(DT_INST_PARENT(inst)), \ | ||
| .sensor_dev = DEVICE_DT_GET(DT_INST_PHANDLE(inst, sensor)), \ | ||
| .sensor_dev = SOURCE_DEV(n), \ |
There was a problem hiding this comment.
This should be .sensor_dev = SOURCE_DEV(inst). CI didn't detect this failure because smartdma driver does not have a built-in test. I will fix this in #88323.
There was a problem hiding this comment.
I checked not thoroughly enough. Thank you for the vigilance.
Checking again, the others drivers not have this mismatch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In some compilation attempts, the compiler did not accept a static const variable as initializer. This caused build errors in only some contexts. Always use the devicetree macros to access it.
Note: this has not yet been tested: PR open to check if it fixes it in the actual Zephyr CI