Skip to content

Fixup SPI init procedure, SPI EEPROM sequencing #17534

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

Merged
merged 1 commit into from
Jul 5, 2022
Merged

Conversation

tzarc
Copy link
Member

@tzarc tzarc commented Jul 2, 2022

Description

Something weird going on with SPI in the sense that the first use would seemingly fail after init, but only on RP2040.
Under normal circumstances, the first read would be EEPROM when using SPI EEPROM, specifically the magic... which would then trigger an EEPROM reset as the data returned is zero'd out on SPI failure.

This PR changes the sequencing:

  • On spi_init it stops the SPI peripheral as a final action,
  • SPI EEPROM now restarts the SPI peripheral each time the status register is read
  • SPI EEPROM driver is now slightly more defensive w.r.t. executing spi_stop() on failure -- the previous assumption was that spi_start() failing did not necessitate a subsequent spi_stop() -- this now explicitly invokes the stop upon all failures.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@tzarc tzarc requested a review from a team July 2, 2022 06:37
@github-actions github-actions bot added the core label Jul 2, 2022
@tzarc tzarc added the bug label Jul 2, 2022
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

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

On spi_init it stops the SPI peripheral as a final action

As a precaution - I've checked all the places that this is called and all drivers do issue a spi_start() before doing anything (as they should) so this should not break anything. Curious why this fails on the RP2040 though. I don't have a SPI flash chip to test but otherwise this LGTM.

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Tested with SPI eeprom chip on a blackpill, and doesn't seem to adversely effect it (this is including with other SPI devices attached too, namely the pmw3360).

@KarlK90
Copy link
Member

KarlK90 commented Jul 4, 2022

Oh wow, I do have an SPI EEPROM. Somehow I read it as SPI Flash all the time. Going to test it this evening with rp2040.

@KarlK90
Copy link
Member

KarlK90 commented Jul 5, 2022

Tested with rp2040 and spi eeprom, LGTM!

@KarlK90 KarlK90 merged commit 29a2bac into qmk:develop Jul 5, 2022
@tzarc tzarc deleted the fixup-spi branch August 4, 2022 11:46
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants