Skip to content

boards: arm: Add adafruit_kb2040 board #51098

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
Nov 3, 2022
Merged

boards: arm: Add adafruit_kb2040 board #51098

merged 4 commits into from
Nov 3, 2022

Conversation

petejohanson
Copy link
Contributor

Add adafruit_kb2040 board with sparkfun,pro-micro header.

Signed-off-by: Peter Johanson [email protected]

@zephyrbot zephyrbot added the area: Devicetree Binding PR modifies or adds a Device Tree binding label Oct 8, 2022
yonsch
yonsch requested changes Oct 9, 2022
View reviewed changes
Copy link
Collaborator

@yonsch yonsch left a comment

Choose a reason for hiding this comment

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

A few nitpicks. Also, if the board.cmake file is not used (since there's currently no west configuration for it as west does not support MSC), I think it can be omitted. Regarding the sparkfun_pro_micro - I see there's a board of that name. Is it also a name of a standard pinout? Or does this port support both boards? Maybe the file should be renamed to sparkfun_pro_micro_connector.dtsi?

@yonsch yonsch added the platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) label Oct 9, 2022
@petejohanson petejohanson force-pushed the boards/adafruit-kb2040-upstream branch 3 times, most recently from 5f6a6fc to 1bf4efd Compare October 13, 2022 04:54
@petejohanson
Copy link
Contributor Author

A few nitpicks. Also, if the board.cmake file is not used (since there's currently no west configuration for it as west does not support MSC), I think it can be omitted. Regarding the sparkfun_pro_micro - I see there's a board of that name. Is it also a name of a standard pinout? Or does this port support both boards? Maybe the file should be renamed to sparkfun_pro_micro_connector.dtsi?

It's a board, but one that's AVR, so won't ever run Zephyr. Renamed the include for completeness. We use the generic header/interconnect extensively in ZMK since there are so many keyboards that use the pro micro footprint.

Thanks for the thorough review!

@petejohanson petejohanson requested review from yonsch and removed request for nashif, MaureenHelm and galak October 13, 2022 04:56
@yonsch
Copy link
Collaborator

yonsch commented Oct 13, 2022

LGTM, will approve when CI passes

@petejohanson petejohanson force-pushed the boards/adafruit-kb2040-upstream branch from 1bf4efd to 0366b9d Compare October 14, 2022 03:21
@zephyrbot zephyrbot requested a review from galak October 14, 2022 03:22
@petejohanson petejohanson force-pushed the boards/adafruit-kb2040-upstream branch 5 times, most recently from 8ffe65d to 15e4be9 Compare October 14, 2022 05:09
@zephyrbot zephyrbot added the area: ADC Analog-to-Digital Converter (ADC) label Oct 14, 2022
@zephyrbot zephyrbot requested a review from anangl October 14, 2022 05:09
@petejohanson
Copy link
Contributor Author

@yonsch Found and fixed the remaining CI issues.

yonsch
yonsch previously approved these changes Nov 2, 2022
View reviewed changes
@yonsch yonsch requested a review from soburi November 2, 2022 07:08
henrikbrixandersen
henrikbrixandersen requested changes Nov 2, 2022
View reviewed changes
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

Looks good. However, please split out the commits. The new DTS binding, which is not specific to the board, should go into its own commit. Same goes for the change to the ADC test.

boards/arm/adafruit_kb2040/adafruit_kb2040_defconfig Outdated
CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=125000000

# Enable GPIO
CONFIG_GPIO=y
Copy link
Member

Choose a reason for hiding this comment

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

Is GPIO needed for basic operation of this board? If not, GPIO should not be enabled by default.

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, but I'm curious, is this really applied consistently across Zephyr? I see tons of boards with this enabled by default. What triggers this being enabled or not by default? Onboard GPIO keys/buttons? Something else?

Copy link
Member

Choose a reason for hiding this comment

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

If the board requires GPIO in order to boot and use the console (e.g. if it has a GPIO controlled regulator, which needs to be turned on), GPIO should be enabled by default. Otherwise not. And yet, we do have a lot of boards enabling GPIO by default where it is not needed.

@petejohanson
dts: bindings: vendor-prefixes: add SparkFun
e84004c 
Add SparkFun (https://www.sparkfun.com/) vendor prefix.

Signed-off-by: Peter Johanson <[email protected]>
@petejohanson
dts: bindings: gpio: Add SparkFun Pro Micro header
a40a1c8 
Add SparkFun Pro Micro header connector that is implemented by many
other controllers. This allows hardware with compatible headers to
define the related GPIOs and peripherals.

Signed-off-by: Peter Johanson <[email protected]>
@petejohanson
boards: arm: Add adafruit_kb2040 board
aa93ecb 
Add `adafruit_kb2040` board with `sparkfun,pro-micro` header.

Signed-off-by: Peter Johanson <[email protected]>
@petejohanson
drivers: adc: Fix ADC tests for new RP2040 board.
de7223a 
Fix ADC tests now that we have more than one RP2040 board.

Signed-off-by: Peter Johanson <[email protected]>
@petejohanson petejohanson force-pushed the boards/adafruit-kb2040-upstream branch from 15e4be9 to de7223a Compare November 3, 2022 15:51
@petejohanson
Copy link
Contributor Author

@henrikbrixandersen Split the commits, including an extra one just for the new vendor prefix for SparkFun as well.

@petejohanson petejohanson requested review from henrikbrixandersen and soburi and removed request for galak, anangl, henrikbrixandersen and soburi November 3, 2022 15:53
@carlescufi carlescufi merged commit 29047f1 into zephyrproject-rtos:main Nov 3, 2022
@soburi soburi mentioned this pull request Jan 16, 2023
Open
52 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soburi soburi soburi approved these changes

@henrikbrixandersen henrikbrixandersen henrikbrixandersen approved these changes

@yonsch yonsch yonsch left review comments

Assignees
No one assigned
Labels
area: ADC Analog-to-Digital Converter (ADC) area: Devicetree Binding PR modifies or adds a Device Tree binding platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@petejohanson @yonsch @soburi @henrikbrixandersen @carlescufi @zephyrbot