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

boards: remove extern mtd_dev_t *mtd<n> declarations from board definitions #20104

Merged
merged 7 commits into from
Dec 7, 2023
Merged

boards: remove extern mtd_dev_t *mtd<n> declarations from board definitions #20104

merged 7 commits into from
Dec 7, 2023

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Nov 23, 2023 •
edited
Loading

Contribution description

The PR removes the extern mld_dev_t *mtd<n> declarations from board definitions.

For that purpose, the extern mtd_dev_t *mtd<n> declarations that use the MTD_<n> definitions of the board are moved from mtd_default.h to mtd.h to declare them even if the mtd_default module is not used.

Testing procedure

Green CI.

Issues/PRs references

Prerequisite for PR #19786
Prerequisite for PR #19540

@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Area: drivers Area: Device drivers Area: boards Area: Board ports labels Nov 23, 2023
@gschorcht gschorcht added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 23, 2023
@gschorcht gschorcht force-pushed the drivers/mtd/mtddev_declarations branch from 9df918d to c531cdd Compare November 23, 2023 17:46
This was referenced Nov 23, 2023
Merged
Merged
@riot-ci
Copy link

riot-ci commented Nov 23, 2023 •
edited
Loading

Murdock results

✔️ PASSED

c93f0a7 tests/pkg/fatfs_vfs: replace external mtd0 declaration

Success Failures Total Runtime
8082 0 8082 10m:37s

Artifacts

@benpicco benpicco changed the title boards: remove extern mld_dev_t *mtd<n> declarations from board definitions boards: remove extern mtd_dev_t *mtd<n> declarations from board definitions Nov 23, 2023
@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 24, 2023
@gschorcht gschorcht force-pushed the drivers/mtd/mtddev_declarations branch from 2a01590 to 1236536 Compare November 25, 2023 07:41
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Nov 25, 2023
@gschorcht gschorcht removed the Platform: native Platform: This PR/issue effects the native platform label Nov 25, 2023
@dylad
Copy link
Member

dylad commented Nov 26, 2023

Just wondering:
Is there a reason you kept mtd.h file from boards/iotlab-m3/include/board.h and boards/common/esp8266/include/board_common.h ? You dropped it from all other board.h files.

@github-actions github-actions bot added the Platform: native Platform: This PR/issue effects the native platform label Nov 27, 2023
@gschorcht
Copy link
Contributor Author

Just wondering: Is there a reason you kept mtd.h file from boards/iotlab-m3/include/board.h and boards/common/esp8266/include/board_common.h

No, I just forgot them, boards/common/esp8266/include/board_common.h just because I only searched for mtd.h in all board.h files but not in board*.h.

@benpicco
Copy link
Contributor

benpicco commented Dec 4, 2023

Please squash!
(you might also need a rebase to satisfy CI)

benpicco
benpicco reviewed Dec 4, 2023
View reviewed changes
drivers/include/mtd.h Outdated
Comment on lines 171 to 189
#ifdef MTD_0
extern mtd_dev_t *MTD_0;
#endif
#ifdef MTD_1
extern mtd_dev_t *MTD_1;
#endif
#ifdef MTD_2
extern mtd_dev_t *MTD_2;
#endif
#ifdef MTD_3
extern mtd_dev_t *MTD_3;
#endif
#ifdef MTD_4
extern mtd_dev_t *MTD_4;
#endif
#ifdef MTD_5
extern mtd_dev_t *MTD_5;
#endif
#endif /* !DOXYGEN */
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this?

@benpicco
Copy link
Contributor

benpicco commented Dec 4, 2023

Hm why not rip off the band aid completely and replace MTD_0 with mtd_dev_get(0) for the few occurences that there are.

I might also just submit a PR for that.

@gschorcht
Copy link
Contributor Author

Hm why not rip off the band aid completely and replace MTD_0 with mtd_dev_get(0) for the few occurences that there are.

How would you handle this case?

RIOT/tests/pkg/littlefs/main.c

Line 29 in 848f3d1

#if defined(MTD_0) && IS_ACTIVE(CONFIG_USE_HARDWARE_MTD)

@gschorcht
Copy link
Contributor Author

Hm why not rip off the band aid completely and replace MTD_0 with mtd_dev_get(0) for the few occurences that there are.

Do you think that all #define MTD<n> mtd<n> should be completely removed from the board definitions and MTD<n> should not be used at all? Or do you mean that the boards should rather define

#define MTD_0 mtd_dev_get(0)

instead of

#define MTD0 mtd0

@gschorcht
drivers/mtd_default: remove extern mtd_dev_t * declarations
4b6e13c
@gschorcht
drivers/mtd: replace checks by assert in mtd_dev_get
9410ec7 
Assertions are used instead of returning a NULL pointer to detect errors in the MTD definition and access in the case that the return value is not evaluated.
@gschorcht
boards: remove extern mtd_dev_t* Declarations
26030e2
@gschorcht
cpu/native: replace external mtd0 declaration
d59d48d 
Since the `extern mtd_dev_t *` declarations were removed from board definitions, `mtd_dev_get` has to be used instead.
@gschorcht
tests/unittests: fix MTD unit test compilation
cfc2232
@gschorcht
tests/pkg/fatfs: replace external mtd0 declaration
1ad92d4 
Since the `extern mtd_dev_t *` declarations were removed from board definitions, `mtd_dev_get` has to be used instead.
@gschorcht
tests/pkg/fatfs_vfs: replace external mtd0 declaration
c93f0a7 
Since the `extern mtd_dev_t *` declarations were removed from board definitions, `mtd_dev_get` has to be used instead.
@gschorcht gschorcht force-pushed the drivers/mtd/mtddev_declarations branch from 4a21c4c to c93f0a7 Compare December 7, 2023 14:34
@github-actions github-actions bot added the Area: cpu Area: CPU/MCU ports label Dec 7, 2023
benpicco
benpicco approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@benpicco benpicco added this pull request to the merge queue Dec 7, 2023
Merged via the queue into RIOT-OS:master with commit 67df59b Dec 7, 2023
26 checks passed
@gschorcht
Copy link
Contributor Author

Thanks

@gschorcht gschorcht deleted the drivers/mtd/mtddev_declarations branch December 10, 2023 16:20
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.01 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

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

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

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

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

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

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

@smlng smlng Awaiting requested review from smlng

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

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: native Platform: This PR/issue effects the native platform Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2024.01
Development

Successfully merging this pull request may close these issues.
5 participants
@gschorcht @riot-ci @dylad @benpicco @MrKevinWeiss