ASoC: intel: sof_sdw_rt1308: amp_num should be += num_codecs by bardliao · Pull Request #1954 · thesofproject/linux

bardliao · 2020-03-30T05:14:13Z

The amp_num represents the number of amps. A dai link may have
more than one amp. So amp_num should += the number of amps in
the dai link. And test dai_links->init does not work since dai_links->init
is already set when we test it.

Fixes: #1953

The amp_num represents the number of amps. A dai link may have more than one amp. So amp_num should += the number of amps in the dai link. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

spk_init should be use It is more readable to test dai_links->num_codecs than dai_links->init. Also, sof_sdw_rt1308_init() will be called once if two rt1308s are aggregated. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

…me dailink The only reason that we do that is to calculate how many rt1308s do we have. However, we can get the number by dailinks->num_codecs. Now, we increase rt1308 amp_num by += num_codecs. So we can do codec init in each dailink now. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

lyakh · 2020-03-30T08:59:17Z

+		return codec_index;

-		if (codec_info_list[codec_index].init)
-			codec_info_list[codec_index].init(link, dai_links,


I don't understand the process well enough, but this raises an alarm for me. Are we really sure that it's always enough to just call .init() once for the first codec iinfo? That we'll never have mixed codecs or a codec, that needs certain initialisation performed for each instance? Individual codec .init() implementations might need to check whether an instance has already initialised on this card, but can we really skip this completely globally?

@bardliao I guess this is ok as this init() is specific to the sof-sdw driver and you only do dailink specific actions, but it's worth a check still. I.e. is this future proof to not all init for all codecs on the dailink?

I don't understand the process well enough, but this raises an alarm for me. Are we really sure that it's always enough to just call .init() once for the first codec iinfo? That we'll never have mixed codecs or a codec, that needs certain initialisation performed for each instance? Individual codec .init() implementations might need to check whether an instance has already initialised on this card, but can we really skip this completely globally?

@lyakh If we look at the "codec init" functions, we can see that they are used to set dailink init function. So it is fine to called them once per dailink. We don't support mixed codecs in a dailink for now. If we want to do that, we will do more modification than this PR. Current code doesn't allow us to have different init function for the same codec either. So, I think the PR is safe for current code.

@bardliao I guess this is ok as this init() is specific to the sof-sdw driver and you only do dailink specific actions, but it's worth a check still. I.e. is this future proof to not all init for all codecs on the dailink?

@kv2019i Yes, I checked all the init functions for supported codecs. None of them need to be called more than once per dailink. In fact, sof_sdw_rt1308_init() was the only one which will be called more than one time per dailink before this change.

kv2019i

Looks pretty good. A couple of questions inline.

kv2019i · 2020-03-30T09:32:18Z

-	info->amp_num++;
+	info->amp_num += dai_links->num_codecs;
 	if (info->amp_num == 1)
 		dai_links->init = first_spk_init;


@bardliao How about the logic below checking "info->amp_num == 1". Is this still valid in case amp_num is incremented possibly by many amps in one call?

Agree with @kv2019i . This code assumes amp_num ==1 or ==2, I don't understand this at all. Also num_codecs can include other devices, not just the amps, so the logic is not future proof?

kv2019i · 2020-03-30T09:44:58Z

+		return codec_index;

-		if (codec_info_list[codec_index].init)
-			codec_info_list[codec_index].init(link, dai_links,


@bardliao I guess this is ok as this init() is specific to the sof-sdw driver and you only do dailink specific actions, but it's worth a check still. I.e. is this future proof to not all init for all codecs on the dailink?

plbossart

Sorry @bardliao I am not convinced. I don't think it's right to change the logic and deal with the case where multiple devices per link means multiple amplifiers per link. That's a restriction we never had before.

plbossart · 2020-03-30T14:43:40Z

-	info->amp_num++;
+	info->amp_num += dai_links->num_codecs;
 	if (info->amp_num == 1)
 		dai_links->init = first_spk_init;


Agree with @kv2019i . This code assumes amp_num ==1 or ==2, I don't understand this at all. Also num_codecs can include other devices, not just the amps, so the logic is not future proof?

plbossart · 2020-03-30T14:46:38Z

-		unsigned int part_id;
-		int codec_index;
+	unsigned int part_id;
+	int codec_index;


no, I don't think this change is correct.
We don't want to make the assumption that if there are more than one devices on the link then they are all RT1308. That's not good, it means we cannot support hybrid configurations.

ranj063 · 2020-03-30T22:54:02Z

+		 * dai links.
 		 */
-		if (dai_links->init)
+		if (dai_links->num_codecs == 2)


@bardliao I think the check would be a lot more intuitive if you say if (dai_lisnk->num_codecs == info->amp_num)

but that's a wrong assumption! We should NOT assume num_codecs == num_amps.

hmm yes you are right @plbossart . It makes sense only if there is only one set of amps.

RanderWang · 2020-03-31T07:04:32Z

@bardliao the root reason is: set_codec_init_func only search the codecs in current link since there was no group support at design time. Now we need to check all the codecs in all links for a special group.

bardliao · 2020-03-31T07:54:35Z

Thanks all. I will create a new PR for #1953

Init codecs which belong to the same group id on all links. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> (backported from thesofproject#1954) Signed-off-by: Hui Wang <hui.wang@canonical.com>

BugLink: https://bugs.launchpad.net/bugs/1872916 Init codecs which belong to the same group id on all links. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> (backported from thesofproject#1954) Signed-off-by: Hui Wang <hui.wang@canonical.com>

bardliao requested review from RanderWang and plbossart March 30, 2020 05:14

bardliao requested review from dbaluta, kv2019i, lgirdwood and ranj063 as code owners March 30, 2020 05:14

ASoC: intel: sof_sdw_rt1308: amp_num should be += num_codecs

ab148aa

The amp_num represents the number of amps. A dai link may have more than one amp. So amp_num should += the number of amps in the dai link. Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

bardliao force-pushed the rt1308_init branch from 1f5283b to de220c5 Compare March 30, 2020 05:47

bardliao changed the title ~~ASoC: intel: sof_sdw_rt1308: amp_num should be += num_cpus~~ ASoC: intel: sof_sdw_rt1308: amp_num should be += num_codecs Mar 30, 2020

bardliao force-pushed the rt1308_init branch from de220c5 to ce6352b Compare March 30, 2020 06:53

paulstelian97 approved these changes Mar 30, 2020

View reviewed changes

lyakh reviewed Mar 30, 2020

View reviewed changes

kv2019i requested changes Mar 30, 2020

View reviewed changes

plbossart requested changes Mar 30, 2020

View reviewed changes

ranj063 reviewed Mar 30, 2020

View reviewed changes

bardliao closed this Mar 31, 2020

sinahuang mentioned this pull request Apr 1, 2020

[BUG][CML]Aplay0,2 rt1308 right speaker have sound output when muted speaker in alsamixer. #1953

Closed

YvonneYang2 mentioned this pull request Apr 1, 2020

[BUG][CML]DSP Panic occurred when pause/resume on aplay0,2(RT1308) thesofproject/sof#2658

Closed

Conversation

bardliao commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Mar 31, 2020

Uh oh!

bardliao commented Mar 31, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bardliao commented Mar 30, 2020 •

edited

Loading

plbossart Mar 30, 2020 •

edited

Loading