Skip to content

Fix runtime pm issue on volteer #2201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 4 commits into from
Jun 18, 2020
Merged

Fix runtime pm issue on volteer #2201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca0d2fb
ASoC: Intel: sof_max98373: share code between I2S & sdw mode of max98373
RanderWang Jun 15, 2020
c4a8dc4
ASoC: Intel: sdw_max98373: add trigger function to sdw platform
RanderWang Jun 15, 2020
1427750
ASoC: Intel: sdw_max98373: add card_late_probe support
RanderWang Jun 15, 2020
daedc46
ASoc: codec: max98373: remove Idle_bias_on to make codec suspended
RanderWang Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions sound/soc/codecs/max98373.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,6 @@ static const struct snd_soc_component_driver soc_codec_dev_max98373 = {
.num_dapm_widgets = ARRAY_SIZE(max98373_dapm_widgets),
.dapm_routes = max98373_audio_map,
.num_dapm_routes = ARRAY_SIZE(max98373_audio_map),
.idle_bias_on = 1,
.use_pmdown_time = 1,
.endianness = 1,
.non_legacy_dai_naming = 1,
Expand All @@ -883,7 +882,6 @@ const struct snd_soc_component_driver soc_codec_dev_max98373_sdw = {
.num_dapm_widgets = ARRAY_SIZE(max98373_dapm_widgets),
.dapm_routes = max98373_audio_map,
.num_dapm_routes = ARRAY_SIZE(max98373_audio_map),
.idle_bias_on = 1,
.use_pmdown_time = 1,
.endianness = 1,
.non_legacy_dai_naming = 1,
Expand Down
1 change: 1 addition & 0 deletions sound/soc/intel/boards/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ snd-soc-sof-sdw-objs += sof_sdw.o \
sof_sdw_rt711.o sof_sdw_rt700.o \
sof_sdw_rt1308.o sof_sdw_rt715.o \
sof_sdw_rt5682.o \
sof_maxim_common.o \
sof_sdw_dmic.o sof_sdw_hdmi.o hda_dsp_common.o
obj-$(CONFIG_SND_SOC_INTEL_SOF_RT5682_MACH) += snd-soc-sof_rt5682.o
obj-$(CONFIG_SND_SOC_INTEL_HASWELL_MACH) += snd-soc-sst-haswell.o
Expand Down
4 changes: 2 additions & 2 deletions sound/soc/intel/boards/sof_maxim_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

#define MAX_98373_PIN_NAME 16

static const struct snd_soc_dapm_route max_98373_dapm_routes[] = {
const struct snd_soc_dapm_route max_98373_dapm_routes[] = {
/* speaker */
{ "Left Spk", NULL, "Left BE_OUT" },
{ "Right Spk", NULL, "Right BE_OUT" },
Expand Down Expand Up @@ -59,7 +59,7 @@ static int max98373_hw_params(struct snd_pcm_substream *substream,
return 0;
}

static int max98373_trigger(struct snd_pcm_substream *substream, int cmd)
int max98373_trigger(struct snd_pcm_substream *substream, int cmd)
{
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct snd_soc_dai *codec_dai;
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/intel/boards/sof_maxim_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@

extern struct snd_soc_dai_link_component max_98373_components[2];
extern struct snd_soc_ops max_98373_ops;
extern const struct snd_soc_dapm_route max_98373_dapm_routes[];

int max98373_spk_codec_init(struct snd_soc_pcm_runtime *rtd);
void sof_max98373_codec_conf(struct snd_soc_card *card);
int max98373_trigger(struct snd_pcm_substream *substream, int cmd);

#endif /* __SOF_MAXIM_COMMON_H */
27 changes: 22 additions & 5 deletions sound/soc/intel/boards/sof_sdw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,11 @@ static struct snd_soc_codec_conf codec_conf[] = {
/* two MAX98373s on link1 with different unique id */
{
.dlc = COMP_CODEC_CONF("sdw:1:19f:8373:0:3"),
.name_prefix = "mx8373-1",
.name_prefix = "Right",
},
{
.dlc = COMP_CODEC_CONF("sdw:1:19f:8373:0:7"),
.name_prefix = "mx8373-2",
.name_prefix = "Left",
},
{
.dlc = COMP_CODEC_CONF("sdw:0:25d:5682:0"),
Expand Down Expand Up @@ -172,12 +172,12 @@ static struct snd_soc_dai_link_component platform_component[] = {
};

/* these wrappers are only needed to avoid typecast compilation errors */
static int sdw_startup(struct snd_pcm_substream *substream)
int sdw_startup(struct snd_pcm_substream *substream)
{
return sdw_startup_stream(substream);
}

static void sdw_shutdown(struct snd_pcm_substream *substream)
void sdw_shutdown(struct snd_pcm_substream *substream)
{
sdw_shutdown_stream(substream);
}
Expand Down Expand Up @@ -219,6 +219,7 @@ static struct sof_sdw_codec_info codec_info_list[] = {
.direction = {true, true},
.dai_name = "max98373-aif1",
.init = sof_sdw_mx8373_init,
.codec_card_late_probe = sof_sdw_mx8373_late_probe,
},
{
.id = 0x5682,
Expand Down Expand Up @@ -909,12 +910,28 @@ static int sof_card_dai_links_create(struct device *dev,
return 0;
}

int sof_sdw_card_late_probe(struct snd_soc_card *card)
{
int i, ret;

for (i = 0; i < ARRAY_SIZE(codec_info_list); i++) {
if (!codec_info_list[i].late_probe)
continue;

ret = codec_info_list[i].codec_card_late_probe(card);
if (ret < 0)
return ret;
}

return sof_sdw_hdmi_card_late_probe(card);
}

/* SoC card */
static const char sdw_card_long_name[] = "Intel Soundwire SOF";

static struct snd_soc_card card_sof_sdw = {
.name = "soundwire",
.late_probe = sof_sdw_hdmi_card_late_probe,
.late_probe = sof_sdw_card_late_probe,
.codec_conf = codec_conf,
.num_configs = ARRAY_SIZE(codec_conf),
};
Expand Down
9 changes: 9 additions & 0 deletions sound/soc/intel/boards/sof_sdw_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include <linux/bits.h>
#include <linux/types.h>
#include <sound/soc.h>

#define MAX_NO_PROPS 2
#define MAX_HDMI_NUM 4
Expand Down Expand Up @@ -61,6 +62,9 @@ struct sof_sdw_codec_info {
struct snd_soc_dai_link *dai_links,
struct sof_sdw_codec_info *info,
bool playback);

bool late_probe;
Copy link
Collaborator

@ranj063 ranj063 Jun 17, 2020

Choose a reason for hiding this comment

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

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

Copy link
Author

@RanderWang RanderWang Jun 18, 2020

Choose a reason for hiding this comment

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

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

int (*codec_card_late_probe)(struct snd_soc_card *card);
};

struct mc_private {
Expand All @@ -71,6 +75,9 @@ struct mc_private {

extern unsigned long sof_sdw_quirk;

int sdw_startup(struct snd_pcm_substream *substream);
void sdw_shutdown(struct snd_pcm_substream *substream);

/* generic HDMI support */
int sof_sdw_hdmi_init(struct snd_soc_pcm_runtime *rtd);

Expand Down Expand Up @@ -111,6 +118,8 @@ int sof_sdw_mx8373_init(const struct snd_soc_acpi_link_adr *link,
struct sof_sdw_codec_info *info,
bool playback);

int sof_sdw_mx8373_late_probe(struct snd_soc_card *card);

/* RT5682 support */
int sof_sdw_rt5682_init(const struct snd_soc_acpi_link_adr *link,
struct snd_soc_dai_link *dai_links,
Expand Down
29 changes: 22 additions & 7 deletions sound/soc/intel/boards/sof_sdw_max98373.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,13 @@
#include <sound/soc.h>
#include <sound/soc-acpi.h>
#include "sof_sdw_common.h"
#include "sof_maxim_common.h"

static const struct snd_soc_dapm_widget mx8373_widgets[] = {
SND_SOC_DAPM_SPK("Left Spk", NULL),
SND_SOC_DAPM_SPK("Right Spk", NULL),
};

static const struct snd_soc_dapm_route mx8373_map[] = {
/* Speakers */
{ "Left Spk", NULL, "mx8373-1 BE_OUT" },
{ "Right Spk", NULL, "mx8373-2 BE_OUT" },
};

static const struct snd_kcontrol_new mx8373_controls[] = {
SOC_DAPM_PIN_SWITCH("Left Spk"),
SOC_DAPM_PIN_SWITCH("Right Spk"),
Expand Down Expand Up @@ -51,13 +46,19 @@ static int spk_init(struct snd_soc_pcm_runtime *rtd)
return ret;
}

ret = snd_soc_dapm_add_routes(&card->dapm, mx8373_map, 2);
ret = snd_soc_dapm_add_routes(&card->dapm, max_98373_dapm_routes, 2);
if (ret)
dev_err(rtd->dev, "failed to add first SPK map: %d\n", ret);

return ret;
}

static const struct snd_soc_ops max_98373_sdw_ops = {
.startup = sdw_startup,
.trigger = max98373_trigger,
Copy link
Collaborator

@lyakh lyakh Jun 16, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Author

@RanderWang RanderWang Jun 17, 2020

Choose a reason for hiding this comment

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

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

.shutdown = sdw_shutdown,
};

int sof_sdw_mx8373_init(const struct snd_soc_acpi_link_adr *link,
struct snd_soc_dai_link *dai_links,
struct sof_sdw_codec_info *info,
Expand All @@ -67,5 +68,19 @@ int sof_sdw_mx8373_init(const struct snd_soc_acpi_link_adr *link,
if (info->amp_num == 2)
dai_links->init = spk_init;

info->late_probe = true;
Copy link
Collaborator

@lyakh lyakh Jun 16, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Author

@RanderWang RanderWang Jun 17, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@lyakh lyakh Jun 17, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@plbossart plbossart Jun 17, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@lyakh lyakh Jun 18, 2020

Choose a reason for hiding this comment

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

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

Copy link
Author

@RanderWang RanderWang Jun 18, 2020

Choose a reason for hiding this comment

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

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


dai_links->ops = &max_98373_sdw_ops;

return 0;
}

int sof_sdw_mx8373_late_probe(struct snd_soc_card *card)
{
struct snd_soc_dapm_context *dapm = &card->dapm;

/* Disable Left and Right Spk pin after boot */
snd_soc_dapm_disable_pin(dapm, "Left Spk");
snd_soc_dapm_disable_pin(dapm, "Right Spk");
return snd_soc_dapm_sync(dapm);
}