Fix runtime pm issue on volteer #2201

RanderWang · 2020-06-16T02:41:29Z

This PR ports patch #2123 to fix runtime pm issue.
And also remove idle_bias_on in codec driver to let codec be able to suspend

fixes #2053

RanderWang · 2020-06-16T02:43:11Z

@ClarexZhou can you help to validate it ? thanks

ClarexZhou · 2020-06-16T06:50:01Z

@ClarexZhou can you help to validate it ? thanks

Verified. Runtime PM is fixed on 373 in SDW mode. Also no side effect on 373 in I2S mode.

lyakh · 2020-06-16T06:54:26Z

sound/soc/intel/boards/sof_sdw_max98373.c


+static const struct snd_soc_ops max_98373_sdw_ops = {
+	.startup = sdw_startup,
+	.trigger = max98373_trigger,


where is this function defined? I don't see it in this file and this file doesn't seem to even include any max98373 headers?

it is define @ https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sof_maxim_common.c#L62

lyakh · 2020-06-16T07:00:34Z

sound/soc/intel/boards/sof_sdw_max98373.c

 	if (info->amp_num == 2)
 		dai_links->init = spk_init;

+	info->late_probe = true;


do I understand it correctly, that you're modifying here a global static common struct? Are we sure no other driver will want to use it?

we also increase info->amp_num in this function. It is only used by sof_sdw machine driver for it is a static struct define in sof_sdw.c

So what happens if you have several codecs of the same type?

@lyakh can you clarify your question? we do have configurations with 2 amplifiers of the same type, not sure what you see as wrong or incorrect?

@plbossart @RanderWang I think I'm getting it slowly. codec_info_list is a global array of objects - one per device type. Each of these objects is shared by all instances of that respective device - both for reading and writing. That's also how .amp_num is used. And there's no locking because (so far) those fields are changed from .init() and it's called for each device instance successively so no race is possible. This seems to be ok so far, but is a bit... strange. Can we also have codecs of different types mixed on a system? Would we then have to count them separately or together?

yes, no race for sdw-sof is executed serially. We have one case on TGL: 1308 i2s + 711 sdw + HDMI.

bardliao · 2020-06-16T08:29:07Z

@RanderWang I finally got what you are trying to do by reading a6c4e1a. However, I am not sure if it is a right solution. "Left Spk" and "Right Spk" are SND_SOC_DAPM_SPK which is supposed to be controlled by DAPM. Maybe what we really need is to enable/disable "VMON", "IMON", and "FBMON".

plbossart · 2020-06-16T14:39:36Z

SOFCI TEST

This patch unify the codec prefix name to share code between I2S & sdw mode of max98373. Signed-off-by: randerwang <rander.wang@linux.intel.com>

Port patch: ASoC: Intel: Boards: tgl_max98373: add dai_trigger function to sdw platform to fix runtime pm issue. Signed-off-by: randerwang <rander.wang@linux.intel.com>

Disable Left and Right Spk pin after boot so that sof can get suspended. Please check patch: ASoC: Intel: Boards: tgl_max98373: add dai_trigger function Signed-off-by: randerwang <rander.wang@linux.intel.com>

Idle_bias_on is used to decide bias on/off in standby state by dapm. When Idle_bias_on is set to one, dapm will keep max98373 active at idle time. Max98373 is doing nothing in this state, so remove idle_bias_on setting to let max98373 get suspended when it is idle. Signed-off-by: randerwang <rander.wang@linux.intel.com> Reviewed-by : Ryan Lee <ryans.lee@maximintegrated.com>

RanderWang · 2020-06-17T05:32:53Z

@RanderWang I finally got what you are trying to do by reading a6c4e1a. However, I am not sure if it is a right solution. "Left Spk" and "Right Spk" are SND_SOC_DAPM_SPK which is supposed to be controlled by DAPM. Maybe what we really need is to enable/disable "VMON", "IMON", and "FBMON".

these widgets are only controlled by codec driver, not by machine driver

RanderWang · 2020-06-17T06:19:45Z

ICL-HDA was failed but it is not caused by this PR which only affects sdw platform

bardliao · 2020-06-17T07:01:20Z

@RanderWang I finally got what you are trying to do by reading a6c4e1a. However, I am not sure if it is a right solution. "Left Spk" and "Right Spk" are SND_SOC_DAPM_SPK which is supposed to be controlled by DAPM. Maybe what we really need is to enable/disable "VMON", "IMON", and "FBMON".

these widgets are only controlled by codec driver, not by machine driver

That's a good point. Thanks.

ranj063 · 2020-06-17T23:11:07Z

sound/soc/intel/boards/sof_sdw_common.h

 		     struct sof_sdw_codec_info *info,
 		     bool playback);
