[PATCH v2] ASoC: Add pinctrl PM to components of active DAIs

[PATCH v2] ASoC: Add pinctrl PM to components of active DAIs

[Nicolin Chen]

It's quite popular that more drivers are using pinctrl PM, for example:
(Documentation/devicetree/bindings/arm/primecell.txt). Just like what
runtime PM does, it would de-active and en-active pin group depending
on whether it's being used or not.

And this pinctrl PM might be also beneficial to cpu dai drivers because
they might have actual pinctrl so as to sleep their pins and wake them
up as needed.

To achieve this goal, this patch sets pins to the default state during
resume or startup; While during suspend and shutdown, it would set pins
to the sleep state.

As pinctrl PM would return zero if there is no such pinctrl sleep state
settings, this patch would not break current ASoC subsystem directly.

[ However, there is still an exception that the patch can not handle,
that is, when cpu dai driver does not have pinctrl property but another
device has it. (The AUDMUX <-> SSI on Freescale i.MX6 series for example.
SSI as a cpu dai doesn't contain pinctrl property while AUDMUX, an Audio
Multiplexer, has it). In this case, this kind of cpu dai driver needs to
find a way to obtain the pinctrl property as its own, by moving property
from AUDMUX to SSI, or creating a pins link/dependency between these two
devices, or using a more decent way after we figure it out. ]

Signed-off-by: Nicolin Chen <b42378@freescale.com>
---
Changelog
v1->v2:
 * Use proper verbs in comments.
 * Add 'dai->active' condition for resume case.

---
 sound/soc/soc-core.c | 29 +++++++++++++++++++++++++++++
 sound/soc/soc-pcm.c  |  3 +++
 2 files changed, 32 insertions(+)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4280c70..5c972ac 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -684,6 +684,13 @@ int snd_soc_suspend(struct device *dev)
 	if (card->suspend_post)
 		card->suspend_post(card);
 
+	/* deactivate pins to sleep state */
+	for (i = 0; i < card->num_rtd; i++) {
+		struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
+		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		pinctrl_pm_select_sleep_state(cpu_dai->dev);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_suspend);
@@ -807,6 +814,14 @@ int snd_soc_resume(struct device *dev)
 	if (list_empty(&card->codec_dev_list))
 		return 0;
 
+	/* activate pins from sleep state */
+	for (i = 0; i < card->num_rtd; i++) {
+		struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
+		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		if (cpu_dai->active)
+			pinctrl_pm_select_default_state(cpu_dai->dev);
+	}
+
 	/* AC97 devices might have other drivers hanging off them so
 	 * need to resume immediately.  Other drivers don't have that
 	 * problem and may take a substantial amount of time to resume
@@ -1929,6 +1944,13 @@ int snd_soc_poweroff(struct device *dev)
 
 	snd_soc_dapm_shutdown(card);
 
+	/* deactivate pins to sleep state */
+	for (i = 0; i < card->num_rtd; i++) {
+		struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
+		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		pinctrl_pm_select_sleep_state(cpu_dai->dev);
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_poweroff);
@@ -3766,6 +3788,13 @@ int snd_soc_register_card(struct snd_soc_card *card)
 	if (ret != 0)
 		soc_cleanup_card_debugfs(card);
 
+	/* deactivate pins to sleep state */
+	for (i = 0; i < card->num_rtd; i++) {
+		struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
+		struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+		pinctrl_pm_select_sleep_state(cpu_dai->dev);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_register_card);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 330c9a6..05bdba0 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -183,6 +183,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	struct snd_soc_dai_driver *codec_dai_drv = codec_dai->driver;
 	int ret = 0;
 
+	pinctrl_pm_select_default_state(cpu_dai->dev);
 	pm_runtime_get_sync(cpu_dai->dev);
 	pm_runtime_get_sync(codec_dai->dev);
 	pm_runtime_get_sync(platform->dev);
@@ -317,6 +318,7 @@ out:
 	pm_runtime_put(platform->dev);
 	pm_runtime_put(codec_dai->dev);
 	pm_runtime_put(cpu_dai->dev);
+	pinctrl_pm_select_sleep_state(cpu_dai->dev);
 
 	return ret;
 }
@@ -426,6 +428,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	pm_runtime_put(platform->dev);
 	pm_runtime_put(codec_dai->dev);
 	pm_runtime_put(cpu_dai->dev);
+	pinctrl_pm_select_sleep_state(cpu_dai->dev);
 
 	return 0;
 }
-- 
1.8.4

