[PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Vinod Koul]

We need card to be early suspended and late resumed, so use
prepare and complete for card suspend and resume.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Tested-by: Harsha Priya <harshapriya.n@intel.com>
---
 sound/soc/intel/boards/bxt_da7219_max98357a.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/bxt_da7219_max98357a.c b/sound/soc/intel/boards/bxt_da7219_max98357a.c
index 3774b117d365..df5f269a9b67 100644
--- a/sound/soc/intel/boards/bxt_da7219_max98357a.c
+++ b/sound/soc/intel/boards/bxt_da7219_max98357a.c
@@ -441,11 +441,30 @@ static int broxton_audio_probe(struct platform_device *pdev)
 	return devm_snd_soc_register_card(&pdev->dev, &broxton_audio_card);
 }
 
+#ifdef CONFIG_PM_SLEEP
+
+static void broxton_complete(struct device *dev)
+{
+	snd_soc_resume(dev);
+}
+
+#else
+#define broxton_complete NULL
+#endif
+
+static const struct dev_pm_ops broxton_pm_ops = {
+	.prepare = snd_soc_suspend,
+	.complete = broxton_complete,
+	.freeze = snd_soc_suspend,
+	.thaw = snd_soc_resume,
+	.poweroff = snd_soc_poweroff,
+	.restore = snd_soc_resume,
+};
 static struct platform_driver broxton_audio = {
 	.probe = broxton_audio_probe,
 	.driver = {
 		.name = "bxt_da7219_max98357a_i2s",
-		.pm = &snd_soc_pm_ops,
+		.pm = &broxton_pm_ops,
 	},
 };
 module_platform_driver(broxton_audio)
-- 
1.9.1

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Mark Brown]

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

On Fri, Jun 17, 2016 at 10:03:30AM +0530, Vinod Koul wrote:

> +static void broxton_complete(struct device *dev)
> +{
> +	snd_soc_resume(dev);
> +}
> +
> +#else
> +#define broxton_complete NULL
> +#endif

Just use snd_soc_resume() directly.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Lars-Peter Clausen]

On 06/17/2016 06:33 AM, Vinod Koul wrote:
> We need card to be early suspended and late resumed, so use
> prepare and complete for card suspend and resume.

Why and why is this something that other cards do not need to do?

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Vinod Koul]

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

On Fri, Jun 17, 2016 at 12:14:07PM +0100, Mark Brown wrote:
> On Fri, Jun 17, 2016 at 10:03:30AM +0530, Vinod Koul wrote:
> 
> > +static void broxton_complete(struct device *dev)
> > +{
> > +	snd_soc_resume(dev);
> > +}
> > +
> > +#else
> > +#define broxton_complete NULL
> > +#endif
> 
> Just use snd_soc_resume() directly.

Nope, that leads to warnings.

.complete is the only callback that expects void return whereas
snd_soc_resume like other returns int.

struct dev_pm_ops {
        int (*prepare)(struct device *dev);
        void (*complete)(struct device *dev);
        int (*suspend)(struct device *dev);
        int (*resume)(struct device *dev);
	...
}

Thanks
-- 
~Vinod

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

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

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Vinod Koul]

On Fri, Jun 17, 2016 at 01:18:10PM +0200, Lars-Peter Clausen wrote:
> On 06/17/2016 06:33 AM, Vinod Koul wrote:
> > We need card to be early suspended and late resumed, so use
> > prepare and complete for card suspend and resume.
> 
> Why and why is this something that other cards do not need to do?

When card suspends, the DAPM suspend closes the widgets, which translates to
we sending IPC to DSP for tearing down the pipelines.

So we need the platform to be suspended last and resume first. This way the
snd_soc_suspend will tear down pipelines and snd_soc_resume restore those
back.

Thanks
-- 
~Vinod

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Mark Brown]

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

On Fri, Jun 17, 2016 at 05:45:48PM +0530, Vinod Koul wrote:
> On Fri, Jun 17, 2016 at 01:18:10PM +0200, Lars-Peter Clausen wrote:

> > Why and why is this something that other cards do not need to do?

> When card suspends, the DAPM suspend closes the widgets, which translates to
> we sending IPC to DSP for tearing down the pipelines.

> So we need the platform to be suspended last and resume first. This way the
> snd_soc_suspend will tear down pipelines and snd_soc_resume restore those
> back.

That doesn't answer the question - this applies to any control
mechanism.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Mark Brown]

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

On Fri, Jun 17, 2016 at 05:44:00PM +0530, Vinod Koul wrote:
> On Fri, Jun 17, 2016 at 12:14:07PM +0100, Mark Brown wrote:

> > Just use snd_soc_resume() directly.

> Nope, that leads to warnings.

> .complete is the only callback that expects void return whereas
> snd_soc_resume like other returns int.

This is possibly an indication that you're abusing things then - this is
a very unusual interface to use as well...  the patch just looks very
fishy as is, and as we were talking about in the other thread Lars
started it seems like this is bodging around something rather than
solving the right problem.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Vinod Koul]

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

On Fri, Jun 17, 2016 at 01:31:56PM +0100, Mark Brown wrote:
> On Fri, Jun 17, 2016 at 05:45:48PM +0530, Vinod Koul wrote:
> > On Fri, Jun 17, 2016 at 01:18:10PM +0200, Lars-Peter Clausen wrote:
> 
> > > Why and why is this something that other cards do not need to do?

Other systems do not have a DSP sitting and need to redownload code which
takes time and results in card being resumed even when the platform is not
ready.

The logs are indicating the snd_soc_resume() is triggered even before the
platform resume has returned which needs to be avoided.

One of the ways to ensure a dependency for PM is resolved, we tinker with PM
callbacks here to ensure the platform is ready before resume is inoked here

> > When card suspends, the DAPM suspend closes the widgets, which translates to
> > we sending IPC to DSP for tearing down the pipelines.
> 
> > So we need the platform to be suspended last and resume first. This way the
> > snd_soc_suspend will tear down pipelines and snd_soc_resume restore those
> > back.
> 
> That doesn't answer the question - this applies to any control
> mechanism.

Thanks
-- 
~Vinod

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

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

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Vinod Koul]

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

On Fri, Jun 17, 2016 at 01:35:23PM +0100, Mark Brown wrote:
> On Fri, Jun 17, 2016 at 05:44:00PM +0530, Vinod Koul wrote:
> > On Fri, Jun 17, 2016 at 12:14:07PM +0100, Mark Brown wrote:
> 
> > > Just use snd_soc_resume() directly.
> 
> > Nope, that leads to warnings.
> 
> > .complete is the only callback that expects void return whereas
> > snd_soc_resume like other returns int.
> 
> This is possibly an indication that you're abusing things then - this is
> a very unusual interface to use as well...  the patch just looks very
> fishy as is, and as we were talking about in the other thread Lars
> started it seems like this is bodging around something rather than
> solving the right problem.

In the hindsight, I do agree that instead of this, we should move platform
to do suspend_late and resume_early. That way the dependency between card
and platform is taken care in a much bteer fashion.

The problem is two components racing and need of the resume being in serial
fashion.

Thanks
-- 
~Vinod

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

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

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Mark Brown]

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

On Mon, Jun 20, 2016 at 05:14:29PM +0530, Vinod Koul wrote:
> On Fri, Jun 17, 2016 at 01:31:56PM +0100, Mark Brown wrote:
> > On Fri, Jun 17, 2016 at 05:45:48PM +0530, Vinod Koul wrote:
> > > On Fri, Jun 17, 2016 at 01:18:10PM +0200, Lars-Peter Clausen wrote:

> > > > Why and why is this something that other cards do not need to do?

> Other systems do not have a DSP sitting and need to redownload code which
> takes time and results in card being resumed even when the platform is not
> ready.

So what you're actually trying to do here is serialize the resume - this
isn't just a thing with DSPs, it's also a thing with I2C, regulators and
so on.  A quick glance suggests that disabling async suspend might help
here.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

Re: [PATCH] ASoC: Intel: Add support for PM ops in bxt-da7219_max98357a

[Vinod Koul]

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

On Mon, Jun 20, 2016 at 02:26:40PM +0100, Mark Brown wrote:
> On Mon, Jun 20, 2016 at 05:14:29PM +0530, Vinod Koul wrote:
> > On Fri, Jun 17, 2016 at 01:31:56PM +0100, Mark Brown wrote:
> > > On Fri, Jun 17, 2016 at 05:45:48PM +0530, Vinod Koul wrote:
> > > > On Fri, Jun 17, 2016 at 01:18:10PM +0200, Lars-Peter Clausen wrote:
> 
> > > > > Why and why is this something that other cards do not need to do?
> 
> > Other systems do not have a DSP sitting and need to redownload code which
> > takes time and results in card being resumed even when the platform is not
> > ready.
> 
> So what you're actually trying to do here is serialize the resume - this
> isn't just a thing with DSPs, it's also a thing with I2C, regulators and
> so on.  A quick glance suggests that disabling async suspend might help
> here.

Okay let me check that

Thanks
-- 
~Vinod

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

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