| Message ID | 87o8w6jqrk.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: soc-core cleanup step8 | expand |
On Tue, Dec 17, 2019 at 6:47 PM Kuninori Morimoto < kuninori.morimoto.gx@renesas.com> wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Card dai_link has .ignore_suspend, and ALSA SoC cares it when suspend. > For example, like this > > for_each_card_rtds(card, rtd) { > if (rtd->dai_link->ignore_suspend) > continue; > ... > } > > But in snd_soc_suspend(), it doesn't care about > it when suspending Component. This patch cares it. > Morimoto-san, I am a bit skeptical about this change but I could be wrong. I am not sure if the ignore_suspend field in the DAI link definitions was meant to be applicable for the components as well. Mark/Takashi would have to confirm this. Thanks, Ranjani
Hi Sridharan Cc Mark > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Card dai_link has .ignore_suspend, and ALSA SoC cares it when suspend. > For example, like this > > for_each_card_rtds(card, rtd) { > if (rtd->dai_link->ignore_suspend) > continue; > ... > } > > But in snd_soc_suspend(), it doesn't care about > it when suspending Component. This patch cares it. > > Morimoto-san, > > I am a bit skeptical about this change but I could be wrong. > I am not sure if the ignore_suspend field in the DAI link > definitions was meant to be applicable for the components as well. > Mark/Takashi would have to confirm this. Hmm.. from naming point of view, it has no problem, but I'm not sure detail, either. It is just one of cleanup patch, thus, I have no big objection if it was not accepted. Mark I think this patch doesn't have effect to other patches. You can just ignore it if you can't accept. But, please let me know if you have some issues. I can post v3 Thank you for your help !! Best regards --- Kuninori Morimoto
>> Card dai_link has .ignore_suspend, and ALSA SoC cares it when suspend. >> For example, like this >> >> for_each_card_rtds(card, rtd) { >> if (rtd->dai_link->ignore_suspend) >> continue; >> ... >> } >> >> But in snd_soc_suspend(), it doesn't care about >> it when suspending Component. This patch cares it. >> >> Morimoto-san, >> >> I am a bit skeptical about this change but I could be wrong. >> I am not sure if the ignore_suspend field in the DAI link >> definitions was meant to be applicable for the components as well. >> Mark/Takashi would have to confirm this. Even for dai links, it's not clear to me what .ignore_suspend means. For Intel machine drivers, the .ignore_suspend flag is used for DMIC links which may be used in S0ix/D0ix states. I don't believe this works if there are multiple target states, e.g. you would probably want to leave the link active in S0ix, but suspend it in S3? I think the current .ignore_suspend settings only work with the assumption that applications will close all capture streams before going to S3.
On Wed, Dec 18, 2019 at 08:54:27AM -0600, Pierre-Louis Bossart wrote: > Even for dai links, it's not clear to me what .ignore_suspend means. It means exactly what it says, don't do anything on suspend. > For Intel machine drivers, the .ignore_suspend flag is used for DMIC links > which may be used in S0ix/D0ix states. I don't believe this works if there > are multiple target states, e.g. you would probably want to leave the link > active in S0ix, but suspend it in S3? > I think the current .ignore_suspend settings only work with the assumption > that applications will close all capture streams before going to S3. These numbered suspend states are a platform specific concept, other systems will typically just suspend or not suspend. It sounds like this feature does not map onto your systems.
On 12/18/19 11:14 AM, Mark Brown wrote: > On Wed, Dec 18, 2019 at 08:54:27AM -0600, Pierre-Louis Bossart wrote: > >> Even for dai links, it's not clear to me what .ignore_suspend means. > > It means exactly what it says, don't do anything on suspend. > >> For Intel machine drivers, the .ignore_suspend flag is used for DMIC links >> which may be used in S0ix/D0ix states. I don't believe this works if there >> are multiple target states, e.g. you would probably want to leave the link >> active in S0ix, but suspend it in S3? > >> I think the current .ignore_suspend settings only work with the assumption >> that applications will close all capture streams before going to S3. > > These numbered suspend states are a platform specific concept, other > systems will typically just suspend or not suspend. It sounds like this > feature does not map onto your systems. Correct. What is not clear to me is how we can specify a behavior that would be dependent on the target states.
On Wed, Dec 18, 2019 at 11:48:18AM -0600, Pierre-Louis Bossart wrote: > On 12/18/19 11:14 AM, Mark Brown wrote: > > These numbered suspend states are a platform specific concept, other > > systems will typically just suspend or not suspend. It sounds like this > > feature does not map onto your systems. > Correct. What is not clear to me is how we can specify a behavior that would > be dependent on the target states. You can't currently, you'd need to extend the framework to support that.
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 8e49fb8..c10b9af 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -562,15 +562,25 @@ int snd_soc_suspend(struct device *dev) snd_soc_dapm_sync(&card->dapm); /* suspend all COMPONENTs */ - for_each_card_components(card, component) { - struct snd_soc_dapm_context *dapm = + for_each_card_rtds(card, rtd) { + + if (rtd->dai_link->ignore_suspend) + continue; + + for_each_rtd_components(rtd, i, component) { + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component); - /* - * If there are paths active then the COMPONENT will be held - * with bias _ON and should not be suspended. - */ - if (!snd_soc_component_is_suspended(component)) { + /* + * ignore if component was already suspended + */ + if (snd_soc_component_is_suspended(component)) + continue; + + /* + * If there are paths active then the COMPONENT will be + * held with bias _ON and should not be suspended. + */ switch (snd_soc_dapm_get_bias_level(dapm)) { case SND_SOC_BIAS_STANDBY: /*