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

drivers: add driver for AT25xxx family of EEPROMs #11945

Merged
merged 2 commits into from Apr 7, 2020
Merged

drivers: add driver for AT25xxx family of EEPROMs #11945

merged 2 commits into from Apr 7, 2020

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jul 31, 2019 •
edited

Contribution description

ST offers SPI based EEPROMs from the M95xxx family.
I've tested it with the M95M01 EEPROM. Other variants exist that differ in page size and address length, but follow the same command scheme.

Devices from other vendors that function in the same way include the families AT25xxx, 25AAxxx, 25LCxxx, CAT25xxx & BR25Sxxx.

I've decided to make both page size and address length a compile-time constant as it allows for compile-time optimizations and the I considered likelihood of having two EEPROMs of different type on the same board to be rather slim.

I can move those parameters to at25xxx_params_t if desired.

The EEPROM interface definition is blatantly stolen from #11929, but no periph_eeprom compatibility wrapper is provided yet.

Testing procedure

  • connect an EEPROM to the SPI bus
  • set AT25XXX_PARAM_… according do your board and EEPROM device
  • run tests/driver_at25xxx

Issues/PRs references

will be converted to the new EEPROM API once #12091 is finalized.

This provides an SPI Implementation analogous to the I2C EEPROM implementation in #11929

So we'll have

  • AT24xxx -> I2C EEPROM
  • AT25xxx -> SPI EEPROM

@benpicco benpicco added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jul 31, 2019
@benpicco benpicco changed the title drivers: add driver for M95xxx family of EEPROMs drivers: add driver for AT25xxx family of EEPROMs Jul 31, 2019
@benpicco benpicco mentioned this pull request Jul 31, 2019
Merged
@benpicco benpicco force-pushed the EEPROM-m95m01 branch 2 times, most recently from 612968d to 46e3d18 Compare August 1, 2019 08:30
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 26, 2019
@maribu maribu self-requested a review August 27, 2019 09:54
maribu
maribu reviewed Aug 27, 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.

Looks very good to me. I have some inline-comments. But I guess we should wait if an agreement about a common EEPROM API can be reached within reasonable time, so that using external EEPROM devices becomes easier.

drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
drivers/at25xxx/at25xxx.c Show resolved Hide resolved
drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
drivers/at25xxx/at25xxx.c Show resolved Hide resolved
drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
@maribu maribu 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 labels Aug 27, 2019
@benpicco benpicco added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 28, 2019
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 15, 2019
fabian18
fabian18 reviewed Feb 25, 2020
View reviewed changes
Copy link
Contributor

@fabian18 fabian18 left a comment

Choose a reason for hiding this comment

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

The code looks pretty slim and clean in my eyes. Have a look at inline comments.

drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor Author

Rebased to current master

@benpicco benpicco removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 4, 2020
@benpicco benpicco force-pushed the EEPROM-m95m01 branch 2 times, most recently from 3cd680e to ee0f483 Compare March 5, 2020 18:49
@benpicco benpicco added this to Backlog / Proposals in RIOT Sprint Day via automation Mar 30, 2020
@fjmolinas
Copy link
Contributor

@benpicco could you post some test output?

@benpicco
Copy link
Contributor Author

benpicco commented Mar 31, 2020 •
edited 

2020-03-31 13:50:53,452 # AT25XXX EEPROM driver test application
2020-03-31 13:50:53,452 # 
2020-03-31 13:50:53,454 # EEPROM size: 128 kiB
2020-03-31 13:50:53,456 # EEPROM page size: 256 bytes
2020-03-31 13:50:53,459 # EEPROM address length: 24 bits
2020-03-31 13:50:53,459 # 
2020-03-31 13:50:53,462 # .Running test_normal_write test
2020-03-31 13:50:53,473 # .Running test_page_write test
2020-03-31 13:50:53,492 # .Running test_page_clear test
2020-03-31 13:50:53,509 # 
2020-03-31 13:50:53,510 # OK (3 tests)
2020-03-31 13:50:53,512 # All tests done.

Which reminds me - I should probably remove that additional debug output.

@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 31, 2020
@benpicco
Copy link
Contributor Author 

2020-03-31 13:54:08,546 # main(): This is RIOT! (Version: 2020.04-devel-1744-g06810-EEPROM-m95m01)
2020-03-31 13:54:08,550 # AT25XXX EEPROM driver test application
2020-03-31 13:54:08,550 # 
2020-03-31 13:54:08,552 # EEPROM size: 128 kiB
2020-03-31 13:54:08,554 # EEPROM page size: 256 bytes
2020-03-31 13:54:08,557 # EEPROM address length: 24 bits
2020-03-31 13:54:08,557 # 
2020-03-31 13:54:08,599 # ...
2020-03-31 13:54:08,600 # OK (3 tests)

@aabadie aabadie moved this from Backlog / Proposals to Under Review in RIOT Sprint Day Apr 7, 2020
@aabadie aabadie self-assigned this Apr 7, 2020
aabadie
aabadie reviewed Apr 7, 2020
View reviewed changes
drivers/at25xxx/at25xxx.c Outdated Show resolved Hide resolved
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 7, 2020
aabadie
aabadie reviewed Apr 7, 2020
View reviewed changes
drivers/include/at25xxx.h Outdated Show resolved Hide resolved
@aabadie
Copy link
Contributor

aabadie commented Apr 7, 2020

Sorry, I just noticed one last thing regarding doxygen. Without this, the module would have appeared in the first level modules list.

@benpicco
drivers: add AT25xxx series EEPROM
97fc2f2 
This adds a driver for the ST M95xxx series SPI EEPROMs.
The driver has been tested with the M95M01 EEPROM, but should
work with other chips from that family.

SPI-EEPROMs from other vendors from the families AT25xxx, 25AAxxx,
25LCxxx, CAT25xxx & BR25Sxxx should also in the same way.
@benpicco
tests: add test for at25xxx EEPROM driver
5ef52c7
aabadie
aabadie approved these changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment •
edited

Choose a reason for hiding this comment

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

ACK , please squash! (already squashed...)

RIOT Sprint Day automation moved this from Under Review to Waiting For Murdock Apr 7, 2020
@aabadie aabadie merged commit 067b324 into RIOT-OS:master Apr 7, 2020
RIOT Sprint Day automation moved this from Waiting For Murdock to Done Apr 7, 2020
@benpicco benpicco deleted the EEPROM-m95m01 branch April 7, 2020 11:20
@benpicco
Copy link
Contributor Author

benpicco commented Apr 7, 2020

Thank you for the review!

@benpicco benpicco mentioned this pull request Apr 7, 2020
Closed
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu left review comments

@kaspar030 kaspar030 kaspar030 left review comments

@fabian18 fabian18 fabian18 left review comments

@aabadie aabadie aabadie approved these changes

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

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

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

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

@smlng smlng Awaiting requested review from smlng

Assignees

@aabadie aabadie

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 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 Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
RIOT Sprint Day
  
Done
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@benpicco @fjmolinas @aabadie @maribu @kaspar030 @fabian18 @leandrolanzieri