[alsa-devel] [PATCH] ASoC: core: init delayed_work for codec-codec links
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;
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?
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?
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.
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.
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.
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;
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).
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;
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.
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;
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).
participants (2)
-
Mark Brown -
Richard Fitzgerald