diff mbox series

[RFC,05/15] ASoC: rt5677: Auto enable/disable DSP for hotwording

Message ID 20190906194636.217881-6-cujomalainey@chromium.org (mailing list archive)
State New, archived
Headers show
Series Add Samus Hotwording for RT5677 | expand

Commit Message

Curtis Malainey Sept. 6, 2019, 7:46 p.m. UTC 
From: Ben Zhang <benzh@chromium.org>

The kcontrol 'DSP VAD Switch' is automatically enabled/disabled
when the hotwording PCM stream is opened/closed.

A function pointer in struct rt5677_priv is used to avoid module
dependency cycle between snd_soc_rt5677 and snd_soc_rt5677_spi.

Signed-off-by: Ben Zhang <benzh@chromium.org>
Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
---
 sound/soc/codecs/rt5677-spi.c | 12 ++++++++++++
 sound/soc/codecs/rt5677.c     |  6 +++++-
 sound/soc/codecs/rt5677.h     |  2 ++
 3 files changed, 19 insertions(+), 1 deletion(-)

Comments

Mark Brown Sept. 11, 2019, 10:25 a.m. UTC | #1 
On Fri, Sep 06, 2019 at 12:46:27PM -0700, Curtis Malainey wrote:
> From: Ben Zhang <benzh@chromium.org>
> 
> The kcontrol 'DSP VAD Switch' is automatically enabled/disabled
> when the hotwording PCM stream is opened/closed.

So why do we have the switch?
Curtis Malainey Sept. 11, 2019, 8:22 p.m. UTC | #2 
On Wed, Sep 11, 2019 at 3:25 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Sep 06, 2019 at 12:46:27PM -0700, Curtis Malainey wrote:
> > From: Ben Zhang <benzh@chromium.org>
> >
> > The kcontrol 'DSP VAD Switch' is automatically enabled/disabled
> > when the hotwording PCM stream is opened/closed.
>
> So why do we have the switch?

