Skip to content
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

drivers/periph_{i2c,spi}: fix doc on initialization #21188

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 4, 2025

Contribution description

Both SPI and I2C peripheral drivers MUST NOT be initialized by the application code / (non-peripheral) device driver, as this is done automatically be default. Some peripheral drivers have a non-idempotent initialization and initializing them twice will break things.

Sadly, the doc states the exact opposite.

This updates the documentation to match the implementation. In addition the special case is pointed out of disabling the automatic initialization during boot, in which case the app indeed has to do the initialization.

Testing procedure

The CI will generate a preview of the doc that can be checked.

Issues/PRs references

Might fix #21187

Both SPI and I2C peripheral drivers *MUST NOT* be initialized by the
application code / (non-peripheral) device driver, as this is done
automatically be default. Some peripheral drivers have a non-idempotent
initialization and initializing them twice will break things.

Sadly, the doc states the exact opposite.

This updates the documentation to match the implementation. In addition
the special case is pointed out of disabling the automatic
initialization during boot, in which case the app indeed has to do the
initialization.
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 4, 2025
@maribu maribu requested a review from MrKevinWeiss as a code owner February 4, 2025 08:26
@github-actions github-actions bot added Area: drivers Area: Device drivers and removed Area: doc Area: Documentation labels Feb 4, 2025
@riot-ci
Copy link

riot-ci commented Feb 4, 2025

Murdock results

✔️ PASSED

a825735 drivers/periph_{i2c,spi}: fix doc on initialization

Success Failures Total Runtime
1 0 1 01m:13s

Artifacts

@dylad
Copy link
Member

dylad commented Feb 4, 2025

What about removing i2c/spi_init from the header to prevent users from easily adding this function ? We can keep those init functions somehow "private". And of course, explicitly explain why we do this in the doc :)

@maribu
Copy link
Member Author

maribu commented Feb 4, 2025

I just had a similar thought. Maybe rename the functions to i2c_init_internal_use_only() and and spi_init_internal_use_only() and call them from periph_init.

If and only if periph_init_i2c is not used, provide the symbol i2c_init. That way users will notice the issue at link time, even if the provide the forward declaration of i2c_init() by hand.

This will also keep existing users that do correctly add DISABLE_MODULE += periph_init_i2c working without change.

@dylad
Copy link
Member

dylad commented Feb 4, 2025

If and only if periph_init_i2c is not used, provide the symbol i2c_init. That way users will notice the issue at link time, even if the provide the forward declaration of i2c_init() by hand.
This will also keep existing users that do correctly add DISABLE_MODULE += periph_init_i2c working without change.

Sounds perfectly reasonable to me.

Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

ACK.
Changes in the doc looks good. Let's rework the I2C/SPI init in a followup PR.

@dylad dylad enabled auto-merge February 4, 2025 12:05
@dylad dylad added this pull request to the merge queue Feb 4, 2025
Merged via the queue into RIOT-OS:master with commit c81a103 Feb 4, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I2C Crashes Kernel on Raspberry Pi Pico
3 participants