Skip to content

Conversation

@benpicco
Copy link
Contributor

Make sure that when e.g. CONFIG_SERIAL is set, CONFIG_UART_SAM0 is
selected automatically when the sam0 SoC family is used.

This fixes several examples & tests that will otherwise only compile after manually tweaking the configuration.

@codecov-io
Copy link

Codecov Report

Merging #14832 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14832   +/-   ##
=======================================
  Coverage   52.51%   52.51%           
=======================================
  Files         309      309           
  Lines       45048    45048           
  Branches    10419    10419           
=======================================
  Hits        23656    23656           
  Misses      16584    16584           
  Partials     4808     4808

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef9c7a4...0cfe611. Read the comment docs.

@galak
Copy link
Contributor

galak commented Mar 22, 2019

We should open an enhancement request to cleanup the boards to remove enabling any peripherals by default that aren't needed.

@ulfalizer
Copy link
Contributor

ulfalizer commented Mar 22, 2019

Why don't we do e.g. this instead? Should work out to the same thing in practice, unless I'm missing something, and it centralizes SAM0 stuff.

diff --git a/drivers/serial/Kconfig.sam0 b/drivers/serial/Kconfig.sam0
index da3a5f4209..75aa63aa3c 100644
--- a/drivers/serial/Kconfig.sam0
+++ b/drivers/serial/Kconfig.sam0
@@ -5,6 +5,7 @@
 
 menuconfig UART_SAM0
        bool "Atmel SAM0 series SERCOM USART driver"
+       default y
        depends on SOC_FAMILY_SAM0
        select SERIAL_HAS_DRIVER
        select SERIAL_SUPPORT_INTERRUPT

(Unrelated side note: Avoid the menuconfig keyword when there's no symbols following the symbol that depend on it. Looks wonky in e.g. the menuconfig interface.)

@ulfalizer
Copy link
Contributor

ulfalizer commented Mar 22, 2019

Another option is to enable them in the board defconfig file. Might be cleanest of all.

Drawback is having to enable the dependencies too, if they're not default y.

@benpicco
Copy link
Contributor Author

@ulfalizer that would always enable UART_SAM0, no matter if CONFIG_SERIAL is set or not.

Maybe depends on SOC_FAMILY_SAM0 && CONFIG_SERIAL wold work. But I was just copying what stm32 was doing.

@ulfalizer
Copy link
Contributor

ulfalizer commented Mar 22, 2019

@benpicco
It shouldn't, because drivers/serial/Kconfig.sam0 is sourced within an if SERIAL, in drivers/serial/Kconfig.

I added some clarifications to how if works in the guide btw.

@benpicco
Copy link
Contributor Author

benpicco commented Mar 22, 2019

Ah I did no see that! In this case, I might as well enable it there.
I'm feeling tempted to just enable the peripherals for all SoCs then.

Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

LGTM

@ulfalizer
Copy link
Contributor

@benpicco
Can see all the dependencies on the reference pages for symbols btw. Shows how the file got sourced too.

The same information is shown in the menuconfig too, in the symbol information (press ?).

(Trying to plug the menuconfig, because it's handy for checking changes, even though many people don't use it otherwise.)

Make sure that when e.g. CONFIG_SERIAL is set, CONFIG_UART_SAM0 is
selected automatically when the sam0 SoC family is used.

Signed-off-by: Benjamin Valentin <[email protected]>
@galak galak force-pushed the sam0_autoselect branch from a18506f to 36c25f3 Compare April 19, 2019 19:03
@galak galak merged commit 4d9486f into zephyrproject-rtos:master Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants