Skip to content
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

Kconfig: Create Kconfig framework for mcux-sdk #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Kconfig: Create Kconfig framework for mcux-sdk #42

wants to merge 1 commit into from

Conversation

mcuxsusan
Copy link
Contributor

Signed-off-by: Susan Su susan.su@nxp.com

Prerequisites

  • I have checked latest main branch and the issue still exists.
  • I did not see it is stated as known-issue in release notes.
  • No similar GitHub issue is related to this change.
  • My code follows the commit guidelines of this project.
  • I have performed a self-review of my own code.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.

Describe the pull request

A clear and concise description for the change in this Pull Request and which issue is fixed.

Fixes # (issue)

Type of change (please delete options that are not relevant):

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Tests

  • Test configuration (please complete the following information):
    • Hardware setting:
    • Toolchain:
    • Test Tool preparation:
    • Any other dependencies:
  • Test executed
    Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
    • Build Test
    • Run Test

@FelixWang47831 @mcuxsusan
Kconfig: Create Kconfig framework for mcux-sdk
9795c43 
Signed-off-by: Susan Su <susan.su@nxp.com>
@github-actions
Copy link

github-actions bot commented Oct 9, 2021

We do not accept contributions to files under CMSIS, these files are sourced from https://github.com/ARM-software/CMSIS_5, please contribute to the upstream directly.

@github-actions github-actions bot closed this Oct 9, 2021
@mcuxsusan mcuxsusan reopened this Oct 9, 2021
@mcuxsusan
Copy link
Contributor Author

@FelixWang47831, for your information, since your previous commit change in #14 and #38 are all included in this new PR, I will close previous PR.

Paste Felix's suggestion for how to use the kconfig framework.
How to use:

Install kconfig lib(Python version compatibility: 2.7/3.2+): pip(3) install kconfiglib
cd mcuxsdk/core, run "guiconfig", click save button after select software components
cd project folder, run build scripts.

This was referenced Oct 9, 2021
Closed
Closed
@mcuxsusan
Copy link
Contributor Author

Review comments from @dleach02, the kconfig.xx needs to be updated to "Kconfig.xx"

@dleach02
Copy link

dleach02 commented Oct 14, 2021 •
edited
Loading

Review comments from @dleach02, the kconfig.xx needs to be updated to "Kconfig.xx"

I will also add that if this is intended for future integration with Zephyr that Zephyr appears to have a style to keep the Kconfig variable names all CAPS. We should follow that implicit style.

dleach02
dleach02 suggested changes Oct 14, 2021
View reviewed changes
CMSIS/kconfig.CMSIS
@@ -0,0 +1,115 @@
# Copyright 2021 NXP
Copy link

Choose a reason for hiding this comment

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

USE all caps for Kconfig variables.

dleach02
dleach02 reviewed Oct 14, 2021
View reviewed changes
CMSIS/kconfig.CMSIS

menu "CMSIS"

config USE_CMSIS_Include_core_cm
Copy link

Choose a reason for hiding this comment

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

You should keep the symmetry of of the variable names. So we start with "HAS_MCUX_SDK_" then on the enablement use "MCU_SDK_". This clarifies the relationship.

Also the "USE" verb is not a style within Zephyr so I would not apply it here. Just stick with "MCU_SDK_" as the enablement flag.

dleach02
dleach02 reviewed Oct 14, 2021
View reviewed changes
devices/K32L2A41A/all_lib_device_K32L2A41A.cmake
@@ -49,68 +49,68 @@ list(APPEND CMAKE_MODULE_PATH


# Copy the cmake components into projects
Copy link

Choose a reason for hiding this comment

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

Why can't there be a generic top level cmake file for the drivers and components instead of one per device?

If you look at how Zephyr does this, there is a generic "HAS_MCUX_" then a generic "MCUX_" to enable it. The cmake is then at a higher level keeping the lower level device stuff to be very specific to the device.

dleach02
dleach02 reviewed Oct 14, 2021
View reviewed changes
components/kconfig.components
menu "codec"

depends on HAS_MCUX_SDK_component_ak4497_adapter || HAS_MCUX_SDK_component_codec_adapters || HAS_MCUX_SDK_component_cs42448_adapter || HAS_MCUX_SDK_component_cs42888_adapter || HAS_MCUX_SDK_component_da7212_adapter || HAS_MCUX_SDK_component_tfa9896_adapter || HAS_MCUX_SDK_component_tfa9xxx_adapter || HAS_MCUX_SDK_component_wm8524_adapter || HAS_MCUX_SDK_component_wm8904_adapter || HAS_MCUX_SDK_component_wm8960_adapter || HAS_MCUX_SDK_component_codec_i2c_LPC51U68 || HAS_MCUX_SDK_component_codec_i2c_LPC54114_cm4 || HAS_MCUX_SDK_component_codec_i2c_LPC54628 || HAS_MCUX_SDK_component_codec_i2c_LPC54S018 || HAS_MCUX_SDK_component_codec_i2c_LPC54S018M || HAS_MCUX_SDK_component_codec_i2c_LPC55S16 || HAS_MCUX_SDK_component_codec_i2c_LPC55S28 || HAS_MCUX_SDK_component_codec_i2c_LPC55S69_cm33_core0 || HAS_MCUX_SDK_component_codec_i2c_MCIMX7U5 || HAS_MCUX_SDK_component_codec_i2c_MIMX8ML8 || HAS_MCUX_SDK_component_codec_i2c_MIMX8MM6 || HAS_MCUX_SDK_component_codec_i2c_MIMX8QM6_cm4_core0 || HAS_MCUX_SDK_component_codec_i2c_MIMX8QM6_cm4_core1 || HAS_MCUX_SDK_component_codec_i2c_MIMX8QX6 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1011 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1015 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1021 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1024 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1052 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1062 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1064 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1166_cm4 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1166_cm7 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1176_cm4 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT1176_cm7 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT595S_cm33 || HAS_MCUX_SDK_component_codec_i2c_MIMXRT685S_cm33 || HAS_MCUX_SDK_component_codec_i2c_MK66F18 || HAS_MCUX_SDK_component_codec_ak4497_adapter || HAS_MCUX_SDK_component_codec_wm8524_adapter

Copy link

Choose a reason for hiding this comment

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

This is a bit extreme of a "depends". Is it really necessary?

dleach02
dleach02 reviewed Oct 14, 2021
View reviewed changes
devices/kconfig.devices
default n
select USE_device_CMSIS if HAS_MCUX_SDK_device_CMSIS

config USE_device_startup_K32L3A60_cm0plus
Copy link

Choose a reason for hiding this comment

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

What is this "device_startup" for?

dleach02
dleach02 reviewed Oct 14, 2021
View reviewed changes
devices/kconfig.devices
default n
select USE_device_system_K32L3A60_cm4 if HAS_MCUX_SDK_device_system_K32L3A60_cm4

config USE_device_system_K32L3A60_cm0plus
Copy link

Choose a reason for hiding this comment

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

What is "device_system" for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dleach02 dleach02 dleach02 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mcuxsusan @dleach02 @FelixWang47831