Skip to content

Conversation

@dpw13
Copy link
Contributor

@dpw13 dpw13 commented Nov 13, 2024

If CONFIG_SPI_STATS is enabled, the device state for all SPI controller drivers must contain the SPI stats. This space is allocated by calling Z_SPI_INIT_FN as part of the device definition; this is done automatically when using SPI_DEVICE_DT_DEFINE instead of DEVICE_DT_DEFINE. If space for statistics is not properly allocated but CONFIG_SPI_STATS is enabled, an unexpected write to memory outside of the stats region may occur on a SPI transfer. This commit uses SPI_DEVICE_DT_DEFINE or SPI_DEVICE_DT_INST_DEFINE for all in-tree SPI controller drivers.

@github-actions
Copy link

Hello @dpw13, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

ifyall
ifyall previously approved these changes Nov 13, 2024
@dpw13
Copy link
Contributor Author

dpw13 commented Nov 14, 2024

Apologies for the force-push, I'm still working out how to address code style issues and the best way I found was to reformat modified lines pre-commit. It looks like the compliance check wants me to reformat some fairly large sections of macro in some of these files and it appears to me that clang-format results in some pretty ugly reformats. I've applied clang-format in places where the results look good and manually modified everything else. Please let me know if I need to reformat more; I don't mind making the effort to clean up these blocks.

@dpw13 dpw13 requested a review from ifyall November 26, 2024 15:34
tbursztyka
tbursztyka previously approved these changes Nov 26, 2024
Copy link
Contributor

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

@dpw13 force push a PR is a normal procedure, it's totally fine.

lgtm

soburi
soburi previously approved these changes Dec 4, 2024
Copy link
Member

@soburi soburi left a comment

Choose a reason for hiding this comment

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

I checked renesas_ra8, gd32, pl022, these are fine.

If CONFIG_SPI_STATS is enabled, the device state for all SPI controller
drivers must contain the SPI stats. This space is allocated by calling
Z_SPI_INIT_FN as part of the device definition; this is done automatically
when using SPI_DEVICE_DT_DEFINE instead of DEVICE_DT_DEFINE. If space for
statistics is not properly allocated but CONFIG_SPI_STATS is enabled, an
unexpected write to memory outside of the stats region may occur on a SPI
transfer. This commit uses SPI_DEVICE_DT_DEFINE or
SPI_DEVICE_DT_INST_DEFINE for all in-tree SPI controller drivers.

Signed-off-by: Dane Wagner <[email protected]>
@dpw13 dpw13 dismissed stale reviews from soburi and tbursztyka via 0544746 December 4, 2024 15:59
@dpw13 dpw13 force-pushed the dwagner/spi-stats-fixup branch from a6692b1 to 0544746 Compare December 4, 2024 15:59
@dpw13
Copy link
Contributor Author

dpw13 commented Dec 4, 2024

Fixed merge conflict in zephyr/drivers/spi/spi_mcux_lpspi.c. No functional changes since last push.

@dpw13 dpw13 requested a review from tbursztyka December 4, 2024 16:00
@dpw13 dpw13 requested a review from soburi December 4, 2024 16:00
@kartben
Copy link
Contributor

kartben commented Dec 6, 2024

@tbursztyka please take a look / re-approve. Thanks!

@kartben kartben merged commit c4e840a into zephyrproject-rtos:main Dec 6, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants