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

spi: allow impls returning early, add flush. #365

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Feb 16, 2022

Fixes #264
Depends on #351

  • Document that all methods may return early, before bus is idle.
  • Add a flush method to wait for bus to be idle.

The alternative is to document that methods MUST wait for bus idle before returning, but that would be bad for performance IMO. Allowing early return means multiple successive writes can "overlap" so that bytes come out back-to-back without gaps, which is great for workloads with tons of small writes, like SPI displays with embedded-graphics.

Drivers using SpiDevice won't have to worry about this, it'll Just Work since Device impls must flush before deasserting CS. Drivers using SpiBus will have to care if they coordinate SPI activity with other GPIO activity.

@rust-highfive
Copy link

r? @therealprof

(rust-highfive has picked a reviewer for you, use r? to override)

@ryankurte
Copy link
Contributor

a diff against #351 so this is easier to review pre that landing

src/spi/blocking.rs Outdated Show resolved Hide resolved
src/spi/blocking.rs Outdated Show resolved Hide resolved
@Dirbaio Dirbaio force-pushed the spi-flush branch 4 times, most recently from 9520962 to a0c1ed2 Compare February 18, 2022 19:20
@Dirbaio Dirbaio mentioned this pull request Feb 18, 2022
@Dirbaio
Copy link
Member Author

Dirbaio commented Feb 23, 2022

New version

  • Rebased on master
  • Expanded docs on when flush might be needed.

ryankurte
ryankurte previously approved these changes Feb 23, 2022
Copy link
Contributor

@ryankurte ryankurte left a comment

Choose a reason for hiding this comment

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

lgtm~! any other comments @hal? it'd be good to get this merged before too many folks refactor their SPI impls.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Other the nitpick below, looks good to me!

src/spi/blocking.rs Outdated Show resolved Hide resolved
Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM, too.

bors r+

@bors bors bot merged commit a3522db into rust-embedded:master Feb 24, 2022
@eldruin eldruin added this to the v1.0.0 milestone Feb 25, 2022
@eldruin eldruin mentioned this pull request Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify SPI timing behaviour (currently completely broken!)
6 participants