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/nrf9160: add Kconfig dependencies #17291

Merged
merged 1 commit into from
Dec 2, 2021
Merged

cpu/nrf9160: add Kconfig dependencies #17291

merged 1 commit into from
Dec 2, 2021

Conversation

fjmolinas
Copy link
Contributor

Contribution description

These models modules for nrf9160 CPU and the only BOARD using it.

Testing procedure

  • Green CI
  • Module list between makefile and Kconfig should match.

Issues/PRs references

Part of #16875

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 29, 2021
@github-actions github-actions bot added Area: CI Area: Continuous Integration of RIOT components Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration labels Nov 29, 2021
@fjmolinas fjmolinas mentioned this pull request Nov 29, 2021
Closed
dylad
dylad reviewed Nov 29, 2021
View reviewed changes
.murdock Outdated
@@ -26,6 +26,7 @@ mega-xplained
microbit
native
nrf52840dk
nrf59160
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nrf59160
nrf9160

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ups

Copy link
Member

Choose a reason for hiding this comment

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

I guess I was still asleep this morning...
Should be nrf9160dk...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@kaspar030 kaspar030 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2021
@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2021
@fjmolinas fjmolinas 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 Dec 1, 2021
leandrolanzieri
leandrolanzieri reviewed Dec 1, 2021
View reviewed changes
cpu/nrf9160/Kconfig
@@ -37,4 +37,6 @@ config HAS_CPU_NRF9160
help
Indicates that the current cpu is 'nrf9160'.

rsource "vectors/Kconfig"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are missing: source "$(RIOTCPU)/nrf5x_common/Kconfig". As that one already includes the cortexm_common Kconfig you can remove it from here.

Copy link
Member

Choose a reason for hiding this comment

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

@leandrolanzieri The problem here is that nRF9160 doesn't fully reuse nrf5x_common for now because some work is still needed on the driver side. That's why this CPU defines by itself which features it has instead of relying on nrf5x_common.
If this is problem, I can have a look to fix the remaining issues with the shared drivers first, then let nRF9160 fully relies on nrf5x_common. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

But if we don't select CPU_COMMON_NRF5X its features should not be provided, right? This is currently failing because MODULE_NRF5X_COMMON_PERIPH is missing (defined in cpu/nrf5x_common/periph/Kconfig.nrf5x)

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 1, 2021
leandrolanzieri
leandrolanzieri reviewed Dec 1, 2021
View reviewed changes
.murdock Outdated Show resolved Hide resolved
@fjmolinas
Copy link
Contributor Author

Its green now, should I squash @leandrolanzieri @dylad?

@dylad
Copy link
Member

dylad commented Dec 1, 2021

Go ahead !

dylad
dylad approved these changes Dec 2, 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.

LGTM.

@dylad
Copy link
Member

dylad commented Dec 2, 2021

Here we go !

@dylad dylad merged commit 97f7b67 into RIOT-OS:master Dec 2, 2021
@fjmolinas fjmolinas deleted the pr_nrf9160_kconfig branch December 2, 2021 09:07
@fjmolinas
Copy link
Contributor Author

Thanks!

@fjmolinas fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri left review comments

@dylad dylad dylad approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

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

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

Assignees
No one assigned
Labels
Area: CI Area: Continuous Integration of RIOT components Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2022.01
Development

Successfully merging this pull request may close these issues.
4 participants
@fjmolinas @dylad @leandrolanzieri @kaspar030