Skip to content

boards/kw41z*: add common configuration and use it with existing kw41z boards #11044

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
Merged
merged 4 commits into from
May 14, 2019
Merged

boards/kw41z*: add common configuration and use it with existing kw41z boards #11044

merged 4 commits into from
May 14, 2019

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Feb 21, 2019

Contribution description

This PR is factorizing boards configuration of frdm-kw41z, usb-kw41z and phynode-kw41z boards. There are sharing the same clock/uart/spi/i2c configurations so this PR is removing a lot of duplicated code.

Testing procedure

frdm-kw41z, usb-kw41z, phynode-kw41z should still work.

Issues/PRs references

Should help with PR adding support of kw41z boards (#10846 for example).

@aabadie aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: boards Area: Board ports labels Feb 21, 2019
@aabadie aabadie requested review from jnohlgard and jia200x February 21, 2019 09:26
@aabadie
Copy link
Contributor Author

aabadie commented Feb 21, 2019 •
edited
Loading

Briefly tested examples/default on frdm-kw41z and usb-kw41z and it still work. I don't have phynode-kw41z.

@aabadie aabadie force-pushed the pr/boards/kw41z-common branch from e4e019b to 35ebf10 Compare February 21, 2019 10:08
@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 21, 2019
@aabadie
Copy link
Contributor Author

aabadie commented Feb 26, 2019

@gebart, @jia200x, ping

@aabadie aabadie force-pushed the pr/boards/kw41z-common branch from 35ebf10 to 36d5676 Compare February 27, 2019 20:50
@aabadie aabadie force-pushed the pr/boards/kw41z-common branch from 36d5676 to c01cd29 Compare March 20, 2019 13:42
@fjmolinas fjmolinas self-requested a review April 19, 2019 12:10
@fjmolinas fjmolinas 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 and removed 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 Apr 19, 2019
fjmolinas
fjmolinas requested changes Apr 19, 2019
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

In general this looks good but I have problems with the common periph_conf files.

Unless wired internally to an unboard driver i2c, uart and spi depends on external pins, these are not the same for every board.

Eg for usb-k241z your are defining two spi. SPI1 is actually only internally connected to the K22 on board and SPI0 is not exposed or connected to any spi interface. The i2c interface for the same board isn't exposed either, only internally connected to the on board K22.

If we look at frdm-k241z many more pins are exposed and therefore options to connect I2C, UART or SPI devices.

IMO in periph_conf_common.h you can leave the common clock configuration but everything else need its separate configuration file.

PS: also it would be interesting to add the option of having two CPU's on a same board, but that is a completely different subject.

@aabadie
Copy link
Contributor Author

aabadie commented Apr 23, 2019

Unless wired internally to an unboard driver i2c, uart and spi depends on external pins, these are not the same for every board.

I checked again and you are right. I'll update this PR with a new common default SPI configuration, only used by phynode and usb-kw41z. frdm-kw41z will use it's own spi configuration (with 2 spi configured).

@aabadie aabadie force-pushed the pr/boards/kw41z-common branch 2 times, most recently from 1194dab to 6d0422b Compare April 29, 2019 09:35
@aabadie
Copy link
Contributor Author

aabadie commented Apr 29, 2019

@fjmolinas, I reworked this PR following your suggestions. It should be much cleaner now.

@fjmolinas
Copy link
Contributor

@aabadie I'm good with the changes. I'll test on frdm-kw41z & usb-kw41z (all tests) before giving ACK.

@aabadie aabadie force-pushed the pr/boards/kw41z-common branch 2 times, most recently from 34e9cad to d846e36 Compare April 30, 2019 13:44
@fjmolinas fjmolinas mentioned this pull request May 7, 2019
Merged
@fjmolinas
Copy link
Contributor

@aabadie I tested it on usb-kw41z and frdm-kw41z. In general most expected to pass tests are succeeding (except periph_timer witch is fixed in masters).
I do have a failing test in ps_schedstatistics (not sure why yet) but this is also failing in master, I wan't to look into it a bit before ACK but I won't block the PR because of that. Also detected #11493 while running tests. Also, can you rebase?

@aabadie aabadie force-pushed the pr/boards/kw41z-common branch from d846e36 to 30ea848 Compare May 7, 2019 14:20
@aabadie
Copy link
Contributor Author

aabadie commented May 7, 2019

Thanks @fjmolinas, I rebased the branch and fixed some conflicts.

@aabadie
Copy link
Contributor Author

aabadie commented May 7, 2019

It seems like Murdock is stuck since yesterday. @kaspar030 any idea what's going on ?

@aabadie aabadie force-pushed the pr/boards/kw41z-common branch from 30ea848 to 8bd320b Compare May 7, 2019 19:57
@aabadie
Copy link
Contributor Author

aabadie commented May 14, 2019

@fjmolinas, do you finally ACK this one ?

@fjmolinas
Copy link
Contributor

@aabadie Sorry fo the delay, I wanted to perform a final round of testing. There are a bunch of failing tests all related to wrong behaviour with xtimer, but this has been flagged in other ISSUES or PR so ACK.

@fjmolinas fjmolinas merged commit 6b8bbd7 into RIOT-OS:master May 14, 2019
@aabadie aabadie deleted the pr/boards/kw41z-common branch May 20, 2019 06:10
@cladmi cladmi mentioned this pull request Jun 3, 2019
Closed
@aabadie aabadie mentioned this pull request Jun 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@jnohlgard jnohlgard Awaiting requested review from jnohlgard

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

Assignees
No one assigned
Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aabadie @fjmolinas