The source of the switch is commit af48f1d08a547 ("ASoC: rt5677:
Support DSP function for VAD application") and does not explain the
original intent of the switch. I believe the original intent of this
commit is to keep the switch in sync with the VAD state. I do not
believe we use the switch ourselves.
Mark Brown Sept. 12, 2019, 9:26 a.m. UTC | #3 
On Wed, Sep 11, 2019 at 01:22:20PM -0700, Curtis Malainey wrote:

> The source of the switch is commit af48f1d08a547 ("ASoC: rt5677:
> Support DSP function for VAD application") and does not explain the
> original intent of the switch. I believe the original intent of this
> commit is to keep the switch in sync with the VAD state. I do not
> believe we use the switch ourselves.

Well, I would assume that the control is used to allow users to
enable and disable the VAD functionality at runtime.  As with the
routing if it's been exposed to users we should continue to let
them control it.
Curtis Malainey Sept. 16, 2019, 9:29 p.m. UTC | #4 
On Thu, Sep 12, 2019 at 2:26 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Sep 11, 2019 at 01:22:20PM -0700, Curtis Malainey wrote:
>
> > The source of the switch is commit af48f1d08a547 ("ASoC: rt5677:
> > Support DSP function for VAD application") and does not explain the
> > original intent of the switch. I believe the original intent of this
> > commit is to keep the switch in sync with the VAD state. I do not
> > believe we use the switch ourselves.
>
> Well, I would assume that the control is used to allow users to
> enable and disable the VAD functionality at runtime.  As with the
> routing if it's been exposed to users we should continue to let
> them control it.

I will work to add variable inputs, in the samus use case it doesn't
make much sense to use the hotword without the pcm open since that
audio needs to be captured. How would userspace received the detection
without the pcm open?
Mark Brown Sept. 16, 2019, 9:55 p.m. UTC | #5 
On Mon, Sep 16, 2019 at 02:29:32PM -0700, Curtis Malainey wrote:

> I will work to add variable inputs, in the samus use case it doesn't
> make much sense to use the hotword without the pcm open since that
> audio needs to be captured. How would userspace received the detection
> without the pcm open?

They broke it, they get to keep all the pieces.  Equally, if they have a
functional use case that differs to your hard coded one they aren't
prevented from using it.
diff mbox series

Patch

diff --git a/sound/soc/codecs/rt5677-spi.c b/sound/soc/codecs/rt5677-spi.c
index 62621fe4747c..25d75a803cb5 100644
--- a/sound/soc/codecs/rt5677-spi.c
+++ b/sound/soc/codecs/rt5677-spi.c
@@ -26,6 +26,7 @@ 
 
 #include <sound/soc.h>
 
+#include "rt5677.h"
 #include "rt5677-spi.h"
 
 #define DRV_NAME "rt5677spi"
@@ -100,6 +101,12 @@  static struct snd_soc_dai_driver rt5677_spi_dai = {
 /* PCM for streaming audio from the DSP buffer */
 static int rt5677_spi_pcm_open(struct snd_pcm_substream *substream)
 {
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_component *component =
+			snd_soc_rtdcom_lookup(rtd, "rt5677");
+	struct rt5677_priv *rt5677 = snd_soc_component_get_drvdata(component);
+
+	rt5677->set_dsp_vad(component, true);
 	snd_soc_set_runtime_hwparams(substream, &rt5677_spi_pcm_hardware);
 	return 0;
 }
@@ -109,10 +116,15 @@  static int rt5677_spi_pcm_close(struct snd_pcm_substream *substream)
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_component *component =
 			snd_soc_rtdcom_lookup(rtd, DRV_NAME);
+	struct snd_soc_component *codec_component =
+			snd_soc_rtdcom_lookup(rtd, "rt5677");
+	struct rt5677_priv *rt5677 =
+			snd_soc_component_get_drvdata(codec_component);
 	struct rt5677_dsp *rt5677_dsp =
 			snd_soc_component_get_drvdata(component);
 
 	cancel_delayed_work_sync(&rt5677_dsp->copy_work);
+	rt5677->set_dsp_vad(codec_component, false);
 	return 0;
 }
 
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 35d4ec1b7dfd..9cdfe7d488fe 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -918,7 +918,9 @@  static int rt5677_set_dsp_vad(struct snd_soc_component *component, bool on)
 	if (!IS_ENABLED(CONFIG_SND_SOC_RT5677_SPI))
 		return -ENXIO;
 
+	rt5677->dsp_vad_en = on;
 	dev_info(component->dev, "DSP VAD: on=%d, activity=%d\n", on, activity);
+
 	if (on && !activity) {
 		activity = true;
 
@@ -1005,7 +1007,8 @@  static int rt5677_dsp_vad_put(struct snd_kcontrol *kcontrol,
 	rt5677->dsp_vad_en = !!ucontrol->value.integer.value[0];
 
 	if (snd_soc_component_get_bias_level(component) == SND_SOC_BIAS_OFF)
-		rt5677_set_dsp_vad(component, rt5677->dsp_vad_en);
+		rt5677_set_dsp_vad(component,
+				!!ucontrol->value.integer.value[0]);
 
 	return 0;
 }
@@ -5471,6 +5474,7 @@  static int rt5677_i2c_probe(struct i2c_client *i2c)
 		return -ENOMEM;
 
 	rt5677->dev = &i2c->dev;
+	rt5677->set_dsp_vad = rt5677_set_dsp_vad;
 	i2c_set_clientdata(i2c, rt5677);
 
 	if (i2c->dev.of_node) {
diff --git a/sound/soc/codecs/rt5677.h b/sound/soc/codecs/rt5677.h
index 2bbd618b51ac..ec5be7e01fd1 100644
--- a/sound/soc/codecs/rt5677.h
+++ b/sound/soc/codecs/rt5677.h
@@ -1854,6 +1854,8 @@  struct rt5677_priv {
 	struct irq_domain *domain;
 	struct mutex irq_lock;
 	unsigned int irq_en;
+
+	int (*set_dsp_vad)(struct snd_soc_component *component, bool on);
 };
 
 int rt5677_sel_asrc_clk_src(struct snd_soc_component *component,