Skip to content

cpu/nrf9160: add Kconfig dependencies#17291

Merged
dylad merged 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_nrf9160_kconfig
Dec 2, 2021
Merged

cpu/nrf9160: add Kconfig dependencies#17291
dylad merged 1 commit intoRIOT-OS:masterfrom
fjmolinas:pr_nrf9160_kconfig

Conversation

@fjmolinas
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas commented Nov 29, 2021

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
microbit
native
nrf52840dk
nrf59160
Copy link
Copy Markdown
Member

@dylad dylad Nov 29, 2021

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
Copy Markdown
Contributor Author

@fjmolinas fjmolinas Nov 29, 2021

Choose a reason for hiding this comment

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

ups

Copy link
Copy Markdown
Member

@dylad dylad Nov 29, 2021

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
Copy Markdown
Contributor Author

@fjmolinas fjmolinas Nov 30, 2021

Choose a reason for hiding this comment

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

done

@kaspar030 kaspar030 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 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
Indicates that the current cpu is 'nrf9160'.

rsource "vectors/Kconfig"

Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri Dec 1, 2021

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
Copy Markdown
Member

@dylad dylad Dec 1, 2021

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
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri Dec 1, 2021

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
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Dec 1, 2021

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

@dylad
Copy link
Copy Markdown
Member

dylad commented Dec 1, 2021

Go ahead !

dylad
dylad approved these changes Dec 2, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

fjmolinas commented Dec 2, 2021

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