Re: [PATCH v2] ASoC: Add pinctrl PM to components of active DAIs

[Kyungmin Park]

On Tue, Oct 29, 2013 at 12:12 PM, Nicolin Chen <b42378@freescale.com> wrote:
> It's quite popular that more drivers are using pinctrl PM, for example:
> (Documentation/devicetree/bindings/arm/primecell.txt). Just like what
> runtime PM does, it would de-active and en-active pin group depending
> on whether it's being used or not.
>
> And this pinctrl PM might be also beneficial to cpu dai drivers because
> they might have actual pinctrl so as to sleep their pins and wake them
> up as needed.
>
> To achieve this goal, this patch sets pins to the default state during
> resume or startup; While during suspend and shutdown, it would set pins
> to the sleep state.
>
> As pinctrl PM would return zero if there is no such pinctrl sleep state
> settings, this patch would not break current ASoC subsystem directly.
>
> [ However, there is still an exception that the patch can not handle,
> that is, when cpu dai driver does not have pinctrl property but another
> device has it. (The AUDMUX <-> SSI on Freescale i.MX6 series for example.
> SSI as a cpu dai doesn't contain pinctrl property while AUDMUX, an Audio
> Multiplexer, has it). In this case, this kind of cpu dai driver needs to
> find a way to obtain the pinctrl property as its own, by moving property
> from AUDMUX to SSI, or creating a pins link/dependency between these two
> devices, or using a more decent way after we figure it out. ]
>
> Signed-off-by: Nicolin Chen <b42378@freescale.com>
> ---
> Changelog
> v1->v2:
>  * Use proper verbs in comments.
>  * Add 'dai->active' condition for resume case.
>
> ---
>  sound/soc/soc-core.c | 29 +++++++++++++++++++++++++++++
>  sound/soc/soc-pcm.c  |  3 +++
>  2 files changed, 32 insertions(+)
>
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 4280c70..5c972ac 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -684,6 +684,13 @@ int snd_soc_suspend(struct device *dev)
>         if (card->suspend_post)
>                 card->suspend_post(card);
>
> +       /* deactivate pins to sleep state */
> +       for (i = 0; i < card->num_rtd; i++) {
> +               struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
> +               struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +               pinctrl_pm_select_sleep_state(cpu_dai->dev);

I wonder doesn't check it's active or not? if codec is used during
suspend. it doesn't set sleep state?

Thank you,
Kyungmin Park
> +       }
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_suspend);
> @@ -807,6 +814,14 @@ int snd_soc_resume(struct device *dev)
>         if (list_empty(&card->codec_dev_list))
>                 return 0;
>
> +       /* activate pins from sleep state */
> +       for (i = 0; i < card->num_rtd; i++) {
> +               struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
> +               struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +               if (cpu_dai->active)
> +                       pinctrl_pm_select_default_state(cpu_dai->dev);
> +       }
> +
>         /* AC97 devices might have other drivers hanging off them so
>          * need to resume immediately.  Other drivers don't have that
>          * problem and may take a substantial amount of time to resume
> @@ -1929,6 +1944,13 @@ int snd_soc_poweroff(struct device *dev)
>
>         snd_soc_dapm_shutdown(card);
>
> +       /* deactivate pins to sleep state */
> +       for (i = 0; i < card->num_rtd; i++) {
> +               struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
> +               struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +               pinctrl_pm_select_sleep_state(cpu_dai->dev);
> +       }
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_poweroff);
> @@ -3766,6 +3788,13 @@ int snd_soc_register_card(struct snd_soc_card *card)
>         if (ret != 0)
>                 soc_cleanup_card_debugfs(card);
>
> +       /* deactivate pins to sleep state */
> +       for (i = 0; i < card->num_rtd; i++) {
> +               struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
> +               struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +               pinctrl_pm_select_sleep_state(cpu_dai->dev);
> +       }
> +
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_register_card);
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 330c9a6..05bdba0 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -183,6 +183,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
>         struct snd_soc_dai_driver *codec_dai_drv = codec_dai->driver;
>         int ret = 0;
>
> +       pinctrl_pm_select_default_state(cpu_dai->dev);
>         pm_runtime_get_sync(cpu_dai->dev);
>         pm_runtime_get_sync(codec_dai->dev);
>         pm_runtime_get_sync(platform->dev);
> @@ -317,6 +318,7 @@ out:
>         pm_runtime_put(platform->dev);
>         pm_runtime_put(codec_dai->dev);
>         pm_runtime_put(cpu_dai->dev);
> +       pinctrl_pm_select_sleep_state(cpu_dai->dev);
>
>         return ret;
>  }
> @@ -426,6 +428,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
>         pm_runtime_put(platform->dev);
>         pm_runtime_put(codec_dai->dev);
>         pm_runtime_put(cpu_dai->dev);
> +       pinctrl_pm_select_sleep_state(cpu_dai->dev);
>
>         return 0;
>  }
> --
> 1.8.4
>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] ASoC: Add pinctrl PM to components of active DAIs

