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.

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

cpu/sam0_common: SPI: MOSI only operation & fix for adafruit-itsybitsy-m4 #15846

Merged
merged 2 commits into from Jan 28, 2021
Merged

cpu/sam0_common: SPI: MOSI only operation & fix for adafruit-itsybitsy-m4 #15846

merged 2 commits into from Jan 28, 2021

Conversation

benpicco
Copy link
Contributor

Contribution description

  • If no MISO is configured, don't enable it. This is useful for devices like APA102 LEDs that don't have a back-channel
  • On adafruit-itsybitsy-m4 I would not get a signal on MOSI unless I also would mux the pin on acquire. Without this, MOSI would just go low during the transmission with no output. I can't really explain it, in tests/periph_spi with MISO & MOSI connected I would see
    • init 0 0 0
    • send test -> no reply
    • spi_gpio 0
    • send test -> got reply

I narrowed this down to the second gpio_init_mux() fixing SPI. Turns out a late gpio_init_mux() on spi_aquire() would also do.

Also, why are MISO and CLK configured as output gpio? Shouldn't this not be necessary if we mux them to SERCOM mode anyway?

Testing procedure

tests/periph_spi should still work.
Pay attention to changes in power draw with SPI devices attached, I didn't see a difference on a quick test on samr21-xpro after putting the radio to sleep.

Issues/PRs references

@benpicco
cpu/sam0_common: SPI: allow to only configure MOSI/CLK
00a467e 
Some slave devices (e.g. LED strips) don't have a back-channel and
will only need MOSI and CLK.
@benpicco benpicco requested review from jue89 and dylad January 24, 2021 17:01
@benpicco benpicco added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers and removed Area: drivers Area: Device drivers labels Jan 24, 2021
@benpicco benpicco requested a review from keestux January 24, 2021 17:02
@benpicco benpicco mentioned this pull request Jan 24, 2021
Merged
dylad
dylad reviewed Jan 24, 2021
View reviewed changes
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.

On adafruit-itsybitsy-m4 I would not get a signal on MOSI unless I also would mux the pin on acquire

What sorcery is this ??

Overall looks good but I'm wondering if this doesn't hide some weird bug on the hardware side of your board.

cpu/sam0_common/periph/spi.c Outdated Show resolved Hide resolved
cpu/sam0_common/periph/spi.c Outdated Show resolved Hide resolved
@dylad
Copy link
Member

dylad commented Jan 26, 2021

Tested on SAML21 with SPI <-> SDCARD module on test/drivers_sdcard_spi and tests/pkg_fatfs
Tests work as before.

Please squash.

@dylad dylad added this to the Release 2021.04 milestone Jan 26, 2021
@dylad dylad added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 26, 2021
dylad
dylad approved these changes Jan 27, 2021
View reviewed changes
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. I did not notice any regression on SAME54 and SAML21 so we should be safe.

@dylad dylad added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 27, 2021
@dylad dylad merged commit 1d0dbb4 into RIOT-OS:master Jan 28, 2021
@benpicco benpicco deleted the cpu/sam0_common-spi_fixes branch January 28, 2021 10:46
@benpicco benpicco mentioned this pull request Feb 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylad dylad dylad approved these changes

@jue89 jue89 Awaiting requested review from jue89

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

@keestux keestux Awaiting requested review from keestux keestux is a code owner

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines
Projects
None yet
Milestone
Release 2021.04
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@benpicco @dylad