Skip to content

ASoC:SOF:skl:enable the core and get the ROM init. V2#132

Merged
lgirdwood merged 1 commit into
thesofproject:topic/sof-devfrom
zhuyingjiang:topic/sof-dev-yjzhu
Sep 17, 2018
Merged

ASoC:SOF:skl:enable the core and get the ROM init. V2#132
lgirdwood merged 1 commit into
thesofproject:topic/sof-devfrom
zhuyingjiang:topic/sof-dev-yjzhu

Conversation

@zhuyingjiang

Copy link
Copy Markdown
  1. enable the skl core
  2. get the ROM int

Signed-off-by: Zhu Yingjiang yingjiang.zhu@linux.intel.com

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

ok, so this code is confusing as if ROM status is 0 we return 0 ( and 0 globally means success within the kernel) ?? but if ROM status is good we return -EIO ??

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The debug info is error, removed

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

This is debug, it must be removed before merge.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

removed

@zhuyingjiang

Copy link
Copy Markdown
Author

@lgirdwood

@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.

Some more minor updates needed.

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

This should be if (ret < 0) since 0 is always success. This check should also be in the else {} block above.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh, this should be removed, since prepare is not here.

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

snd_sof_dsp_register_poll only return 0 on success.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, will change.

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

you should just return here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, will change

@zhuyingjiang zhuyingjiang left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed according to the comments.

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh, this should be removed, since prepare is not here.

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, will change

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, will change.

@zhuyingjiang

Copy link
Copy Markdown
Author

@lgirdwood changed according to the comments.

@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.

Just fix return code checking and it should be good to merge

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

ret should be checked here in this {} block and if < 0 should power off core and return ret

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, if (ret<0) goto err;

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

always use if (ret < 0) for error checking as some posix/kernel APIs may also return a positive integer on success

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, make all the fail return, if(ret<0) goto err;

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

ret < 0

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good check!

@zhuyingjiang zhuyingjiang left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, the return value changed.

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, if (ret<0) goto err;

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, make all the fail return, if(ret<0) goto err;

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good check!

@zhuyingjiang

Copy link
Copy Markdown
Author

@lgirdwood the return value changed.

@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.

Just one line to delete.

Comment thread sound/soc/sof/intel/hda-loader.c Outdated

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.

No need to change the value of ret here, otherwise the meaning will be lost further up the stack. So best remove this line.

1. enable the skl core
2. get the ROM int

Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>
@zhuyingjiang

Copy link
Copy Markdown
Author

@lgirdwood removed.

@lgirdwood
lgirdwood merged commit a84d54b into thesofproject:topic/sof-dev Sep 17, 2018
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.

2 participants