[Nicolin Chen]

Hi Kyungmin,

On Tue, Oct 29, 2013 at 12:35:06PM +0900, Kyungmin Park wrote:
> > +       /* deactivate pins to sleep state */
> > +       for (i = 0; i < card->num_rtd; i++) {
> > +               struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
> > +               struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> > +               pinctrl_pm_select_sleep_state(cpu_dai->dev);
> 
> I wonder doesn't check it's active or not? if codec is used during
> suspend. it doesn't set sleep state?

I thought codec wouldn't do anything meaningful to cpu dai since the
whole system is going to suspend. But it should be better to add the
condition here as well. I'll send a v3.

Thank you for the comments,
Nicolin Chen

Re: [PATCH v2] ASoC: Add pinctrl PM to components of active DAIs

[Nicolin Chen]

On Tue, Oct 29, 2013 at 11:25:12AM +0800, Nicolin Chen wrote:
> Hi Kyungmin,
> 
> On Tue, Oct 29, 2013 at 12:35:06PM +0900, Kyungmin Park wrote:
> > > +       /* deactivate pins to sleep state */
> > > +       for (i = 0; i < card->num_rtd; i++) {
> > > +               struct snd_soc_pcm_runtime *rtd = &card->rtd[i];
> > > +               struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> > > +               pinctrl_pm_select_sleep_state(cpu_dai->dev);
> > 
> > I wonder doesn't check it's active or not? if codec is used during
> > suspend. it doesn't set sleep state?
> 
> I thought codec wouldn't do anything meaningful to cpu dai since the
> whole system is going to suspend. But it should be better to add the
> condition here as well. I'll send a v3.

Wait...For those inactive streams, they're already in sleep state so it
doesn't matter to call pinctrl_pm_select_sleep_state() or not; While for
those active streams, if we undo this pinctrl_pm_select_sleep_state(),
their pins're remaining active during suspend without any power-saving.

So I think maybe I was right at the first place that it's okay not to set
a condition here? Since the card is going to suspend, why do we need to
care codec's behavior? Please correct me if I'm wrong.

Thank you,
Nicolin Chen

Re: [PATCH v2] ASoC: Add pinctrl PM to components of active DAIs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 337 bytes --]

On Tue, Oct 29, 2013 at 11:42:20AM +0800, Nicolin Chen wrote:

> So I think maybe I was right at the first place that it's okay not to set
> a condition here? Since the card is going to suspend, why do we need to
> care codec's behavior? Please correct me if I'm wrong.

There's no reason CODECs can't implement pinctrl if they want to.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] ASoC: Add pinctrl PM to components of active DAIs

[Nicolin Chen]

On Tue, Oct 29, 2013 at 09:19:59AM -0700, Mark Brown wrote:
> On Tue, Oct 29, 2013 at 11:42:20AM +0800, Nicolin Chen wrote:
> 
> > So I think maybe I was right at the first place that it's okay not to set
> > a condition here? Since the card is going to suspend, why do we need to
> > care codec's behavior? Please correct me if I'm wrong.
> 
> There's no reason CODECs can't implement pinctrl if they want to.

Hmm..Sorry, I'm little confused. What should I do? Adding a pinctrl_pm
for codec_dai->pins? Or a condition like "if (!codec_dai->active)"?

Thank you,
Nicolin Chen

Re: [PATCH v2] ASoC: Add pinctrl PM to components of active DAIs

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 549 bytes --]

On Wed, Oct 30, 2013 at 09:47:30AM +0800, Nicolin Chen wrote:
> On Tue, Oct 29, 2013 at 09:19:59AM -0700, Mark Brown wrote:

> > There's no reason CODECs can't implement pinctrl if they want to.

> Hmm..Sorry, I'm little confused. What should I do? Adding a pinctrl_pm
> for codec_dai->pins? Or a condition like "if (!codec_dai->active)"?

The former; I'd expect the calls to be done for the CODEC and the CPU
(you'd need the active check for the CODEC as well I guess though almost
all the time the CPU and CODEC active states should be the same).

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]