diff mbox series

[v2] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function

Message ID 1551752535-27367-1-git-send-email-mac.chiang@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] ASoC: Intel: boards: kbl_da7219_max98927: add dai_trigger function | expand

Commit Message

Chiang, Mac March 5, 2019, 2:22 a.m. UTC 
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(-)

Comments

Pierre-Louis Bossart March 5, 2019, 4:03 a.m. UTC | #1 
>   
>   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);
Chiang, Mac March 5, 2019, 5:42 a.m. UTC | #2 
>>   
>>   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);
Pierre-Louis Bossart March 5, 2019, 3:07 p.m. UTC | #3 
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
>
diff mbox series

Patch

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 */