-
Notifications
You must be signed in to change notification settings - Fork 8k
boards: add steelseries apexpromini #95905
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this board has hwmv1 remnants that need to go
# Enable HW stack protection | ||
CONFIG_HW_STACK_PROTECTION=y | ||
|
||
# enable uart driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capitalise first letters of comments
# enable USB | ||
CONFIG_USB_DEVICE_STACK_NEXT=y | ||
CONFIG_CDC_ACM_SERIAL_INITIALIZE_AT_BOOT=y | ||
CONFIG_USBD_CDC_ACM_LOG_LEVEL_OFF=y | ||
|
||
# enable input keyboard matrix | ||
CONFIG_INPUT=y | ||
CONFIG_INPUT_KEYMAP=y | ||
CONFIG_INPUT_GPIO_KBD_MATRIX=y | ||
# Required because rows are actually columns | ||
CONFIG_INPUT_KBD_MATRIX_16_BIT_ROW=y | ||
|
||
# enable spi flash | ||
CONFIG_FLASH=y | ||
CONFIG_SPI_NOR_FLASH_LAYOUT_PAGE_SIZE=4096 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none of this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it does not have a debug LED or an UART port, it just has a USB port and some keys, so it feels weird to let them be optional features. But maybe they should be anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it has USB then CDC ACM as the console by default is ok, the rest though is not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this board, USB will not work without CDC_ACM_SERIAL_INITIALIZE_AT_BOOT, and KBD_MATRIX will not work without INPUT_KBD_MATRIX_16_BIT_ROW. Is there a wait to enforce or to document those dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the USB ones can stay, the kbd one can be done in Kconfig.defconfig
a pseudo example:
if BLAH
config BLAH_OTHER
default SOMETHING
endif # BLAH
2e7ebe0
to
72afde9
Compare
a18ff01
to
249117c
Compare
@nashif maybe you can help review since you had your opinions about vendor-prefixes.txt being so important |
}; | ||
|
||
&kbd { | ||
keymap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can put this straight under the kbd node on line 69 if you want to, no point shuffling nodes around
249117c
to
91926b4
Compare
.. _apexpromini_board: | ||
|
||
SteelSeries ApexProMini | ||
####################### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
####################### | |
.. zephyr:board:: apexpromini |
name: apexpromini | ||
full_name: SteelSeries Apex Pro Mini |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it does look like official product name is indeed "Apex Pro Mini" (with spaces) so please use the same spelling everywhere.
Also, given there are spaces, i think apex_pro_mini
might be a better suited board name?
Finally, vendor name can be ommited in the board "full name"
name: apexpromini | |
full_name: SteelSeries Apex Pro Mini | |
name: apex_pro_mini | |
full_name: Apex Pro Mini |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you check line wrapping in this file and make it consistent? there's a mix of very aggressive (< 80 characters) and no wrapping at all currently :) 100 characters per line would be good. Thanks!
afb72ed
to
5825a2e
Compare
if INPUT_GPIO_KBD_MATRIX | ||
|
||
config INPUT_KBD_MATRIX_16_BIT_ROW | ||
default y | ||
|
||
endif # INPUT_GPIO_KBD_MATRIX | ||
|
||
if FLASH | ||
|
||
config SPI_NOR_FLASH_LAYOUT_PAGE_SIZE | ||
default 4096 | ||
|
||
endif # FLASH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no indentation on Kconfig files, just bring the block one tab back
config INPUT_KBD_MATRIX_16_BIT_ROW | ||
default y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since there's only one option in the if block you can change this to
default y if INPUT_GPIO_KBD_MATRIX
keep the whole thing compact, same for the flash option below
flash1: fm25q128c@0 { | ||
compatible = "jedec,spi-nor"; | ||
reg = <0>; | ||
spi-max-frequency = <10000000>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use DT_FREQ_M
fe8ac99
to
38569bf
Compare
default y | ||
depends on SPI | ||
|
||
config INPUT_KBD_MATRIX_16_BIT_ROW | ||
default y if INPUT_GPIO_KBD_MATRIX | ||
|
||
config SPI_NOR_FLASH_LAYOUT_PAGE_SIZE | ||
default 4096 if FLASH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I realize my comment on this was misleading :-) what I mean is that in Kconfig we don't indent the block inside the "if", not that we don't indent at all, so in this case the lines under config
(deulfat y
, depends on
SPI`) should be indented by one tab
Look at other files maybe, I'm clearly pretty bad at explaining it
38569bf
to
dfdcd04
Compare
dfdcd04
to
d270d07
Compare
the latest push I made to the branch should fix the doc issues :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for docs!
, <&gpioc 2 GPIO_ACTIVE_HIGH> | ||
, <&gpioc 3 GPIO_ACTIVE_HIGH> | ||
; | ||
keymap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline missing on line 69
853fb34
d270d07
to
853fb34
Compare
@everedero you need to add my earlier fix back please :) |
Initial support for the Apex Pro Mini keyboard, based on STM32L412. Signed-off-by: Eve Redero <eve.redero@gmail.com>
853fb34
to
1eb32b0
Compare
|
Assigning to @fabiobaltieri due to this new board having a strong focus on Input subsystem stuff |
Initial support for the Apex Pro Mini keyboard,
based on STM32L412.