Skip to content

Add rate and channels to HDA config#2007

Merged
plbossart merged 2 commits intothesofproject:topic/sof-devfrom
bardliao:hda-params
Apr 15, 2020
Merged

Add rate and channels to HDA config#2007
plbossart merged 2 commits intothesofproject:topic/sof-devfrom
bardliao:hda-params

Conversation

@bardliao
Copy link
Collaborator

@bardliao bardliao commented Apr 13, 2020

FW needs to know rate and channels for HDA dai.

This PR should not be merged before #1943

@bardliao bardliao added the ABI involves ABI changes, need extra attention label Apr 13, 2020
plbossart
plbossart requested changes Apr 13, 2020
View reviewed changes
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

minor nit-pick below.

plbossart
plbossart previously approved these changes Apr 13, 2020
View reviewed changes
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

thanks @bardliao please add a reference to the firmware PR so that we merge at the same time - or at least with the same ABI change

@bardliao bardliao mentioned this pull request Apr 13, 2020
Merged
@mengdonglin mengdonglin mentioned this pull request Apr 14, 2020
Closed
slawblauciak
slawblauciak previously approved these changes Apr 14, 2020
View reviewed changes
@slawblauciak
Copy link

slawblauciak commented Apr 14, 2020

I wonder if we should add the FIFO size, since that is configurable by the kernel. Right now it's probably not needed for anything, but we're already extending the ABI and the FW does propagate the FIFO size information.

@slawblauciak slawblauciak mentioned this pull request Apr 14, 2020
Merged
kv2019i
kv2019i requested changes Apr 14, 2020
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

@bardliao Code looks good. I think we need to improve the commit messages though. I was a bit late with the SDW patches, but let's do better in these HDA patches. Please see my comments and proposals inline.

sound/soc/sof/topology.c
@@ -3064,7 +3064,7 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index,
config->hdr.size = size;
Copy link
Collaborator

@kv2019i kv2019i Apr 14, 2020

Choose a reason for hiding this comment

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

@bardliao Can you add a mention in commit message that as "hda_tokens" array was empty, this error has been harmless.

include/uapi/sound/sof/tokens.h
#define SOF_TKN_MUTE_LED_USE 1300
#define SOF_TKN_MUTE_LED_DIRECTION 1301

/* HDA */
Copy link
Collaborator

@kv2019i kv2019i Apr 14, 2020

Choose a reason for hiding this comment

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

Typo in summary line: rate amd channels -> rate and channels

Copy link
Collaborator

@kv2019i kv2019i Apr 14, 2020

Choose a reason for hiding this comment

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

I wonder whether it makes sense to keep 2nd and 3rd patches separate. The first one doesn't really make much sense on its own. And I don't think it's worth having the same explanation again in this commit, on why this is done.

If you keep these separate, maybe something more useful to commit description than
"FW needs to know rate and channels."
Maybe just add "Added support for rate and channel tokens for HDA DAI. Added in topology interface 3.16."

@bardliao
ASoC: SOF: topology: fix: parse hda_tokens to &config->hda
dad9530 
Items in hda_tokens are for &config->hda. So fix it to the right
object. This error has been harmless as hda_tokens array was empty.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
@bardliao bardliao dismissed stale reviews from slawblauciak and plbossart via c1ab7ef April 15, 2020 05:33
@bardliao
ASoC: SOF: topology: Get HDA rate and channels from topology
279e2e8 
FW interface for HDA DAI parameters was extended with information on
sampling rate and channel count in version 3.16. Align kernel header
with the FW change. This change is backwards compatible. Old firmware
will ignore the values.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
@bardliao
Copy link
Collaborator Author

bardliao commented Apr 15, 2020

Thanks @kv2019i , PR updated

kv2019i
kv2019i approved these changes Apr 15, 2020
View reviewed changes
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

@bardliao Great, thanks, look good now!

plbossart
plbossart approved these changes Apr 15, 2020
View reviewed changes
Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

thanks @bardliao

@plbossart plbossart merged commit f9b8747 into thesofproject:topic/sof-dev Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@plbossart plbossart plbossart approved these changes

@lyakh lyakh lyakh approved these changes

@kv2019i kv2019i kv2019i approved these changes

@juimonen juimonen Awaiting requested review from juimonen

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@keyonjie keyonjie Awaiting requested review from keyonjie

@ranj063 ranj063 Awaiting requested review from ranj063 ranj063 is a code owner

@singalsu singalsu Awaiting requested review from singalsu

+1 more reviewer

@slawblauciak slawblauciak slawblauciak left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ABI involves ABI changes, need extra attention

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bardliao @slawblauciak @plbossart @lyakh @kv2019i