* [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
@ 2019-03-01 17:14 mac.chiang
2019-03-01 18:06 ` Pierre-Louis Bossart
2019-03-05 2:22 ` [PATCH v2] " mac.chiang
0 siblings, 2 replies; 13+ messages in thread
From: mac.chiang @ 2019-03-01 17:14 UTC (permalink / raw)
To: alsa-devel; +Cc: harshapriya.n, pierre-louis.bossart, jenny.tc
From: Mac Chiang <mac.chiang@intel.com>
s0ix entry failure is root cause by default speaker/vi sense path activate all the time
that result in the audio IPs are not power gated(checked by pch_ip_power_gating_status).
Adding the run time speaker control ensure playback widgets are going to free and off
eventually. With this func, counter works in s0ix subsequently (checked by slp_s0_residency_usc).
Signed-off-by: Jenny TC <jenny.tc@intel.com>
Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
Signed-off-by: Mac Chiang <mac.chiang@intel.com>
---
sound/soc/intel/boards/kbl_da7219_max98927.c | 50 ++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
index 723a493..e01ca9a 100644
--- a/sound/soc/intel/boards/kbl_da7219_max98927.c
+++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
@@ -195,8 +195,58 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
return 0;
}
+static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ int j, ret;
+
+ for (j = 0; j < rtd->num_codecs; j++) {
+ struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
+ const char *name = codec_dai->component->name;
+ struct snd_soc_component *component = codec_dai->component;
+ struct snd_soc_dapm_context *dapm =
+ snd_soc_component_get_dapm(component);
+ char pin_name[20];
+
+ if (strcmp(name, MAXIM_DEV0_NAME) &&
+ strcmp(name, MAXIM_DEV1_NAME))
+ continue;
+
+ snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
+ codec_dai->component->name_prefix);
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ ret = snd_soc_dapm_enable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to enable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ ret = snd_soc_dapm_disable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to disable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ }
+ }
+
+ return 0;
+}
+
static struct snd_soc_ops kabylake_ssp0_ops = {
.hw_params = kabylake_ssp0_hw_params,
+ .trigger = kabylake_ssp0_trigger,
};
static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-03-01 17:14 [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function mac.chiang
@ 2019-03-01 18:06 ` Pierre-Louis Bossart
2019-03-04 2:33 ` Chiang, Mac
2019-03-05 2:22 ` [PATCH v2] " mac.chiang
1 sibling, 1 reply; 13+ messages in thread
From: Pierre-Louis Bossart @ 2019-03-01 18:06 UTC (permalink / raw)
To: mac.chiang, alsa-devel; +Cc: harshapriya.n, Takashi Iwai, Mark Brown, jenny.tc
On 3/1/19 11:14 AM, mac.chiang@intel.com wrote:
> From: Mac Chiang <mac.chiang@intel.com>
>
> s0ix entry failure is root cause by default speaker/vi sense path activate all the time
> that result in the audio IPs are not power gated(checked by pch_ip_power_gating_status).
> Adding the run time speaker control ensure playback widgets are going to free and off
> eventually. With this func, counter works in s0ix subsequently (checked by slp_s0_residency_usc).
<rant>
If you submit a patch before internal reviews are complete, don't be
surprised at the feedback. And if you don't copy maintainers this patch
is going to be ignored as well.
</rant>
While the solution does solve a real issue, this commit message is
probably one of the most convoluted and Intel-centric I've seen in a
long time.
The issue really is that the amplifier feedback is not modeled as being
dependent on any active output. Even when there is no playback
happening, parts of the graph, specifically the IV sense->speaker
protection->output remains active and this prevents the DSP from
entering low-power states.
One alternative would be to have userspace play with pins/kcontrols to
take down the feedback loop. This is however not practical nor desirable.
A second approach would be to have a different model in the codec
driver, however Maxim indicated that the feedback includes stuff that is
not strictly related to playback.
This patch suggest a machine driver level approach where the speaker
pins are enabled/disabled dynamically depending on stream start/stop
events. DPAM graph representations show the feedback loop is indeed
disabled and low-power states can be reached.
Can you please update the commit message and resubmit.
Thanks.
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
> Signed-off-by: Mac Chiang <mac.chiang@intel.com>
> ---
> sound/soc/intel/boards/kbl_da7219_max98927.c | 50 ++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
> index 723a493..e01ca9a 100644
> --- a/sound/soc/intel/boards/kbl_da7219_max98927.c
> +++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
> @@ -195,8 +195,58 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
> return 0;
> }
>
> +static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + int j, ret;
> +
> + for (j = 0; j < rtd->num_codecs; j++) {
> + struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
> + const char *name = codec_dai->component->name;
> + struct snd_soc_component *component = codec_dai->component;
> + struct snd_soc_dapm_context *dapm =
> + snd_soc_component_get_dapm(component);
> + char pin_name[20];
> +
> + if (strcmp(name, MAXIM_DEV0_NAME) &&
> + strcmp(name, MAXIM_DEV1_NAME))
> + continue;
> +
> + snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
> + codec_dai->component->name_prefix);
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + ret = snd_soc_dapm_enable_pin(dapm, pin_name);
> + if (ret) {
> + dev_err(rtd->dev, "failed to enable %s: %d\n",
> + pin_name, ret);
> + return ret;
> + }
> + snd_soc_dapm_sync(dapm);
> + break;
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + ret = snd_soc_dapm_disable_pin(dapm, pin_name);
> + if (ret) {
> + dev_err(rtd->dev, "failed to disable %s: %d\n",
> + pin_name, ret);
> + return ret;
> + }
> + snd_soc_dapm_sync(dapm);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> static struct snd_soc_ops kabylake_ssp0_ops = {
> .hw_params = kabylake_ssp0_hw_params,
> + .trigger = kabylake_ssp0_trigger,
> };
>
> static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-03-01 18:06 ` Pierre-Louis Bossart
@ 2019-03-04 2:33 ` Chiang, Mac
0 siblings, 0 replies; 13+ messages in thread
From: Chiang, Mac @ 2019-03-04 2:33 UTC (permalink / raw)
To: Pierre-Louis Bossart, alsa-devel
Cc: N, Harshapriya, Takashi Iwai, Mark Brown, Tc, Jenny
Sure. I'll refer your comments and submit again.
-----Original Message-----
From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
Sent: Saturday, March 2, 2019 2:06 AM
To: Chiang, Mac <mac.chiang@intel.com>; alsa-devel@alsa-project.org
Cc: Tc, Jenny <jenny.tc@intel.com>; N, Harshapriya <harshapriya.n@intel.com>; Mark Brown <broonie@kernel.org>; Takashi Iwai <tiwai@suse.de>
Subject: Re: [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
On 3/1/19 11:14 AM, mac.chiang@intel.com wrote:
> From: Mac Chiang <mac.chiang@intel.com>
>
> s0ix entry failure is root cause by default speaker/vi sense path
> activate all the time that result in the audio IPs are not power gated(checked by pch_ip_power_gating_status).
> Adding the run time speaker control ensure playback widgets are going
> to free and off eventually. With this func, counter works in s0ix subsequently (checked by slp_s0_residency_usc).
<rant>
If you submit a patch before internal reviews are complete, don't be surprised at the feedback. And if you don't copy maintainers this patch is going to be ignored as well.
</rant>
While the solution does solve a real issue, this commit message is probably one of the most convoluted and Intel-centric I've seen in a long time.
The issue really is that the amplifier feedback is not modeled as being dependent on any active output. Even when there is no playback happening, parts of the graph, specifically the IV sense->speaker
protection->output remains active and this prevents the DSP from
entering low-power states.
One alternative would be to have userspace play with pins/kcontrols to
take down the feedback loop. This is however not practical nor desirable.
A second approach would be to have a different model in the codec
driver, however Maxim indicated that the feedback includes stuff that is
not strictly related to playback.
This patch suggest a machine driver level approach where the speaker
pins are enabled/disabled dynamically depending on stream start/stop
events. DPAM graph representations show the feedback loop is indeed
disabled and low-power states can be reached.
Can you please update the commit message and resubmit.
Thanks.
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
> Signed-off-by: Mac Chiang <mac.chiang@intel.com>
> ---
> sound/soc/intel/boards/kbl_da7219_max98927.c | 50 ++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
> index 723a493..e01ca9a 100644
> --- a/sound/soc/intel/boards/kbl_da7219_max98927.c
> +++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
> @@ -195,8 +195,58 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
> return 0;
> }
>
> +static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + int j, ret;
> +
> + for (j = 0; j < rtd->num_codecs; j++) {
> + struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
> + const char *name = codec_dai->component->name;
> + struct snd_soc_component *component = codec_dai->component;
> + struct snd_soc_dapm_context *dapm =
> + snd_soc_component_get_dapm(component);
> + char pin_name[20];
> +
> + if (strcmp(name, MAXIM_DEV0_NAME) &&
> + strcmp(name, MAXIM_DEV1_NAME))
> + continue;
> +
> + snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
> + codec_dai->component->name_prefix);
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + ret = snd_soc_dapm_enable_pin(dapm, pin_name);
> + if (ret) {
> + dev_err(rtd->dev, "failed to enable %s: %d\n",
> + pin_name, ret);
> + return ret;
> + }
> + snd_soc_dapm_sync(dapm);
> + break;
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + ret = snd_soc_dapm_disable_pin(dapm, pin_name);
> + if (ret) {
> + dev_err(rtd->dev, "failed to disable %s: %d\n",
> + pin_name, ret);
> + return ret;
> + }
> + snd_soc_dapm_sync(dapm);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> static struct snd_soc_ops kabylake_ssp0_ops = {
> .hw_params = kabylake_ssp0_hw_params,
> + .trigger = kabylake_ssp0_trigger,
> };
>
> static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-03-01 17:14 [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function mac.chiang
2019-03-01 18:06 ` Pierre-Louis Bossart
@ 2019-03-05 2:22 ` mac.chiang
2019-03-05 4:03 ` Pierre-Louis Bossart
2019-03-08 5:16 ` [PATCH v3] " mac.chiang
1 sibling, 2 replies; 13+ messages in thread
From: mac.chiang @ 2019-03-05 2:22 UTC (permalink / raw)
To: alsa-devel; +Cc: harshapriya.n, tiwai, broonie, pierre-louis.bossart, jenny.tc
From: Mac Chiang <mac.chiang@intel.com>
amplifier feedback is not modeled as being dependent on any active output.
Even when there is no playback happening, parts of the graph, specifically the IV
sense->speaker protection->output remains active and this prevents the DSP from
entering low-power states.
This patch suggest a machine driver level approach where the speaker
pins are enabled/disabled dynamically depending on stream start/stop
events. DPAM graph representations show the feedback loop is indeed
disabled and low-power states can be reached.
Signed-off-by: Jenny TC <jenny.tc@intel.com>
Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
Signed-off-by: Mac Chiang <mac.chiang@intel.com>
---
sound/soc/intel/boards/kbl_da7219_max98927.c | 69 +++++++++++++++++++++++++++-
1 file changed, 68 insertions(+), 1 deletion(-)
diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
index 723a493..3e05190 100644
--- a/sound/soc/intel/boards/kbl_da7219_max98927.c
+++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
@@ -195,8 +195,58 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
return 0;
}
+static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ int j, ret;
+
+ for (j = 0; j < rtd->num_codecs; j++) {
+ struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
+ const char *name = codec_dai->component->name;
+ struct snd_soc_component *component = codec_dai->component;
+ struct snd_soc_dapm_context *dapm =
+ snd_soc_component_get_dapm(component);
+ char pin_name[20];
+
+ if (strcmp(name, MAXIM_DEV0_NAME) &&
+ strcmp(name, MAXIM_DEV1_NAME))
+ continue;
+
+ snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
+ codec_dai->component->name_prefix);
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ ret = snd_soc_dapm_enable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to enable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ ret = snd_soc_dapm_disable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to disable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ }
+ }
+
+ return 0;
+}
+
static struct snd_soc_ops kabylake_ssp0_ops = {
.hw_params = kabylake_ssp0_hw_params,
+ .trigger = kabylake_ssp0_trigger,
};
static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
@@ -864,6 +914,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
{
struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
struct kbl_hdmi_pcm *pcm;
+ struct snd_soc_dapm_context *dapm = &card->dapm;
struct snd_soc_component *component = NULL;
int err, i = 0;
char jack_name[NAME_SIZE];
@@ -890,9 +941,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
if (!component)
return -EINVAL;
+
return hdac_hdmi_jack_port_init(component, &card->dapm);
- return 0;
+ if (err < 0)
+ return err;
+
+ err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
+ if (err) {
+ dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
+ return err;
+ }
+
+ err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
+ if (err) {
+ dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
+ return err;
+ }
+
+ return snd_soc_dapm_sync(dapm);
}
/* kabylake audio machine driver for SPT + DA7219 */
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-03-05 2:22 ` [PATCH v2] " mac.chiang
@ 2019-03-05 4:03 ` Pierre-Louis Bossart
2019-03-05 5:42 ` Chiang, Mac
2019-03-08 5:16 ` [PATCH v3] " mac.chiang
1 sibling, 1 reply; 13+ messages in thread
From: Pierre-Louis Bossart @ 2019-03-05 4:03 UTC (permalink / raw)
To: mac.chiang, alsa-devel; +Cc: harshapriya.n, tiwai, broonie, jenny.tc
>
> static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
> @@ -864,6 +914,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
> {
> struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
> struct kbl_hdmi_pcm *pcm;
> + struct snd_soc_dapm_context *dapm = &card->dapm;
> struct snd_soc_component *component = NULL;
> int err, i = 0;
> char jack_name[NAME_SIZE];
> @@ -890,9 +941,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
> if (!component)
> return -EINVAL;
>
> +
> return hdac_hdmi_jack_port_init(component, &card->dapm);
That part looks quite broken. You have functional code after the
unconditional return above for the jack init.
This is not a diff illusion, I applied this patch and looked at the diff
with emacs and I don't know how this could possibly work.
The code prior to this patch was also weird with 2 returns at the end of
kabylake_card_late_probe()
My take is that this has never been tested against Mark's branch but
ported without tests from the Chrome tree.
>
> - return 0;
> + if (err < 0)
> + return err;
> +
> + err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
> + if (err) {
> + dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
> + return err;
> + }
> +
> + err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
> + if (err) {
> + dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
> + return err;
> + }
> +
> + return snd_soc_dapm_sync(dapm);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-03-05 4:03 ` Pierre-Louis Bossart
@ 2019-03-05 5:42 ` Chiang, Mac
2019-03-05 15:07 ` Pierre-Louis Bossart
0 siblings, 1 reply; 13+ messages in thread
From: Chiang, Mac @ 2019-03-05 5:42 UTC (permalink / raw)
To: Pierre-Louis Bossart, alsa-devel
Cc: N, Harshapriya, tiwai, broonie, Tc, Jenny
>>
>> static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, @@
>> -864,6 +914,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
>> {
>> struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
>> struct kbl_hdmi_pcm *pcm;
>> + struct snd_soc_dapm_context *dapm = &card->dapm;
>> struct snd_soc_component *component = NULL;
>> int err, i = 0;
>> char jack_name[NAME_SIZE];
>> @@ -890,9 +941,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
>> if (!component)
>> return -EINVAL;
>>
>> +
>> return hdac_hdmi_jack_port_init(component, &card->dapm);
>>
>That part looks quite broken. You have functional code after the unconditional return above for the jack init.
>
>This is not a diff illusion, I applied this patch and looked at the diff with emacs and I don't know how this could possibly work.
>The code prior to this patch was also weird with 2 returns at the end of
>kabylake_card_late_probe()
>My take is that this has never been tested against Mark's branch but ported without tests from the Chrome tree.
I'm missing one line change as below. So there is only one return in kabylake_card_late_probe() as prior one. I've tested and verified it on Chrome tree.
error = hdac_hdmi_jack_port_init(component, &card->dapm);
>>
>> - return 0;
>> + if (err < 0)
>> + return err;
>> +
>> + err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
>> + if (err) {
>> + dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
>> + return err;
>> + }
>> +
>> + err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
>> + if (err) {
>> + dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
>> + return err;
>> + }
>> +
>> + return snd_soc_dapm_sync(dapm);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-03-05 5:42 ` Chiang, Mac
@ 2019-03-05 15:07 ` Pierre-Louis Bossart
0 siblings, 0 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2019-03-05 15:07 UTC (permalink / raw)
To: Chiang, Mac, alsa-devel; +Cc: N, Harshapriya, tiwai, broonie, Tc, Jenny
On 3/4/19 11:42 PM, Chiang, Mac wrote:
>>>
>>> static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, @@
>>> -864,6 +914,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
>>> {
>>> struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
>>> struct kbl_hdmi_pcm *pcm;
>>> + struct snd_soc_dapm_context *dapm = &card->dapm;
>>> struct snd_soc_component *component = NULL;
>>> int err, i = 0;
>>> char jack_name[NAME_SIZE];
>>> @@ -890,9 +941,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
>>> if (!component)
>>> return -EINVAL;
>>>
>>> +
>>> return hdac_hdmi_jack_port_init(component, &card->dapm);
>>>
>> That part looks quite broken. You have functional code after the unconditional return above for the jack init.
>>
>> This is not a diff illusion, I applied this patch and looked at the diff with emacs and I don't know how this could possibly work.
>
>> The code prior to this patch was also weird with 2 returns at the end of
>> kabylake_card_late_probe()
>
>> My take is that this has never been tested against Mark's branch but ported without tests from the Chrome tree.
>
> I'm missing one line change as below. So there is only one return in kabylake_card_late_probe() as prior one. I've tested and verified it on Chrome tree.
In case I wasn't clear, your patch needs to be fixed and tested against
Mark's tree. It cannot possibly work as is.
> error = hdac_hdmi_jack_port_init(component, &card->dapm);
>
>>>
>>> - return 0;
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
>>> + if (err) {
>>> + dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
>>> + if (err) {
>>> + dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> + return snd_soc_dapm_sync(dapm);
>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-03-05 2:22 ` [PATCH v2] " mac.chiang
2019-03-05 4:03 ` Pierre-Louis Bossart
@ 2019-03-08 5:16 ` mac.chiang
2019-03-08 15:31 ` Pierre-Louis Bossart
1 sibling, 1 reply; 13+ messages in thread
From: mac.chiang @ 2019-03-08 5:16 UTC (permalink / raw)
To: alsa-devel
Cc: Harshapriya.n, pierre-louis.bossart, tiwai, mac.chiang, broonie,
jenny.tc
From: Mac Chiang <mac.chiang@intel.com>
amplifier feedback is not modeled as being dependent on any active output.
Even when there is no playback happening, parts of the graph, specifically the IV
sense->speaker protection->output remains active and this prevents the DSP from
entering low-power states.
This patch suggest a machine driver level approach where the speaker
pins are enabled/disabled dynamically depending on stream start/stop
events. DPAM graph representations show the feedback loop is indeed
disabled and low-power states can be reached.
Signed-off-by: Jenny TC <jenny.tc@intel.com>
Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
Signed-off-by: Mac Chiang <mac.chiang@intel.com>
---
Changelog:
v3:
- fixed the return logic at the end of kabylake_card_late_probe()
v2:
- described clearly in commit message
- added snd_soc_dapm_disable_pin in kabylake_card_late_probe()
---
sound/soc/intel/boards/kbl_da7219_max98927.c | 71 +++++++++++++++++++++++++++-
1 file changed, 69 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
index 723a493..a9dcc0a 100644
--- a/sound/soc/intel/boards/kbl_da7219_max98927.c
+++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
@@ -195,8 +195,58 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
return 0;
}
+static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ int j, ret;
+
+ for (j = 0; j < rtd->num_codecs; j++) {
+ struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
+ const char *name = codec_dai->component->name;
+ struct snd_soc_component *component = codec_dai->component;
+ struct snd_soc_dapm_context *dapm =
+ snd_soc_component_get_dapm(component);
+ char pin_name[20];
+
+ if (strcmp(name, MAXIM_DEV0_NAME) &&
+ strcmp(name, MAXIM_DEV1_NAME))
+ continue;
+
+ snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
+ codec_dai->component->name_prefix);
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ ret = snd_soc_dapm_enable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to enable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ ret = snd_soc_dapm_disable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to disable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ }
+ }
+
+ return 0;
+}
+
static struct snd_soc_ops kabylake_ssp0_ops = {
.hw_params = kabylake_ssp0_hw_params,
+ .trigger = kabylake_ssp0_trigger,
};
static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
@@ -864,6 +914,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
{
struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
struct kbl_hdmi_pcm *pcm;
+ struct snd_soc_dapm_context *dapm = &card->dapm;
struct snd_soc_component *component = NULL;
int err, i = 0;
char jack_name[NAME_SIZE];
@@ -890,9 +941,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
if (!component)
return -EINVAL;
- return hdac_hdmi_jack_port_init(component, &card->dapm);
- return 0;
+ err = hdac_hdmi_jack_port_init(component, &card->dapm);
+
+ if (err < 0)
+ return err;
+
+ err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
+ if (err) {
+ dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
+ return err;
+ }
+
+ err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
+ if (err) {
+ dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
+ return err;
+ }
+
+ return snd_soc_dapm_sync(dapm);
}
/* kabylake audio machine driver for SPT + DA7219 */
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-03-08 5:16 ` [PATCH v3] " mac.chiang
@ 2019-03-08 15:31 ` Pierre-Louis Bossart
0 siblings, 0 replies; 13+ messages in thread
From: Pierre-Louis Bossart @ 2019-03-08 15:31 UTC (permalink / raw)
To: mac.chiang, alsa-devel; +Cc: Harshapriya.n, tiwai, broonie, jenny.tc
On 3/7/19 11:16 PM, mac.chiang@intel.com wrote:
> From: Mac Chiang <mac.chiang@intel.com>
>
> amplifier feedback is not modeled as being dependent on any active output.
> Even when there is no playback happening, parts of the graph, specifically the IV
> sense->speaker protection->output remains active and this prevents the DSP from
> entering low-power states.
>
> This patch suggest a machine driver level approach where the speaker
> pins are enabled/disabled dynamically depending on stream start/stop
> events. DPAM graph representations show the feedback loop is indeed
> disabled and low-power states can be reached.
I can't think of a better solution so
Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
> Signed-off-by: Mac Chiang <mac.chiang@intel.com>
> ---
> Changelog:
> v3:
> - fixed the return logic at the end of kabylake_card_late_probe()
> v2:
> - described clearly in commit message
> - added snd_soc_dapm_disable_pin in kabylake_card_late_probe()
> ---
> sound/soc/intel/boards/kbl_da7219_max98927.c | 71 +++++++++++++++++++++++++++-
> 1 file changed, 69 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
> index 723a493..a9dcc0a 100644
> --- a/sound/soc/intel/boards/kbl_da7219_max98927.c
> +++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
> @@ -195,8 +195,58 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
> return 0;
> }
>
> +static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + int j, ret;
> +
> + for (j = 0; j < rtd->num_codecs; j++) {
> + struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
> + const char *name = codec_dai->component->name;
> + struct snd_soc_component *component = codec_dai->component;
> + struct snd_soc_dapm_context *dapm =
> + snd_soc_component_get_dapm(component);
> + char pin_name[20];
> +
> + if (strcmp(name, MAXIM_DEV0_NAME) &&
> + strcmp(name, MAXIM_DEV1_NAME))
> + continue;
> +
> + snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
> + codec_dai->component->name_prefix);
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + ret = snd_soc_dapm_enable_pin(dapm, pin_name);
> + if (ret) {
> + dev_err(rtd->dev, "failed to enable %s: %d\n",
> + pin_name, ret);
> + return ret;
> + }
> + snd_soc_dapm_sync(dapm);
> + break;
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + ret = snd_soc_dapm_disable_pin(dapm, pin_name);
> + if (ret) {
> + dev_err(rtd->dev, "failed to disable %s: %d\n",
> + pin_name, ret);
> + return ret;
> + }
> + snd_soc_dapm_sync(dapm);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> static struct snd_soc_ops kabylake_ssp0_ops = {
> .hw_params = kabylake_ssp0_hw_params,
> + .trigger = kabylake_ssp0_trigger,
> };
>
> static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
> @@ -864,6 +914,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
> {
> struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
> struct kbl_hdmi_pcm *pcm;
> + struct snd_soc_dapm_context *dapm = &card->dapm;
> struct snd_soc_component *component = NULL;
> int err, i = 0;
> char jack_name[NAME_SIZE];
> @@ -890,9 +941,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
> if (!component)
> return -EINVAL;
>
> - return hdac_hdmi_jack_port_init(component, &card->dapm);
>
> - return 0;
> + err = hdac_hdmi_jack_port_init(component, &card->dapm);
> +
> + if (err < 0)
> + return err;
> +
> + err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
> + if (err) {
> + dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
> + return err;
> + }
> +
> + err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
> + if (err) {
> + dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
> + return err;
> + }
> +
> + return snd_soc_dapm_sync(dapm);
> }
>
> /* kabylake audio machine driver for SPT + DA7219 */
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
@ 2019-05-02 7:06 mac.chiang
2019-05-02 16:08 ` Pierre-Louis Bossart
0 siblings, 1 reply; 13+ messages in thread
From: mac.chiang @ 2019-05-02 7:06 UTC (permalink / raw)
To: alsa-devel
Cc: mac.chiang, broonie, chintan.m.patel, pierre-louis.bossart, jenny.tc
From: Mac Chiang <mac.chiang@intel.com>
amplifier feedback is not modeled as being dependent on any active
output. Even when there is no playback happening, parts of the graph,
specifically the IV sense->speaker protection->output remains active
and this prevents the DSP from entering low-power states.
This patch suggest a machine driver level approach where the speaker
pins are enabled/disabled dynamically depending on stream start/stop
events. DPAM graph representations show the feedback loop is indeed
disabled and low-power states can be reached.
Signed-off-by: Jenny TC <jenny.tc@intel.com>
Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
Signed-off-by: Mac Chiang <mac.chiang@intel.com>
---
sound/soc/intel/boards/kbl_da7219_max98927.c | 73 +++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
index f72a7bf..1efe7fd 100644
--- a/sound/soc/intel/boards/kbl_da7219_max98927.c
+++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
@@ -219,8 +219,60 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
return 0;
}
+static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ int j, ret;
+
+ for (j = 0; j < rtd->num_codecs; j++) {
+ struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
+ const char *name = codec_dai->component->name;
+ struct snd_soc_component *component = codec_dai->component;
+ struct snd_soc_dapm_context *dapm =
+ snd_soc_component_get_dapm(component);
+ char pin_name[20];
+
+ if (strcmp(name, MAX98927_DEV0_NAME) &&
+ strcmp(name, MAX98927_DEV1_NAME) &&
+ strcmp(name, MAX98373_DEV0_NAME) &&
+ strcmp(name, MAX98373_DEV1_NAME))
+ continue;
+
+ snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
+ codec_dai->component->name_prefix);
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ ret = snd_soc_dapm_enable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to enable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ ret = snd_soc_dapm_disable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to disable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ }
+ }
+
+ return 0;
+}
+
static struct snd_soc_ops kabylake_ssp0_ops = {
.hw_params = kabylake_ssp0_hw_params,
+ .trigger = kabylake_ssp0_trigger,
};
static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
@@ -950,6 +1002,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
{
struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
struct kbl_hdmi_pcm *pcm;
+ struct snd_soc_dapm_context *dapm = &card->dapm;
struct snd_soc_component *component = NULL;
int err, i = 0;
char jack_name[NAME_SIZE];
@@ -976,9 +1029,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
if (!component)
return -EINVAL;
- return hdac_hdmi_jack_port_init(component, &card->dapm);
- return 0;
+ err = hdac_hdmi_jack_port_init(component, &card->dapm);
+
+ if (err < 0)
+ return err;
+
+ err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
+ if (err) {
+ dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
+ return err;
+ }
+
+ err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
+ if (err) {
+ dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
+ return err;
+ }
+
+ return snd_soc_dapm_sync(dapm);
}
/* kabylake audio machine driver for SPT + DA7219 */
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-05-02 7:06 [PATCH] " mac.chiang
@ 2019-05-02 16:08 ` Pierre-Louis Bossart
2019-05-03 2:50 ` Chiang, Mac
0 siblings, 1 reply; 13+ messages in thread
From: Pierre-Louis Bossart @ 2019-05-02 16:08 UTC (permalink / raw)
To: mac.chiang, alsa-devel; +Cc: broonie, chintan.m.patel, jenny.tc
On 5/2/19 2:06 AM, mac.chiang@intel.com wrote:
> From: Mac Chiang <mac.chiang@intel.com>
>
> amplifier feedback is not modeled as being dependent on any active
> output. Even when there is no playback happening, parts of the graph,
> specifically the IV sense->speaker protection->output remains active
> and this prevents the DSP from entering low-power states.
>
> This patch suggest a machine driver level approach where the speaker
> pins are enabled/disabled dynamically depending on stream start/stop
> events. DPAM graph representations show the feedback loop is indeed
> disabled and low-power states can be reached.
>
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
> Signed-off-by: Mac Chiang <mac.chiang@intel.com>
I am confused here.
There was a first patch sent on March 1, then updates posted by Jenny TC
with a v3 that I acked on March 8, but there's also a v4 on April 1.
So is this the same as v4? if yes why did you drop the change log or not
add a note it's a resend? If no, what has changed?
> ---
> sound/soc/intel/boards/kbl_da7219_max98927.c | 73 +++++++++++++++++++++++++++-
> 1 file changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
> index f72a7bf..1efe7fd 100644
> --- a/sound/soc/intel/boards/kbl_da7219_max98927.c
> +++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
> @@ -219,8 +219,60 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
> return 0;
> }
>
> +static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + int j, ret;
> +
> + for (j = 0; j < rtd->num_codecs; j++) {
> + struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
> + const char *name = codec_dai->component->name;
> + struct snd_soc_component *component = codec_dai->component;
> + struct snd_soc_dapm_context *dapm =
> + snd_soc_component_get_dapm(component);
> + char pin_name[20];
> +
> + if (strcmp(name, MAX98927_DEV0_NAME) &&
> + strcmp(name, MAX98927_DEV1_NAME) &&
> + strcmp(name, MAX98373_DEV0_NAME) &&
> + strcmp(name, MAX98373_DEV1_NAME))
> + continue;
> +
> + snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
> + codec_dai->component->name_prefix);
> +
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_START:
> + case SNDRV_PCM_TRIGGER_RESUME:
> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> + ret = snd_soc_dapm_enable_pin(dapm, pin_name);
> + if (ret) {
> + dev_err(rtd->dev, "failed to enable %s: %d\n",
> + pin_name, ret);
> + return ret;
> + }
> + snd_soc_dapm_sync(dapm);
> + break;
> + case SNDRV_PCM_TRIGGER_STOP:
> + case SNDRV_PCM_TRIGGER_SUSPEND:
> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> + ret = snd_soc_dapm_disable_pin(dapm, pin_name);
> + if (ret) {
> + dev_err(rtd->dev, "failed to disable %s: %d\n",
> + pin_name, ret);
> + return ret;
> + }
> + snd_soc_dapm_sync(dapm);
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> static struct snd_soc_ops kabylake_ssp0_ops = {
> .hw_params = kabylake_ssp0_hw_params,
> + .trigger = kabylake_ssp0_trigger,
> };
>
> static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
> @@ -950,6 +1002,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
> {
> struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
> struct kbl_hdmi_pcm *pcm;
> + struct snd_soc_dapm_context *dapm = &card->dapm;
> struct snd_soc_component *component = NULL;
> int err, i = 0;
> char jack_name[NAME_SIZE];
> @@ -976,9 +1029,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
> if (!component)
> return -EINVAL;
>
> - return hdac_hdmi_jack_port_init(component, &card->dapm);
>
> - return 0;
> + err = hdac_hdmi_jack_port_init(component, &card->dapm);
> +
> + if (err < 0)
> + return err;
> +
> + err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
> + if (err) {
> + dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
> + return err;
> + }
> +
> + err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
> + if (err) {
> + dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
> + return err;
> + }
> +
> + return snd_soc_dapm_sync(dapm);
> }
>
> /* kabylake audio machine driver for SPT + DA7219 */
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
2019-05-02 16:08 ` Pierre-Louis Bossart
@ 2019-05-03 2:50 ` Chiang, Mac
0 siblings, 0 replies; 13+ messages in thread
From: Chiang, Mac @ 2019-05-03 2:50 UTC (permalink / raw)
To: Pierre-Louis Bossart, alsa-devel; +Cc: broonie, Patel, Chintan M, Tc, Jenny
>> From: Mac Chiang <mac.chiang@intel.com>
>>
>> amplifier feedback is not modeled as being dependent on any active
>> output. Even when there is no playback happening, parts of the graph,
>> specifically the IV sense->speaker protection->output remains active
>> and this prevents the DSP from entering low-power states.
>>
>> This patch suggest a machine driver level approach where the speaker
>> pins are enabled/disabled dynamically depending on stream start/stop
>> events. DPAM graph representations show the feedback loop is indeed
>> disabled and low-power states can be reached.
>>
>> Signed-off-by: Jenny TC <jenny.tc@intel.com>
>> Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
>> Signed-off-by: Mac Chiang <mac.chiang@intel.com>
> I am confused here.
> There was a first patch sent on March 1, then updates posted by Jenny TC with a v3 that I acked on March 8, but there's also a v4 on April 1.
> So is this the same as v4? if yes why did you drop the change log or not add a note it's a resend? If no, what has changed?
Yes, it's the same as v4. I pick up it directly and should notice the change log missing. Sorry for spam this, I just resend it[patch resend] with change log manually.
>> ---
>> sound/soc/intel/boards/kbl_da7219_max98927.c | 73 +++++++++++++++++++++++++++-
>> 1 file changed, 71 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c
>> b/sound/soc/intel/boards/kbl_da7219_max98927.c
>> index f72a7bf..1efe7fd 100644
>> --- a/sound/soc/intel/boards/kbl_da7219_max98927.c
>> +++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
>> @@ -219,8 +219,60 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
>> return 0;
>> }
>>
>> +static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream,
>> +int cmd) {
>> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> + int j, ret;
>> +
>> + for (j = 0; j < rtd->num_codecs; j++) {
>> + struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
>> + const char *name = codec_dai->component->name;
>> + struct snd_soc_component *component = codec_dai->component;
>> + struct snd_soc_dapm_context *dapm =
>> + snd_soc_component_get_dapm(component);
>> + char pin_name[20];
>> +
>> + if (strcmp(name, MAX98927_DEV0_NAME) &&
>> + strcmp(name, MAX98927_DEV1_NAME) &&
>> + strcmp(name, MAX98373_DEV0_NAME) &&
>> + strcmp(name, MAX98373_DEV1_NAME))
>> + continue;
>> +
>> + snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
>> + codec_dai->component->name_prefix);
>> +
>> + switch (cmd) {
>> + case SNDRV_PCM_TRIGGER_START:
>> + case SNDRV_PCM_TRIGGER_RESUME:
>> + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> + ret = snd_soc_dapm_enable_pin(dapm, pin_name);
>> + if (ret) {
>> + dev_err(rtd->dev, "failed to enable %s: %d\n",
>> + pin_name, ret);
>> + return ret;
>> + }
>> + snd_soc_dapm_sync(dapm);
>> + break;
>> + case SNDRV_PCM_TRIGGER_STOP:
>> + case SNDRV_PCM_TRIGGER_SUSPEND:
>> + case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> + ret = snd_soc_dapm_disable_pin(dapm, pin_name);
>> + if (ret) {
>> + dev_err(rtd->dev, "failed to disable %s: %d\n",
>> + pin_name, ret);
>> + return ret;
>> + }
>> + snd_soc_dapm_sync(dapm);
>> + break;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static struct snd_soc_ops kabylake_ssp0_ops = {
>> .hw_params = kabylake_ssp0_hw_params,
>> + .trigger = kabylake_ssp0_trigger,
>> };
>>
>> static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, @@
>> -950,6 +1002,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
>> {
>> struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
>> struct kbl_hdmi_pcm *pcm;
>> + struct snd_soc_dapm_context *dapm = &card->dapm;
>> struct snd_soc_component *component = NULL;
>> int err, i = 0;
>> char jack_name[NAME_SIZE];
>> @@ -976,9 +1029,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
>> if (!component)
>> return -EINVAL;
>>
>> - return hdac_hdmi_jack_port_init(component, &card->dapm);
>>
>> - return 0;
>> + err = hdac_hdmi_jack_port_init(component, &card->dapm);
>> +
>> + if (err < 0)
>> + return err;
>> +
>> + err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
>> + if (err) {
>> + dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
>> + return err;
>> + }
>> +
>> + err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
>> + if (err) {
>> + dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
>> + return err;
>> + }
>> +
>> + return snd_soc_dapm_sync(dapm);
>> }
>>
>> /* kabylake audio machine driver for SPT + DA7219 */
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function
@ 2019-05-03 2:27 mac.chiang
0 siblings, 0 replies; 13+ messages in thread
From: mac.chiang @ 2019-05-03 2:27 UTC (permalink / raw)
To: alsa-devel
Cc: mac.chiang, broonie, chintan.m.patel, pierre-louis.bossart, jenny.tc
From: Mac Chiang <mac.chiang@intel.com>
amplifier feedback is not modeled as being dependent on any active
output. Even when there is no playback happening, parts of the graph,
specifically the IV sense->speaker protection->output remains active
and this prevents the DSP from entering low-power states.
This patch suggest a machine driver level approach where the speaker
pins are enabled/disabled dynamically depending on stream start/stop
events. DPAM graph representations show the feedback loop is indeed
disabled and low-power states can be reached.
Signed-off-by: Jenny TC <jenny.tc@intel.com>
Signed-off-by: Harshapriya.n <harshapriya.n@intel.com>
Signed-off-by: Mac Chiang <mac.chiang@intel.com>
---
Changelog:
v4:
- Changed device name in in kabylake_ssp0_trigger to address a
conflict with recently merged patch
MAXIM_DEV0_NAME -> MAX98927_DEV0_NAME ...
v3:
- fixed the return logic at the end of kabylake_card_late_probe()
v2:
- described clearly in commit message
- added snd_soc_dapm_disable_pin in kabylake_card_late_probe()
---
---
sound/soc/intel/boards/kbl_da7219_max98927.c | 73 +++++++++++++++++++++++++++-
1 file changed, 71 insertions(+), 2 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c
index f72a7bf..1efe7fd 100644
--- a/sound/soc/intel/boards/kbl_da7219_max98927.c
+++ b/sound/soc/intel/boards/kbl_da7219_max98927.c
@@ -219,8 +219,60 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream,
return 0;
}
+static int kabylake_ssp0_trigger(struct snd_pcm_substream *substream, int cmd)
+{
+ struct snd_soc_pcm_runtime *rtd = substream->private_data;
+ int j, ret;
+
+ for (j = 0; j < rtd->num_codecs; j++) {
+ struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
+ const char *name = codec_dai->component->name;
+ struct snd_soc_component *component = codec_dai->component;
+ struct snd_soc_dapm_context *dapm =
+ snd_soc_component_get_dapm(component);
+ char pin_name[20];
+
+ if (strcmp(name, MAX98927_DEV0_NAME) &&
+ strcmp(name, MAX98927_DEV1_NAME) &&
+ strcmp(name, MAX98373_DEV0_NAME) &&
+ strcmp(name, MAX98373_DEV1_NAME))
+ continue;
+
+ snprintf(pin_name, ARRAY_SIZE(pin_name), "%s Spk",
+ codec_dai->component->name_prefix);
+
+ switch (cmd) {
+ case SNDRV_PCM_TRIGGER_START:
+ case SNDRV_PCM_TRIGGER_RESUME:
+ case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+ ret = snd_soc_dapm_enable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to enable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ case SNDRV_PCM_TRIGGER_STOP:
+ case SNDRV_PCM_TRIGGER_SUSPEND:
+ case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+ ret = snd_soc_dapm_disable_pin(dapm, pin_name);
+ if (ret) {
+ dev_err(rtd->dev, "failed to disable %s: %d\n",
+ pin_name, ret);
+ return ret;
+ }
+ snd_soc_dapm_sync(dapm);
+ break;
+ }
+ }
+
+ return 0;
+}
+
static struct snd_soc_ops kabylake_ssp0_ops = {
.hw_params = kabylake_ssp0_hw_params,
+ .trigger = kabylake_ssp0_trigger,
};
static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd,
@@ -950,6 +1002,7 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
{
struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(card);
struct kbl_hdmi_pcm *pcm;
+ struct snd_soc_dapm_context *dapm = &card->dapm;
struct snd_soc_component *component = NULL;
int err, i = 0;
char jack_name[NAME_SIZE];
@@ -976,9 +1029,25 @@ static int kabylake_card_late_probe(struct snd_soc_card *card)
if (!component)
return -EINVAL;
- return hdac_hdmi_jack_port_init(component, &card->dapm);
- return 0;
+ err = hdac_hdmi_jack_port_init(component, &card->dapm);
+
+ if (err < 0)
+ return err;
+
+ err = snd_soc_dapm_disable_pin(dapm, "Left Spk");
+ if (err) {
+ dev_err(card->dev, "failed to disable Left Spk: %d\n", err);
+ return err;
+ }
+
+ err = snd_soc_dapm_disable_pin(dapm, "Right Spk");
+ if (err) {
+ dev_err(card->dev, "failed to disable Right Spk: %d\n", err);
+ return err;
+ }
+
+ return snd_soc_dapm_sync(dapm);
}
/* kabylake audio machine driver for SPT + DA7219 */
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-05-03 2:50 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-01 17:14 [PATCH] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function mac.chiang
2019-03-01 18:06 ` Pierre-Louis Bossart
2019-03-04 2:33 ` Chiang, Mac
2019-03-05 2:22 ` [PATCH v2] " mac.chiang
2019-03-05 4:03 ` Pierre-Louis Bossart
2019-03-05 5:42 ` Chiang, Mac
2019-03-05 15:07 ` Pierre-Louis Bossart
2019-03-08 5:16 ` [PATCH v3] " mac.chiang
2019-03-08 15:31 ` Pierre-Louis Bossart
2019-05-02 7:06 [PATCH] " mac.chiang
2019-05-02 16:08 ` Pierre-Louis Bossart
2019-05-03 2:50 ` Chiang, Mac
2019-05-03 2:27 mac.chiang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.