Skip to content

Swap the initialization order of ’spi_cs_control‘ members #97756

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Swap the initialization order of ’spi_cs_control‘ members #97756

wants to merge 1 commit into from

Conversation

ZhaoxiangJin
Copy link
Contributor

@ZhaoxiangJin ZhaoxiangJin commented Oct 17, 2025 •
edited
Loading

Fixes #97581, since the issue block some PRs which target v4.3 release, so, i set 'hotfix' tag to this PR, if anyone thinks it's inappropriate, feel free to remove it.

@ZhaoxiangJin ZhaoxiangJin mentioned this pull request Oct 17, 2025
Closed
1 task
@ZhaoxiangJin ZhaoxiangJin marked this pull request as ready for review October 17, 2025 02:29
@ZhaoxiangJin ZhaoxiangJin added the Hotfix Fix for issues blocking development, i.e. upstream CI issues, tests failing in upstream CI , etc. label Oct 17, 2025
@zephyrbot zephyrbot added the size: XS A PR changing only a single line of code label Oct 17, 2025
hakehuang
hakehuang previously approved these changes Oct 17, 2025
View reviewed changes
@ZhaoxiangJin ZhaoxiangJin force-pushed the fix-display-build-failure-on-mcxn branch 2 times, most recently from 7cb0cf0 to 17e5fd6 Compare October 17, 2025 04:44
@ZhaoxiangJin ZhaoxiangJin changed the title Fix grammar issues Swap the initialization order of ’spi_cs_control‘ members Oct 17, 2025
@JarmouniA
Copy link
Contributor

Putting cs_is_gpio last was suggested here #97366 (comment), following this PR #97292. Cc @decsny

@JarmouniA
Copy link
Contributor

Cc @jeremydick

@JarmouniA JarmouniA mentioned this pull request Oct 17, 2025
Open
1 task
@hakehuang
Copy link
Contributor

hakehuang commented Oct 17, 2025 •
edited
Loading

Putting cs_is_gpio last was suggested here #97366 (comment), following this PR #97292. Cc @decsny

the root cause here is that SPI_CS_CONTROL_INIT_GPIO and SPI_CS_CONTROL_INIT_NATIVE macros only initialize the union members, and cs_is_gpio must be initialized regardless of which union member is being used, Declan's former code does not have the cs_is_gpio used in SPI_CS_CONTROL_INIT_NATIVE, so it works. @decsny

@hakehuang hakehuang self-requested a review October 17, 2025 08:07
@hakehuang hakehuang added the area: SPI SPI bus label Oct 17, 2025
@ZhaoxiangJin
Copy link
Contributor Author

this is the results after preprocessing
image

@ZhaoxiangJin
include: mipi_dbi.h: Swap the initialization order
23b73e0 
When using the macro 'SPI_CS_CONTROL_INIT_GPIO' to
initialize the 'spi_cs_control' member, it is missing ','
after '.delay' leading to a compilation error.

We can place the initialization of the 'cs_is_gpio'
member at the very beginning. This way, whether initialized
via 'SPI_CS_CONTROL_INIT_GPIO' or 'SPI_CS_CONTROL_INIT_NATIVE',
there will be no compilation issues.

Signed-off-by: Zhaoxiang Jin <[email protected]>
@ZhaoxiangJin ZhaoxiangJin force-pushed the fix-display-build-failure-on-mcxn branch from 17e5fd6 to 23b73e0 Compare October 17, 2025 08:47
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@nashif nashif added Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. and removed Hotfix Fix for issues blocking development, i.e. upstream CI issues, tests failing in upstream CI , etc. labels Oct 17, 2025
@decsny
Copy link
Member

decsny commented Oct 18, 2025

i think if this PR merge that I already put then it would solve the problem, but nobody is reviewing/merging it even though its a hotfix: #97292

decsny
decsny requested changes Oct 18, 2025
View reviewed changes
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.

i think this PR is not right, it doesnt respect anonymous initialization order of the struct, which can cause error based on compiler, according to user report using clang

@ZhaoxiangJin
Copy link
Contributor Author

ZhaoxiangJin commented Oct 18, 2025 •
edited
Loading

i think if this PR merge that I already put then it would solve the problem, but nobody is reviewing/merging it even though its a hotfix: #97292

Let's close this PR and move to yours

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@decsny decsny decsny requested changes

@hakehuang hakehuang hakehuang approved these changes

@JarmouniA JarmouniA Awaiting requested review from JarmouniA

@danieldegrasse danieldegrasse Awaiting requested review from danieldegrasse

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@teburd teburd Awaiting requested review from teburd

Assignees

No one assigned

Labels

area: Display area: SPI SPI bus size: XS A PR changing only a single line of code Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tests: display: tests.drivers.display.check.shield : build failure for frdm_mcxn947/mcxn947/cpu0 and frdm_mcxn236

6 participants

@ZhaoxiangJin @JarmouniA @hakehuang @decsny @nashif @zephyrbot