-
Notifications
You must be signed in to change notification settings - Fork 8k
Enable nxp MICFIL driver and Dialog DA7212 driver #96367
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
Closed
ZhaoxiangJin
wants to merge
10
commits into
zephyrproject-rtos:main
from
nxp-upstream:enable-nxp-pdm
Closed
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
611f84a
dts: bindings: add dialog vendor prefix
ZhaoxiangJin 3432318
drivers: audio: Add dialog7212 driver
ZhaoxiangJin 6f15a7f
boards: frdm_mcxn236: Enable da7212 on frdm_mcxn236
ZhaoxiangJin d7fc5b1
samples: i2s_codec: Refine sample for samll RAM platforms
ZhaoxiangJin ffa63a5
samples: i2s_codec: enable i2s_codec samples for frdm_mcxn236
ZhaoxiangJin af1fa22
audio: dmic: Format dmic.h
ZhaoxiangJin e1b8489
drivers: audio: Add NXP MICFIL driver
ZhaoxiangJin a2a730b
drivers: mcux_syscon: enable MICFIL clock control
ZhaoxiangJin d5e9f21
board: frdm_mcxn236: Enable MICFIL on frdm_mcxn236
ZhaoxiangJin 2c1449a
samples: i2s_codec: Enable nxp micfil test
ZhaoxiangJin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
# Copyright 2025 NXP | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
config AUDIO_CODEC_DA7212 | ||
bool "Dialog DA7212 codec support" | ||
default y | ||
select I2C | ||
depends on DT_HAS_DIALOG_DA7212_ENABLED | ||
help | ||
Enable support for the Dialog DA7212 audio codec. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Copyright 2025 NXP | ||
# | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
config AUDIO_DMIC_NXP_MICFIL | ||
bool "NXP MICFIL driver" | ||
default y | ||
depends on DT_HAS_NXP_MICFIL_ENABLED | ||
select HAS_MCUX | ||
select CLOCK_CONTROL | ||
help | ||
Enable NXP MICFIL driver. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please refer everywhere as DMIC PDM to avoid confusion with MICFIL PDM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed the driver to micfil, please review |
||
if AUDIO_DMIC_NXP_MICFIL | ||
|
||
config DMIC_NXP_MICFIL_QUEUE_SIZE | ||
int "Message queue depth" | ||
default 4 | ||
help | ||
Depth of the message queue used to pass filled buffers to the app. | ||
|
||
endif # AUDIO_DMIC_NXP_MICFIL |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Please add a link to the reference manual so reviewers can analyze the hardware IP.
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.
I will add it when i back to the office, but i'd like to know where the link should be added? I noticed that you made a comment in the CMakeLists file, so do I need to put link in the CMakeLists file? This seems rather strange.
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 link should be added in the commit message.
Also, this PR is a mix of everything adding a codec, adding a dmic driver, enhancements to samples, whitespace fixes to DMIC.
This is a nightmare to review. Let's pause a bit and organize everything in a more logical way so that we can have good code quality and have meaningful reviews.
Let's split this into multiple PRs like this:
PR1: Add support for Dialog7212 codec driver
This will include following patches:
ffa63a5 samples: i2s_codec: enable i2s_codec samples for frdm_mcxn236
6f15a7f boards: frdm_mcxn236: Enable da7212 on frdm_mcxn236
3432318 drivers: audio: Add dialog7212 driver
611f84a dts: bindings: add dialog vendor prefix
PR2: Small enhancements to audio
d7fc5b1 samples: i2s_codec: Refine sample for samll RAM platforms
af1fa22 audio: dmic: Format dmic.h
PR3: Add NXP DMIC MICFIL support
1449a5ed4 samples: i2s_codec: Enable nxp micfil test
d5e9f21 board: frdm_mcxn236: Enable MICFIL on frdm_mcxn236
a2a730b drivers: mcux_syscon: enable MICFIL clock control
e1b8489 drivers: audio: Add NXP MICFIL driver
This 3 PRs cand be sent separately are independent and can be reviewed in parallel. We go merge PR2 very fast.
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.
One thing to be careful of is the "refine sample for small RAM platforms".
It is unclear to me which set of enablement needed this fix. If it was needed for the da7212 enablement then put it before that set of commits. If needed for the MICFIL support then it is in the right place based on the recommendation.
The high-level guidance here is that each commit needs to stand on its own and be bisectable. There should be a logical grouping of commits. And don't overload a PR with a bunch of significant enablement because of the challenge it puts on the reviewers.
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.
"refine sample for small RAM platforms" should placed before i2s_codec example enablement for DA7212 and MICFIL, that is before ffa63a5 and 2c1449a.
From a technical/logical perspective, the commit sequence in this PR is well-structured and bisectable:
However, I recognize my oversight regarding reviewer burden. I will split this PR, please move your comments to new PRs