Skip to content

Conversation

@jackrosenthal
Copy link
Contributor

This adds support for the EC (embedded controller) on a Google reference board with codename "kukui". This board uses the STM32F098RC chip. We built an application for the board and verified UART functionality on the board.

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.

Some nits related to some recent cleanups.

@zephyrbot
Copy link

zephyrbot commented Nov 11, 2019

All checks are passing now.

checkpatch (informational only, not a failure)

-:175: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "st,stm32f098rc" appears un-documented -- check ./dts/bindings/
#175: FILE: boards/arm/google_kukui/google_kukui.dts:12:
+	compatible = "st,stm32f098rc", "st,stm32f098";

-:175: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "st,stm32f098" appears un-documented -- check ./dts/bindings/
#175: FILE: boards/arm/google_kukui/google_kukui.dts:12:
+	compatible = "st,stm32f098rc", "st,stm32f098";

- total: 0 errors, 2 warnings, 284 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@ulfalizer
Copy link
Contributor

-:80: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "st,stm32f098rc" appears un-documented -- check ./dts/bindings/
#80: FILE: boards/arm/google_kukui/google_kukui.dts:12:

  • compatible = "st,stm32f098rc", "st,stm32f098";

#20289 would've helped here, but it doesn't seem to be in yet.

@jackrosenthal
Copy link
Contributor Author

-:80: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string "st,stm32f098rc" appears un-documented -- check ./dts/bindings/
#80: FILE: boards/arm/google_kukui/google_kukui.dts:12:

  • compatible = "st,stm32f098rc", "st,stm32f098";

#20289 would've helped here, but it doesn't seem to be in yet.

Should I do anything to resolve this warning, or leave as is?

@ulfalizer
Copy link
Contributor

ulfalizer commented Nov 12, 2019

Should I do anything to resolve this warning, or leave as is?

Could leave as is.

Another option would be to mention those compatibles in a comment in a binding just to make the check happy (checkpatch.pl is just grepping for the strings internally), and use variant-compatible later if we remember.

Not super important at least.

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.

Nits fixed.

@jackrosenthal jackrosenthal force-pushed the master branch 2 times, most recently from a5f5dc2 to d4d87ae Compare November 12, 2019 00:48
@erwango erwango added the platform: STM32 ST Micro STM32 label Nov 12, 2019
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Thanks for this addition!
Additionally to the comments on the files, please take care to the 3 following points:

  • Please add board documentation
  • Please rename first commit to soc: ... since you're actually touching soc sbsystem, not dt
  • Please add a dts related to cmmit to add the missing soc dtsi files

Copy link
Contributor Author

@jackrosenthal jackrosenthal left a comment

Choose a reason for hiding this comment

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

Thanks for this addition!
Additionally to the comments on the files, please take care to the 3 following points:

  • Please add board documentation

Done. Apologies that the documentation may be a little slim, the board is not released yet and I'm not able to provide photos, etc.

  • Please rename first commit to soc: ... since you're actually touching soc sbsystem, not dt

Done

  • Please add a dts related to cmmit to add the missing soc dtsi files

Done. It's equivalent to STM32F091xC, but I copied the files.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Some more comments.
Also, please add yourself in front of this board in CODEOWNER file

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

One remaining comment on .dtsi package variant file name.
Otherwise this is fine.

This adds a Kconfig options and device tree configs for the STM32F098
series of SoC.

Signed-off-by: Jack Rosenthal <[email protected]>
This adds support for the EC (embedded controller) on a Google
reference board with codename "kukui". This board uses the STM32F098RC
chip. We built an application for the board and verified UART
functionality on the board.

Signed-off-by: Jack Rosenthal <[email protected]>
Signed-off-by: Simon Glass <[email protected]>
@erwango erwango requested a review from dbkinder November 21, 2019 08:04
@erwango erwango added this to the v2.2.0 milestone Nov 21, 2019
@jackrosenthal
Copy link
Contributor Author

What steps remain in getting this merged?

@erwango
Copy link
Member

erwango commented Nov 22, 2019

@jackrosenthal, we're in the process of releasing v2.1, so merge window is frozen for now. New one should be open in a couple of weeks.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

+1 for docs, thanks.

@nashif nashif merged commit a07acbd into zephyrproject-rtos:master Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants