Skip to content

Simplifications for codec_adapter_set_params() - #5160

Merged
dbaluta merged 5 commits into
thesofproject:mainfrom
ranj063:fix/codec_adapter
Jan 11, 2022
Merged

dbaluta merged 5 commits into
thesofproject:mainfrom
ranj063:fix/codec_adapter

Conversation

@ranj063

@ranj063 ranj063 commented Jan 6, 2022

Copy link
Copy Markdown
Collaborator

No description provided.

@marc-hb

This comment has been minimized.

@ranj063

ranj063 commented Jan 6, 2022

Copy link
Copy Markdown
Collaborator Author

The build changes have already been tested and reviewed in #5153. I think #5153 should be merged first: smaller PRs are better : more "Continuous" Integration. We also just upgraded the Docker image - the fewer pieces moving at once the better.

this is a completely unrelated PR though?

@marc-hb

This comment has been minimized.

@ranj063

ranj063 commented Jan 6, 2022

Copy link
Copy Markdown
Collaborator Author

this is a completely unrelated PR though?

Now it is unrelated. It was related in your initial push to this PR that included commit "topology2: Add build support" and other commits from #5153: https://github.com/thesofproject/sof/commits/e7c05730897d9b36

Your next force-push apparently removed the #5153 commits.

right that was a mistake

@marc-hb
marc-hb removed their request for review January 6, 2022 18:55
@ranj063
ranj063 force-pushed the fix/codec_adapter branch from 1e87728 to c70f860 Compare January 6, 2022 19:55
lgirdwood
lgirdwood previously approved these changes Jan 6, 2022

@lgirdwood lgirdwood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dbaluta any comments ?

@ranj063
ranj063 force-pushed the fix/codec_adapter branch from c70f860 to 1dd7fb9 Compare January 6, 2022 22:45
cujomalainey
cujomalainey previously approved these changes Jan 6, 2022

@cujomalainey cujomalainey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

much appreciated thank you!

@ranj063

ranj063 commented Jan 8, 2022

Copy link
Copy Markdown
Collaborator Author

@lgirdwood I think #5166 should go first and then I can rebase this one

dbaluta
dbaluta previously approved these changes Jan 8, 2022
@dbaluta

dbaluta commented Jan 8, 2022

Copy link
Copy Markdown
Collaborator

Looks good to me. Somehow, I missed to add my review.

lyakh
lyakh previously approved these changes Jan 10, 2022
Comment thread src/audio/codec_adapter/codec_adapter.c Outdated
Comment thread src/audio/codec_adapter/codec_adapter.c Outdated
Comment thread src/audio/codec_adapter/codec_adapter.c Outdated
Comment thread src/audio/codec_adapter/codec_adapter.c Outdated
Comment thread src/audio/codec_adapter/codec_adapter.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I didn't fully verify this, but I guess it's a correct way to merge the above two cases and an additional call to codec_load_config() isn't needed because it's already called inside load_setup_config()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@lyakh looking more into this I realized that I could drop load_setup_config() altogether and modify module_load_config() to handle both runtime and setup configs. Updated accordingly.

It can be accessed via cdata in the function.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Remove the goto's and return the appropriate values to simplify the
codec_adapter_set_params() function.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Remove the unnecessary braces in codec_adapter_set_params().

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Check if more fragments of config data are expected first and return
immediately to make the code easier to follow.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@ranj063
ranj063 requested review from lgirdwood and lyakh January 10, 2022 17:11
@ranj063
ranj063 dismissed stale reviews from lyakh, dbaluta, cujomalainey, and lgirdwood January 10, 2022 17:12

Additional patch added

@lgirdwood

Copy link
Copy Markdown
Member

@dbaluta good for you ?

Comment thread src/audio/codec_adapter/codec/generic.c Outdated
Comment thread src/audio/codec_adapter/codec/generic.c Outdated
Comment thread src/audio/codec_adapter/codec_adapter.c Outdated
Modify module_load_config() to handle both setup and runtime configs and
remove load_setup_config().

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
@dbaluta
dbaluta merged commit 56784a9 into thesofproject:main Jan 11, 2022
@ranj063
ranj063 deleted the fix/codec_adapter branch January 11, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants