[PATCH] ASoC: core: init delayed_work for codec-codec links

[PATCH] ASoC: core: init delayed_work for codec-codec links

[Richard Fitzgerald]

If soc_probe_link_dais() finds a codec-codec link it
skips creating a compress or pcm stream and links the
DAIs together. But it must also init the delayed_work
otherwise shutting down the DAI chain will fault when
calling flush_delayed_work_sync() on the linked DAI.

Pointing it to a dummy work callback is cleaner than taking
special cases in the code to bypass the flush_delayed_work_sync().

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4489c5b..bbe136c 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -199,6 +199,10 @@ static ssize_t pmdown_time_set(struct device *dev,
 	return count;
 }
 
+static void dummy_delayed_work(struct work_struct *work)
+{
+}
+
 static DEVICE_ATTR(pmdown_time, 0644, pmdown_time_show, pmdown_time_set);
 
 #ifdef CONFIG_DEBUG_FS
@@ -1428,6 +1432,8 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
 				return ret;
 			}
 		} else {
+			INIT_DELAYED_WORK(&rtd->delayed_work, dummy_delayed_work);
+
 			/* link the DAI widgets */
 			play_w = codec_dai->playback_widget;
 			capture_w = cpu_dai->capture_widget;
-- 
1.7.2.5

Re: [PATCH] ASoC: core: init delayed_work for codec-codec links

[Mark Brown]

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

On Wed, Jul 31, 2013 at 02:16:44PM +0100, Richard Fitzgerald wrote:

> Pointing it to a dummy work callback is cleaner than taking
> special cases in the code to bypass the flush_delayed_work_sync().

Why is this better than pointing at the normal work that you'd expect to
be used there?

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

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

Re: [PATCH] ASoC: core: init delayed_work for codec-codec links

[Richard Fitzgerald]

On Wed, Jul 31, 2013 at 02:25:22PM +0100, Mark Brown wrote:
> On Wed, Jul 31, 2013 at 02:16:44PM +0100, Richard Fitzgerald wrote:
> 
> > Pointing it to a dummy work callback is cleaner than taking
> > special cases in the code to bypass the flush_delayed_work_sync().
> 
> Why is this better than pointing at the normal work that you'd expect to
> be used there?

By 'normal work' do you mean the close_delayed_work() used for
standard PCM DAIs?

Re: [PATCH] ASoC: core: init delayed_work for codec-codec links

[Mark Brown]

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

On Thu, Aug 01, 2013 at 04:13:52PM +0100, Richard Fitzgerald wrote:
> On Wed, Jul 31, 2013 at 02:25:22PM +0100, Mark Brown wrote:

> > Why is this better than pointing at the normal work that you'd expect to
> > be used there?

> By 'normal work' do you mean the close_delayed_work() used for
> standard PCM DAIs?

Yes - making up this empty work doesn't seem like a robust approach.

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

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

Re: [PATCH] ASoC: core: init delayed_work for codec-codec links

[Richard Fitzgerald]

On Thu, Aug 01, 2013 at 04:23:56PM +0100, Mark Brown wrote:
> On Thu, Aug 01, 2013 at 04:13:52PM +0100, Richard Fitzgerald wrote:
> > On Wed, Jul 31, 2013 at 02:25:22PM +0100, Mark Brown wrote:
> 
> > > Why is this better than pointing at the normal work that you'd expect to
> > > be used there?
> 
> > By 'normal work' do you mean the close_delayed_work() used for
> > standard PCM DAIs?
> 
> Yes - making up this empty work doesn't seem like a robust approach.

Yeah, I think the problem is more one of bad naming. I should define that
function as the delayed work for codec-2-codec links, it just happens
that we don't currently have to do anything for them. The only thing
the pcm close_delayed_work() does is to send a SND_SOC_DAPM_STREAM_STOP
but for c2c links we never send a start anyway.

Re: [PATCH] ASoC: core: init delayed_work for codec-codec links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On Thu, Aug 01, 2013 at 04:50:56PM +0100, Richard Fitzgerald wrote:

> Yeah, I think the problem is more one of bad naming. I should define that
> function as the delayed work for codec-2-codec links, it just happens
> that we don't currently have to do anything for them. The only thing
> the pcm close_delayed_work() does is to send a SND_SOC_DAPM_STREAM_STOP
> but for c2c links we never send a start anyway.

That makes sense, yes.

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

[PATCH v2] ASoC: core: init delayed_work for codec-codec links

[Richard Fitzgerald]

We must init the delayed_work for codec-codec links
otherwise shutting down the DAI chain will fault when
calling flush_delayed_work_sync() on the linked DAI.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4489c5b..e649efe 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -199,6 +199,14 @@ static ssize_t pmdown_time_set(struct device *dev,
 	return count;
 }
 
+static void codec2codec_close_delayed_work(struct work_struct *work)
+{
+	/* No action required.
+	 * C2C links only represent a hardware connection and currently
+	 * we assume that we don't have to explicitly start or stop them
+	 */
+}
+
 static DEVICE_ATTR(pmdown_time, 0644, pmdown_time_show, pmdown_time_set);
 
 #ifdef CONFIG_DEBUG_FS
@@ -1428,6 +1436,9 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
 				return ret;
 			}
 		} else {
+			INIT_DELAYED_WORK(&rtd->delayed_work,
+						codec2codec_close_delayed_work);
+
 			/* link the DAI widgets */
 			play_w = codec_dai->playback_widget;
 			capture_w = cpu_dai->capture_widget;
-- 
1.7.2.5

Re: [PATCH v2] ASoC: core: init delayed_work for codec-codec links

[Mark Brown]

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

On Fri, Aug 02, 2013 at 11:01:08AM +0100, Richard Fitzgerald wrote:

> +static void codec2codec_close_delayed_work(struct work_struct *work)
> +{
> +	/* No action required.
> +	 * C2C links only represent a hardware connection and currently
> +	 * we assume that we don't have to explicitly start or stop them
> +	 */
> +}

That's not the reason - what's happening with the stream events is that
since these links are not on the edge of the DAPM graph we don't need to
interface with the outside world.  Further since we're not dealing with
the application layer there is no need to delay the power down, the
reason that's there is to stop us powering the link down if there's just
a small gap between tracks so we don't get pops, clicks and extra gaps
if the application layer is closing the device at the end of every track
(which is quite a common pattern).

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

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

Re: [PATCH v3] ASoC: core: init delayed_work for codec-codec links

[Richard Fitzgerald]

We must init the delayed_work for codec-codec links
otherwise shutting down the DAI chain will fault when
calling flush_delayed_work_sync() on the linked DAI.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4489c5b..450dbf4 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -530,6 +530,10 @@ static int soc_ac97_dev_register(struct snd_soc_codec *codec)
 }
 #endif
 
+static void codec2codec_close_delayed_work(struct work_struct *work)
+{
+}
+
 #ifdef CONFIG_PM_SLEEP
 /* powers down audio subsystem for suspend */
 int snd_soc_suspend(struct device *dev)
@@ -1428,6 +1432,9 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
 				return ret;
 			}
 		} else {
+			INIT_DELAYED_WORK(&rtd->delayed_work,
+						codec2codec_close_delayed_work);
+
 			/* link the DAI widgets */
 			play_w = codec_dai->playback_widget;
 			capture_w = cpu_dai->capture_widget;
-- 
1.7.2.5

Re: [alsa-devel] [PATCH v3] ASoC: core: init delayed_work for codec-codec links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

On Fri, Aug 02, 2013 at 11:51:56AM +0100, Richard Fitzgerald wrote:

> +static void codec2codec_close_delayed_work(struct work_struct *work)
> +{
> +}
> +

Having a commment here was a really good idea since this is the sort of
thing I'd expect someone to come along and try to clean up otherwise.
I'd also expect just passing NULL to INIT_DELAYED_WORK() (with a big
comment attached) to do the right thing, though perhaps it doesn't.

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

Re: [PATCH v4] ASoC: core: init delayed_work for codec-codec links

[Richard Fitzgerald]

We must init the delayed_work for codec-codec links
otherwise shutting down the DAI chain will fault when
calling flush_delayed_work_sync() on the linked DAI.

Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4489c5b..c57d5f1 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -530,6 +530,15 @@ static int soc_ac97_dev_register(struct snd_soc_codec *codec)
 }
 #endif
 
+static void codec2codec_close_delayed_work(struct work_struct *work)
+{
+	/* Currently nothing to do for c2c links
+	 * Since c2c links are internal nodes in the DAPM graph and
+	 * don't interface with the outside world or application layer
+	 * we don't have to do any special handling on close.
+	 */
+}
+
 #ifdef CONFIG_PM_SLEEP
 /* powers down audio subsystem for suspend */
 int snd_soc_suspend(struct device *dev)
@@ -1428,6 +1437,9 @@ static int soc_probe_link_dais(struct snd_soc_card *card, int num, int order)
 				return ret;
 			}
 		} else {
+			INIT_DELAYED_WORK(&rtd->delayed_work,
+						codec2codec_close_delayed_work);
+
 			/* link the DAI widgets */
 			play_w = codec_dai->playback_widget;
 			capture_w = cpu_dai->capture_widget;
-- 
1.7.2.5

Re: [PATCH v4] ASoC: core: init delayed_work for codec-codec links

[Mark Brown]

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

On Mon, Aug 05, 2013 at 01:17:28PM +0100, Richard Fitzgerald wrote:
> We must init the delayed_work for codec-codec links
> otherwise shutting down the DAI chain will fault when
> calling flush_delayed_work_sync() on the linked DAI.

Applied, thanks - please send using a normal patch subject line with no
Re (which makes things get filtered out more easily).

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

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