Skip to content

ipc: Refactor ipc_comp_dai_config #1997

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

Merged
dbaluta merged 1 commit into from
Oct 24, 2019
Merged

ipc: Refactor ipc_comp_dai_config #1997

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 18 additions & 34 deletions src/ipc/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,49 +436,33 @@ int ipc_comp_dai_config(struct ipc *ipc, struct sof_ipc_dai_config *config)
struct sof_ipc_comp_dai *dai;
Comment thread
dbaluta marked this conversation as resolved.
Resolved
struct ipc_comp_dev *icd;
struct list_item *clist;
struct comp_dev *dev;
int ret = -ENODEV;

/* for each component */
list_for_item(clist, &ipc->shared_ctx->comp_list) {
icd = container_of(clist, struct ipc_comp_dev, list);
switch (icd->type) {
case COMP_TYPE_COMPONENT:

/* make sure we only config DAI comps */
switch (icd->cd->comp.type) {
case SOF_COMP_DAI:
case SOF_COMP_SG_DAI:
dev = icd->cd;
dai = (struct sof_ipc_comp_dai *)&dev->comp;

/*
* set config if comp dai_index matches
* config dai_index.
*/
if (dai->dai_index == config->dai_index &&
dai->type == config->type) {
ret = comp_dai_config(dev, config);
if (ret < 0) {
trace_ipc_error
("ipc_comp_dai_config() "
"error: comp_dai_config() "
"failed");
return ret;
}
}
break;
Comment thread
dbaluta marked this conversation as resolved.
Resolved
default:
break;
}
/* make sure we only config DAI comps */
if (icd->type != COMP_TYPE_COMPONENT)
continue;

break;
/* ignore non components */
default:
break;
if (icd->cd->comp.type == SOF_COMP_DAI ||
icd->cd->comp.type == SOF_COMP_SG_DAI) {
dai = (struct sof_ipc_comp_dai *)&icd->cd->comp;
/*
* set config if comp dai_index matches
* config dai_index.
*/
if (dai->dai_index == config->dai_index &&
dai->type == config->type)
ret = comp_dai_config(icd->cd, config);
}
}

if (ret < 0) {
trace_ipc_error("ipc_comp_dai_config() error: "
"comp_dai_config() failed");

@dbaluta dbaluta Oct 24, 2019

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.

No need for curly braces around trace_ipc_error here.

@ktrzcinx ktrzcinx Oct 24, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I need because it is a macro

@paulstelian97 paulstelian97 Oct 24, 2019

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.

The trace_* macros have been written to act like functions from the point of view of using them so no, the braces aren't needed.

@dbaluta dbaluta Oct 24, 2019

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.

@ktrzcinx do you mean that if we have something like this

if (ret < 0) 
    trace_();
foo();

and the trace_() expands to nothing then we are in big trouble?

@dbaluta dbaluta Oct 24, 2019

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'm OK with the code as it is now, but we should keep this discussion open.

@ktrzcinx ktrzcinx Oct 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I claim that when trace_* expands to nothing then it will be ok, but when trace_* expand to any code with more than one line of code without { } inside then it won't. It will be need to spend much more time to debug what happen in such a situation in comparison with usage redundant parenthesis. I know that such a macro function is not a good solution but it is open source project and when using extra {} I know that I am safe for such a situation. Such a situation is not a big abstraction, also in tracing functions, for example:

#define trace(obj_ptr,msg,...) \ 
   typeof(obj_ptr) __obj_ptr__ = obj_ptr; \
   _log_fun(__obj_ptr__ ->id, __obj_ptr__ ->parent_id, __VA_ARGS__)

or

#define trace(obj_ptr,msg,...) \ 
   flush_logs(); \
   _log_fun(obj_ptr->id, obj_ptr->parent_id, __VA_ARGS__)

Now

if (ret < 0)
   trace(obj,"blah");
foo();

will trace independently ret value but my solution will still work correctly. There also won't be any compilation error and a lot of tests should pass. In conclusion, I rather prefer defensive programming with extra {} from debugging.

}

return ret;
}

Expand Down