ASoC: core: init delayed_work for codec-codec links
PATCH 1/1 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().
--- sound/soc/soc-core.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
sound/soc/soc-core.c +6 -0
@@ -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;
Signature · 1 line
1.7.2.5
Mark Brown
maintainerQuoted 2 lines · Richard Fitzgerald
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?
Quoted 7 lines · Mark Brown
Quoted 2 lines · Richard Fitzgerald
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?
Mark Brown
maintainerQuoted 7 lines · Richard Fitzgerald
Quoted 2 lines · Mark Brown
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.
Quoted 10 lines · Mark Brown
Quoted 7 lines · Richard Fitzgerald
Quoted 2 lines · Mark Brown
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.
Mark Brown
maintainerQuoted 5 lines · Richard Fitzgerald
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 aSND_SOC_DAPM_STREAM_STOPbut for c2c links we never send a start anyway.
That makes sense, yes.
PATCH v2 1/1 ASoC: core: init delayed_work for codec-codec links
In reply to Richard FitzgeraldWe 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.
--- sound/soc/soc-core.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
sound/soc/soc-core.c +11 -0
@@ -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;
Signature · 1 line
1.7.2.5
Mark Brown
maintainersound/soc/soc-core.c static ssize_t pmdown_time_set(struct device *dev, · line 208
Quoted 7 lines · Richard Fitzgerald
sound/soc/soc-core.c +7
-0+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.
--- sound/soc/soc-core.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
sound/soc/soc-core.c +7 -0
@@ -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;
Signature · 1 line
1.7.2.5
Mark Brown
maintainersound/soc/soc-core.c static int soc_ac97_dev_register(struct snd_soc_codec *codec) · line 536
Quoted 4 lines · Richard Fitzgerald
sound/soc/soc-core.c +4
-0+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.
--- sound/soc/soc-core.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
sound/soc/soc-core.c +12 -0
@@ -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;
Signature · 1 line
1.7.2.5
Quoted 3 lines · Richard Fitzgerald
We must init the
delayed_workfor codec-codec links otherwise shutting down the DAI chain will fault when callingflush_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).