intel_adsp: dmic: Refactoring of blob parsing function #60172
Merged
Conversation
This file contains 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
softwarecki
requested review from
kv2019i,
marc-hb,
tmleman and
serhiy-katsyuba-intel
softwarecki
requested review from
marcinszkudlinski,
abonislawski,
dcpleung,
andyross and
nashif
as code owners
zephyrbot
requested review from
aborisovich,
ceolin,
juimonen,
lgirdwood,
lyakh and
MaureenHelm
softwarecki
requested review from
jxstelter
and removed request for
ceolin,
juimonen,
lyakh,
MaureenHelm,
lgirdwood and
aborisovich
softwarecki
force-pushed
the
dmic-refactor-pr
branch
from
ef37a85 to
2d33c77
Compare
|
Generally looks very nice, good cleanups again. @softwarecki can you make a sof pr with this pr in west so we can do the CI tests? |
tmleman
reviewed
Aug 2, 2023
singalsu
reviewed
Aug 16, 2023
| LOG_INF("OUTCONTROL = %08x", val); | ||
| LOG_INF(" tie=%d, sip=%d, finit=%d, fci=%d", bf1, bf2, bf3, bf4); | ||
| LOG_INF(" bfth=%d, of=%d, ipm=%d, th=%d", bf5, bf6, bf7, bf8); | ||
| if (bf5 > OUTCONTROL_BFTH_MAX) { |
There was a problem hiding this comment.
I agree that using a reserved bit could be just warning. But exceed of a max value like BFTH feels like it should be an error because it results to incorrect operation.
| val0 = dai_dmic_read(dmic, OUTSTAT0); | ||
| val1 = dai_dmic_read(dmic, OUTSTAT1); | ||
| val0 = dai_dmic_read(dmic, OUTSTAT); | ||
| val1 = dai_dmic_read(dmic, OUTSTAT + PDM_CHANNEL_REGS_SIZE); |
There was a problem hiding this comment.
I don't think this is a code improvement. Are you sure?
There was a problem hiding this comment.
Introduction of the PDM_CHANNEL_REGS_SIZE allows to remove a few switches, so I think this is an improvement.
drivers/dai/intel/dmic/dmic.c
Outdated
| @@ -585,8 +571,7 @@ static void dai_dmic_start(struct dai_intel_dmic *dmic) | |||
|
|
|||
| mic_a = dmic->enable[i] & 1; | |||
| mic_b = (dmic->enable[i] & 2) >> 1; | |||
| fir_a = (dmic->enable[i] > 0) ? 1 : 0; | |||
| fir_b = (dmic->enable[i] > 0) ? 1 : 0; | |||
| start_fir = !!(dmic->enable[i] > 0); | |||
There was a problem hiding this comment.
Instead of two variables with the same value, we have one, so this is a simplification. In fact, double negation is unnecessary because the comparison returns a boolean value. I will fix it.
Definitions of a configuration blob structures were separated from the main drivers header file and moved to a dedicated file to improve code readability. Removed unnecessary nhlt_pdm_fir_coeffs structure. The nhlt_pdm_ctrl_cfg structure was extended with nhlt_pdm_ctrl_fir_cfg and fir coefficients. Signed-off-by: Adrian Warecki <[email protected]>
Moved code fragments responsible for logging and verification of the configuration register values from the dai_dmic_set_config_nhlt function to a separate functions. Behavior of the code verifying the correctness of register values has been changed so that it only displays warnings. Signed-off-by: Adrian Warecki <[email protected]>
All PDM controllers have the same set of registers. Their definitions have been merged to simplify the code. Signed-off-by: Adrian Warecki <[email protected]>
Added reading of a necessary register values in dai_nhlt_dmic_dai_params_get function to simplify its parameter list. The code that calculates dai_params has been moved to it to simplify the dai_dmic_set_config_nhlt function. Signed-off-by: Adrian Warecki <[email protected]>
The while loop in the code fragments waiting for a bit to be cleared has been replaced with the WAIT_FOR macro call. Added a warning in the case of timeout. Signed-off-by: Adrian Warecki <[email protected]>
Created dai_dmic_start_fifo_packers function corresponding to an already existing dai_dmic_stop_fifo_packers. Signed-off-by: Adrian Warecki <[email protected]>
All fir filters have an identical set of registers so their definitions were combined to simplify the code. From the dai_dmic_set_config_nhlt function, a duplicate piece of code responsible for configuring fir was separated into a new function. Signed-off-by: Adrian Warecki <[email protected]>
More verbose variable pdm_idx was used instead of n. The series of references to the array of pdm base addresses has been replaced with a pdm_base variable. Signed-off-by: Adrian Warecki <[email protected]>
Added reading of a necessary register values in dai_nhlt_dmic_dai_params_get function to simplify its parameter list. The code that calculates dai_params has been moved to it to simplify the dai_dmic_set_config_nhlt function. Signed-off-by: Adrian Warecki <[email protected]>
Created set of new functions for configure fir coefficients with support for packed format. This allowed to make the dai_dmic_set_config_nhlt function simpler. Signed-off-by: Adrian Warecki <[email protected]>
softwarecki
force-pushed
the
dmic-refactor-pr
branch
from
d3bd045 to
cf7621e
Compare
|
kv2019i
approved these changes
Aug 22, 2023
There was a problem hiding this comment.
Thanks @softwarecki , looks good and test PR also passes. I'd like @singalsu to approve this series before merge, otherwise good to go.
tmleman
approved these changes
Sep 1, 2023
marc-hb
reviewed
Jan 11, 2024
| p_mfir = FIELD_GET(FIR_CONFIG_FIR_DECIMATION, val) + 1; | ||
|
|
||
| rate_div = p_clkdiv * p_mcic * p_mfir; | ||
| LOG_ERR("dai_index = %d, rate_div = %d, p_clkdiv = %d, p_mcic = %d, p_mfir = %d", |
There was a problem hiding this comment.
@softwarecki why are these LOG_ERR()? They seem printed every time:
This delays thesofproject/sof-test#1075 once more...
There was a problem hiding this comment.
@marc-hb oops, it should be LOG_INF
There was a problem hiding this comment.
ops, it should be LOG_INF
Fix submitted: #67567
marc-hb
added a commit
to marc-hb/zephyr
that referenced
this pull request
Jan 12, 2024
Fix the log level of two LOG_ERR() statements which should have always been LOG_INF(). As confirmed by the author Adrian in zephyrproject-rtos#60172 Fixes commit 3fbaed4 ("dai: intel: ace: dmic: Refactor of dai_nhlt_dmic_dai_params_get function") Signed-off-by: Marc Herbert <[email protected]>
carlescufi
pushed a commit
that referenced
this pull request
Jan 15, 2024
Fix the log level of two LOG_ERR() statements which should have always been LOG_INF(). As confirmed by the author Adrian in #60172 Fixes commit 3fbaed4 ("dai: intel: ace: dmic: Refactor of dai_nhlt_dmic_dai_params_get function") Signed-off-by: Marc Herbert <[email protected]>
kv2019i
pushed a commit
to kv2019i/zephyr
that referenced
this pull request
Mar 4, 2024
Fix the log level of two LOG_ERR() statements which should have always been LOG_INF(). As confirmed by the author Adrian in zephyrproject-rtos#60172 Fixes commit 3fbaed4 ("dai: intel: ace: dmic: Refactor of dai_nhlt_dmic_dai_params_get function") Signed-off-by: Marc Herbert <[email protected]> (cherry picked from commit 8042e73)
kv2019i
pushed a commit
to thesofproject/zephyr
that referenced
this pull request
Mar 5, 2024
Fix the log level of two LOG_ERR() statements which should have always been LOG_INF(). As confirmed by the author Adrian in zephyrproject-rtos#60172 Fixes commit 3fbaed4 ("dai: intel: ace: dmic: Refactor of dai_nhlt_dmic_dai_params_get function") Signed-off-by: Marc Herbert <[email protected]> (cherry picked from commit 8042e73)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Definitions of a configuration blob structures were moved from the main drivers header file to a dedicated file.
Moved code fragments responsible for logging and verification of the configuration register values from the dai_dmic_set_config_nhlt function to a separate functions. Behavior of the code verifying the correctness of register values has been changed so that it only displays warnings.
All PDM controllers have the same set of registers. Their definitions have been merged to simplify the code. The same was done for FIR filters registers.
Code responsible for configuring fir was separated into a new function. Created set of new functions for configure fir coefficients with support for packed format.
Added reading of a necessary register values in dai_nhlt_dmic_dai_params_get function to simplify its parameter list. The code that calculates dai_params has been moved to it to simplify the dai_dmic_set_config_nhlt function.