Skip to content

Fix/upstream reviews3 - #411

Merged
plbossart merged 17 commits into
thesofproject:topic/sof-devfrom
plbossart:fix/upstream-reviews3
Dec 12, 2018
Merged

Fix/upstream reviews3#411
plbossart merged 17 commits into
thesofproject:topic/sof-devfrom
plbossart:fix/upstream-reviews3

Conversation

@plbossart

Copy link
Copy Markdown
Member

This PR addresses most of the feedback from @andy-shev (Andy Shevchenko) on the core parts only.
Reviewers, I would appreciate if you can prioritize and spend a bit of time double-checking the fixes. Also there are 4 issues I added when I don't have the answers on my own.
I realize this is painful work, but I have no intention of doing it alone while you have fun with real work... I will merge these fixes on Thursday latest to get a chance for one round of validation before the next batch.
Thank you.
-Pierre

sound/soc/sof/intel/hda-dai.c: In function ‘hda_link_hw_free’:
sound/soc/sof/intel/hda-dai.c:213:22: warning: variable ‘sdev’ set but not used [-Wunused-but-set-variable]
struct snd_sof_dev *sdev;
^~~~
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
sound/soc/sof/debug.c: In function ‘sof_dfsentry_read’:
sound/soc/sof/debug.c:62:10: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
if (ret < 0)
Defining a 'ret' variable as size_t was borderline criminal in the
first place...
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
sound/soc/sof/topology.c: In function ‘sof_control_load_volume’:
sound/soc/sof/topology.c:261:28: warning: variable ‘cdata’ set but not used [-Wunused-but-set-variable]
struct sof_ipc_ctrl_data *cdata;
^~~~~
sound/soc/sof/topology.c: In function ‘snd_sof_load_topology’:
sound/soc/sof/topology.c:2711:27: warning: variable ‘hdr’ set but not used [-Wunused-but-set-variable]
struct snd_soc_tplg_hdr *hdr;
^~~
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Constants with _MS suffix
simplification of search loop
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Not used, remove for now
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Feedback from Andy Shevchenko
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Unclear if platform_device_register_data can return NULL, err on the
side of caution
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Suggested by Andy Shevchenko
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@ranj063

Copy link
Copy Markdown
Collaborator

@plbossart you're not alone :) I will prioritize this for tonight

Comment threadsound/soc/sof/hw-spi.c Outdated
Comment threadsound/soc/sof/sof-spi-dev.c Outdated

@singalsusingalsuDec 12, 2018

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.

Why "#if 0", can this part be just deleted?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we can delete this, and add it back when PM on it is supported.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

done

@keyonjiekeyonjie left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

minor comments.

Comment threadsound/soc/sof/sof-spi-dev.c Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we can delete this, and add it back when PM on it is supported.

Comment threadsound/soc/sof/control.c Outdated
Comment threadsound/soc/sof/debug.c Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

count = min(count, (size_t) (size - pos)); ?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

done, thanks

Comment threadsound/soc/sof/trace.c Outdated
Maybe there are other cases where this is needed
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
SI units people.
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Feedback from Andy Shevchenko
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Feedback from Andy Shevchenko.
Keep indent on for subparts of the log
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
remove change variable, use return false/true instead
Feedback from Andy Shevchenko
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
remove new line, even if it makes checkpatch unhappy
Feedback from Andy Shevchenko
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Feedback from Andy Shevchenko
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
remove spi_pm, will be added back when needed
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Apparently cannot be unsigned int
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart
plbossart merged commit 099e1fd into thesofproject:topic/sof-devDec 12, 2018
Sign up for freeto 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

@plbossart@ranj063@bardliao@keyonjie@singalsu@RanderWang