Skip to content

cpu/esp32: fix CS handling in spi_transfer_bytes#12538

Merged
maribu merged 1 commit intoRIOT-OS:masterfrom
gschorcht:cpu/esp32/fix_cs_handling
Oct 22, 2019
Merged

cpu/esp32: fix CS handling in spi_transfer_bytes#12538
maribu merged 1 commit intoRIOT-OS:masterfrom
gschorcht:cpu/esp32/fix_cs_handling

Conversation

@gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Oct 22, 2019

Contribution description

This PR fixes the CS pin handling in spi_transfer_bytes. If SPI_CS_UNDEF is given as the cs parameter, CS pin MUST not be handled by the driver.

Testing procedure

It's a very small change. Compilation should be enough as test.

make BOARD=esp32-wroom-32 -C tests/periph_spi

The change was tested with a complex device driver that is using SPI:

USEMODULE=mrf24j40 make BOARD=esp32-mh-et-live-minikit -C examples/gnrc_networking flash term

Issues/PRs references

Problem was figured out in #12118 when testing with #12518.

@gschorcht
cpu/esp32: fix CS handling in spi_transfer_bytes
db945bd 
If `SPI_CS_UNDEF` is given as the `cs` parameter, CS pin must not be handled by the driver. Furthermore, if `cont` parameter is true, CS pin must not be disabled at the end of one transfer.
@gschorcht gschorcht mentioned this pull request Oct 22, 2019
Merged
@gschorcht gschorcht added Area: cpu Area: CPU/MCU ports 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 Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 22, 2019
@gschorcht gschorcht requested a review from maribu October 22, 2019 06:22
maribu
maribu approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@maribu maribu 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'm 100% sure that the code works as expected.

@maribu maribu added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer 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 labels Oct 22, 2019
@maribu maribu merged commit cd5cda2 into RIOT-OS:master Oct 22, 2019
@gschorcht
Copy link
Contributor Author

gschorcht commented Oct 22, 2019

@maribu Thanks a lot.

@gschorcht gschorcht deleted the cpu/esp32/fix_cs_handling branch October 22, 2019 16:32
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu approved these changes

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports 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 Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Platform: ESP Platform: This PR/issue effects ESP-based platforms 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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2020.01

Development

Successfully merging this pull request may close these issues.

3 participants

@gschorcht @maribu @fjmolinas