+
+	bool late_probe;


@RanderWang why do we need this book? Can we not simply check if the function pointer below is NULL?

@ranj063 consider a case: a codec has late_probe function but it is not integrated in some platforms, so we don't need to invoke this function.

lyakh

The only way to revoke my "request for change" seems to me by approving the PR... I'll wait for more approvals from people more knowledgeable about SDW than myself, then I'll change my review to an "approval" too.

bardliao

LGTM and it moves to a good direction that it is better to use the same code cross interfaces.

RanderWang requested review from dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners June 16, 2020 02:41

RanderWang requested a review from bardliao June 16, 2020 02:41

YvonneYang2 mentioned this pull request Jun 16, 2020

[TGL][SDW] runtime PM always in active on TGL chromebook/SDW mode #2162

Closed

lyakh requested changes Jun 16, 2020

View reviewed changes

plbossart previously approved these changes Jun 16, 2020

View reviewed changes

RanderWang dismissed plbossart’s stale review via daedc46 June 17, 2020 05:27

RanderWang force-pushed the tgl-volteer-pm branch from 34194af to daedc46 Compare June 17, 2020 05:27

RanderWang added 4 commits June 17, 2020 13:31

ASoC: Intel: sof_max98373: share code between I2S & sdw mode of max98373

ca0d2fb

This patch unify the codec prefix name to share code between I2S & sdw mode of max98373. Signed-off-by: randerwang <rander.wang@linux.intel.com>

ASoC: Intel: sdw_max98373: add trigger function to sdw platform

c4a8dc4

Port patch: ASoC: Intel: Boards: tgl_max98373: add dai_trigger function to sdw platform to fix runtime pm issue. Signed-off-by: randerwang <rander.wang@linux.intel.com>

ASoC: Intel: sdw_max98373: add card_late_probe support

1427750

Disable Left and Right Spk pin after boot so that sof can get suspended. Please check patch: ASoC: Intel: Boards: tgl_max98373: add dai_trigger function Signed-off-by: randerwang <rander.wang@linux.intel.com>

bardliao approved these changes Jun 17, 2020

View reviewed changes

ranj063 reviewed Jun 17, 2020

View reviewed changes

lyakh self-requested a review June 18, 2020 07:46

lyakh reviewed Jun 18, 2020

View reviewed changes

bardliao approved these changes Jun 18, 2020

View reviewed changes

plbossart approved these changes Jun 18, 2020

View reviewed changes

plbossart merged commit 1b2f260 into thesofproject:topic/sof-dev Jun 18, 2020

RanderWang mentioned this pull request Sep 11, 2020

ASOC: Intel: sof_sdw: restore playback functionality with max98373 amps #2424

Merged

Fix runtime pm issue on volteer #2201

Fix runtime pm issue on volteer #2201

Uh oh!

Conversation

RanderWang commented Jun 16, 2020

Uh oh!

RanderWang commented Jun 16, 2020

Uh oh!

ClarexZhou commented Jun 16, 2020

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!

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!

Choose a reason for hiding this comment

Uh oh!

bardliao commented Jun 16, 2020

Uh oh!

plbossart commented Jun 16, 2020

Uh oh!

RanderWang commented Jun 17, 2020

Uh oh!

RanderWang commented Jun 17, 2020

Uh oh!

bardliao commented Jun 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment • 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!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lyakh left a comment •

edited

Loading