[PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.

Some audio devices require notification of drain events
in order to properly drain and shutdown an audio stream.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 include/sound/pcm.h     | 1 +
 sound/core/pcm_native.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 8bb00a2..bb77aa3 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -109,6 +109,7 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
 #define SNDRV_PCM_TRIGGER_SUSPEND	5
 #define SNDRV_PCM_TRIGGER_RESUME	6
+#define SNDRV_PCM_TRIGGER_DRAIN		7
 
 #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
 
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 166d59c..15fe682 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+
+		if (substream->runtime->trigger_master == substream)
+			substream->ops->trigger(substream,
+					SNDRV_PCM_TRIGGER_DRAIN);
+
 		switch (runtime->status->state) {
 		case SNDRV_PCM_STATE_PREPARED:
 			/* start playback stream if possible */
-- 
1.9.1

[PATCH 2/2] ASoC: Intel: Clean data after SST fw fetch

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

The BDW audio firmware DSP manages the DMA and the DMA cannot be
stopped exactly at the end of the playback stream. This means
stale samples may be played at PCM stop unless the driver copies
silence to the subsequent periods.

Signed-off-by: Libin Yang <libin.yang@intel.com>
Reviewed-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
---
 sound/soc/intel/sst-haswell-ipc.c | 29 ++++++++++++++++++
 sound/soc/intel/sst-haswell-ipc.h |  9 ++++++
 sound/soc/intel/sst-haswell-pcm.c | 64 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+)

diff --git a/sound/soc/intel/sst-haswell-ipc.c b/sound/soc/intel/sst-haswell-ipc.c
index 3f8c482..57d6e89 100644
--- a/sound/soc/intel/sst-haswell-ipc.c
+++ b/sound/soc/intel/sst-haswell-ipc.c
@@ -31,6 +31,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/debugfs.h>
 #include <linux/pm_runtime.h>
+#include <sound/asound.h>
 
 #include "sst-haswell-ipc.h"
 #include "sst-dsp.h"
@@ -240,6 +241,10 @@ struct sst_hsw_stream {
 	u32 (*notify_position)(struct sst_hsw_stream *stream, void *data);
 	void *pdata;
 
+	/* record the fw read position when playback */
+	snd_pcm_uframes_t old_position;
+	bool play_silence;
+
 	struct list_head node;
 };
 
@@ -1476,6 +1481,30 @@ u32 sst_hsw_stream_get_vol_reg(struct sst_hsw *hsw,
 	return stream->reply.volume_register_address[channel];
 }
 
+snd_pcm_uframes_t sst_hsw_stream_get_old_position(struct sst_hsw *hsw,
+	struct sst_hsw_stream *stream)
+{
+	return stream->old_position;
+}
+
+void sst_hsw_stream_set_old_position(struct sst_hsw *hsw,
+	struct sst_hsw_stream *stream, snd_pcm_uframes_t val)
+{
+	stream->old_position = val;
+}
+
+bool sst_hsw_stream_get_silence_start(struct sst_hsw *hsw,
+	struct sst_hsw_stream *stream)
+{
+	return stream->play_silence;
+}
+
+void sst_hsw_stream_set_silence_start(struct sst_hsw *hsw,
+	struct sst_hsw_stream *stream, bool val)
+{
+	stream->play_silence = val;
+}
+
 int sst_hsw_mixer_get_info(struct sst_hsw *hsw)
 {
 	struct sst_hsw_ipc_stream_info_reply *reply;
diff --git a/sound/soc/intel/sst-haswell-ipc.h b/sound/soc/intel/sst-haswell-ipc.h
index 138e894..5c72e49 100644
--- a/sound/soc/intel/sst-haswell-ipc.h
+++ b/sound/soc/intel/sst-haswell-ipc.h
@@ -20,6 +20,7 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
+#include <sound/asound.h>
 
 #define SST_HSW_NO_CHANNELS		4
 #define SST_HSW_MAX_DX_REGIONS		14
@@ -452,6 +453,14 @@ u32 sst_hsw_stream_get_peak_reg(struct sst_hsw *hsw,
 	struct sst_hsw_stream *stream, u32 channel);
 u32 sst_hsw_stream_get_vol_reg(struct sst_hsw *hsw,
 	struct sst_hsw_stream *stream, u32 channel);
+snd_pcm_uframes_t sst_hsw_stream_get_old_position(struct sst_hsw *hsw,
+	struct sst_hsw_stream *stream);
+void sst_hsw_stream_set_old_position(struct sst_hsw *hsw,
+	struct sst_hsw_stream *stream, snd_pcm_uframes_t val);
+bool sst_hsw_stream_get_silence_start(struct sst_hsw *hsw,
+	struct sst_hsw_stream *stream);
+void sst_hsw_stream_set_silence_start(struct sst_hsw *hsw,
+	struct sst_hsw_stream *stream, bool val);
 int sst_hsw_mixer_get_info(struct sst_hsw *hsw);
 
 /* Stream ALSA trigger operations */
diff --git a/sound/soc/intel/sst-haswell-pcm.c b/sound/soc/intel/sst-haswell-pcm.c
index 0180b38..05c855b 100644
--- a/sound/soc/intel/sst-haswell-pcm.c
+++ b/sound/soc/intel/sst-haswell-pcm.c
@@ -36,6 +36,11 @@
 #define HSW_PCM_COUNT		6
 #define HSW_VOLUME_MAX		0x7FFFFFFF	/* 0dB */
 
+#define SST_OLD_POSITION(d, r, o) ((d) +	\
+			frames_to_bytes(r, o))
+#define SST_SAMPLES(r, x) (bytes_to_samples(r,	\
+			frames_to_bytes(r, (x))))
+
 /* simple volume table */
 static const u32 volume_map[] = {
 	HSW_VOLUME_MAX >> 30,
@@ -553,19 +558,29 @@ static int hsw_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
 	struct hsw_priv_data *pdata =
 		snd_soc_platform_get_drvdata(rtd->platform);
 	struct hsw_pcm_data *pcm_data = snd_soc_pcm_get_drvdata(rtd);
+	struct sst_hsw_stream *sst_stream = pcm_data->stream;
 	struct sst_hsw *hsw = pdata->hsw;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	snd_pcm_uframes_t pos;
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		sst_hsw_stream_set_silence_start(hsw, sst_stream, false);
 		sst_hsw_stream_resume(hsw, pcm_data->stream, 0);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		sst_hsw_stream_set_silence_start(hsw, sst_stream, false);
 		sst_hsw_stream_pause(hsw, pcm_data->stream, 0);
 		break;
+	case SNDRV_PCM_TRIGGER_DRAIN:
+		pos = runtime->control->appl_ptr % runtime->buffer_size;
+		sst_hsw_stream_set_old_position(hsw, pcm_data->stream, pos);
+		sst_hsw_stream_set_silence_start(hsw, sst_stream, true);
+		break;
 	default:
 		break;
 	}
@@ -579,13 +594,62 @@ static u32 hsw_notify_pointer(struct sst_hsw_stream *stream, void *data)
 	struct snd_pcm_substream *substream = pcm_data->substream;
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct hsw_priv_data *pdata =
+		snd_soc_platform_get_drvdata(rtd->platform);
+	struct sst_hsw *hsw = pdata->hsw;
 	u32 pos;
+	snd_pcm_uframes_t position = bytes_to_frames(runtime,
+		 sst_hsw_get_dsp_position(hsw, pcm_data->stream));
+	unsigned char *dma_area = runtime->dma_area;
+	snd_pcm_uframes_t dma_frames =
+		bytes_to_frames(runtime, runtime->dma_bytes);
+	snd_pcm_uframes_t old_position;
+	ssize_t samples;
 
 	pos = frames_to_bytes(runtime,
 		(runtime->control->appl_ptr % runtime->buffer_size));
 
 	dev_vdbg(rtd->dev, "PCM: App pointer %d bytes\n", pos);
 
+	/* SST fw don't know where to stop dma
+	 * So, SST driver need to clean the data which has been consumed
+	 */
+	if (dma_area == NULL || dma_frames <= 0
+		|| (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		|| !sst_hsw_stream_get_silence_start(hsw, stream)) {
+		snd_pcm_period_elapsed(substream);
+		return pos;
+	}
+
+	old_position = sst_hsw_stream_get_old_position(hsw, stream);
+	if (position > old_position) {
+		if (position < dma_frames) {
+			samples = SST_SAMPLES(runtime, position - old_position);
+			snd_pcm_format_set_silence(runtime->format,
+				SST_OLD_POSITION(dma_area,
+					runtime, old_position),
+				samples);
+		} else
+			dev_err(rtd->dev, "PCM: position is wrong\n");
+	} else {
+		if (old_position < dma_frames) {
+			samples = SST_SAMPLES(runtime,
+				dma_frames - old_position);
+			snd_pcm_format_set_silence(runtime->format,
+				SST_OLD_POSITION(dma_area,
+					runtime, old_position),
+				samples);
+		} else
+			dev_err(rtd->dev, "PCM: dma_bytes is wrong\n");
+		if (position < dma_frames) {
+			samples = SST_SAMPLES(runtime, position);
+			snd_pcm_format_set_silence(runtime->format,
+				dma_area, samples);
+		} else
+			dev_err(rtd->dev, "PCM: position is wrong\n");
+	}
+	sst_hsw_stream_set_old_position(hsw, stream, position);
+
 	/* let alsa know we have play a period */
 	snd_pcm_period_elapsed(substream);
 	return pos;
-- 
1.9.1

Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[Takashi Iwai]

At Thu, 18 Dec 2014 15:36:03 +0800,
libin.yang@intel.com wrote:
> 
> From: Libin Yang <libin.yang@intel.com>
> 
> Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.
> 
> Some audio devices require notification of drain events
> in order to properly drain and shutdown an audio stream.
> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>  include/sound/pcm.h     | 1 +
>  sound/core/pcm_native.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 8bb00a2..bb77aa3 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -109,6 +109,7 @@ struct snd_pcm_ops {
>  #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
>  #define SNDRV_PCM_TRIGGER_SUSPEND	5
>  #define SNDRV_PCM_TRIGGER_RESUME	6
> +#define SNDRV_PCM_TRIGGER_DRAIN		7
>  
>  #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
>  
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 166d59c..15fe682 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +
> +		if (substream->runtime->trigger_master == substream)
> +			substream->ops->trigger(substream,
> +					SNDRV_PCM_TRIGGER_DRAIN);

The return value check is missing.

Besides, this patch will break many drivers.  Many of them will spew
errors if an unexpected trigger command is issued.  So, you have to
limit this trigger only to some drivers.  A typical workaround is to
add a new SNDRV_PCM_INFO_* and issue the trigger only when the flag is
set, as done for resume and pause operations.

Also, do you want to see this trigger even if the stream isn't started
before drain is issued?  Also, why no need for this trigger for
capture streams?


(BTW, while looking at this patch, I found a bug in the current
 draining code.  Will submit the fix...)


Takashi

Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[Yang, Libin]

Hi Takashi,

Thanks for review, please see my comments.


> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, December 18, 2014 4:30 PM
> To: Yang, Libin
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org;
> liam.r.girdwood@linux.intel.com; Jie, Yang
> Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger
> 
> At Thu, 18 Dec 2014 15:36:03 +0800,
> libin.yang@intel.com wrote:
> >
> > From: Libin Yang <libin.yang@intel.com>
> >
> > Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.
> >
> > Some audio devices require notification of drain events in order to
> > properly drain and shutdown an audio stream.
> >
> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > ---
> >  include/sound/pcm.h     | 1 +
> >  sound/core/pcm_native.c | 5 +++++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h index
> > 8bb00a2..bb77aa3 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -109,6 +109,7 @@ struct snd_pcm_ops {
> >  #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
> >  #define SNDRV_PCM_TRIGGER_SUSPEND	5
> >  #define SNDRV_PCM_TRIGGER_RESUME	6
> > +#define SNDRV_PCM_TRIGGER_DRAIN		7
> >
> >  #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
> >
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index
> > 166d59c..15fe682 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct
> > snd_pcm_substream *substream, int state)  {
> >  	struct snd_pcm_runtime *runtime = substream->runtime;
> >  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +
> > +		if (substream->runtime->trigger_master == substream)
> > +			substream->ops->trigger(substream,
> > +					SNDRV_PCM_TRIGGER_DRAIN);
> 
> The return value check is missing.

Actually, the code doesn't care the return value and I don't want to let the
trigger return value impact the snd_pcm_do_drain_init() flow just like
snd_pcm_do_stop(). 

> 
> Besides, this patch will break many drivers.  Many of them will spew errors if
> an unexpected trigger command is issued.  So, you have to limit this trigger
> only to some drivers.  A typical workaround is to add a new
> SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as done
> for resume and pause operations.

Yes, that's a good suggestion. Thanks for reminding me.

> 
> Also, do you want to see this trigger even if the stream isn't started before
> drain is issued?  Also, why no need for this trigger for capture streams?

In this case, the trigger action will be started. For the stream not started
case, if I skip the trigger , I don't know where I can have the chance to 
trigger later. And I didn't find the suitable place to trigger my code. 
Put the trigger at the end of the function snd_pcm_do_drain_init() or 
in the function snd_pcm_post_drain_init() will be OK? 

I didn't include the capture case because we found the capture currently 
don't need the trigger.

Our issue is the fw don't know when to stop fetching the data. For playback
we need fill silence data when draining, otherwise the fw will get the old data.
But for capture, after the driver has fetched the data, it will stopped and no
old data will be fetched.

> 
> 
> (BTW, while looking at this patch, I found a bug in the current  draining code.
> Will submit the fix...)
> 
> 
> Takashi


Regards,
Libin

Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[Takashi Iwai]

At Fri, 19 Dec 2014 06:29:33 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> Thanks for review, please see my comments.
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Thursday, December 18, 2014 4:30 PM
> > To: Yang, Libin
> > Cc: broonie@kernel.org; alsa-devel@alsa-project.org;
> > liam.r.girdwood@linux.intel.com; Jie, Yang
> > Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger
> > 
> > At Thu, 18 Dec 2014 15:36:03 +0800,
> > libin.yang@intel.com wrote:
> > >
> > > From: Libin Yang <libin.yang@intel.com>
> > >
> > > Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.
> > >
> > > Some audio devices require notification of drain events in order to
> > > properly drain and shutdown an audio stream.
> > >
> > > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > > ---
> > >  include/sound/pcm.h     | 1 +
> > >  sound/core/pcm_native.c | 5 +++++
> > >  2 files changed, 6 insertions(+)
> > >
> > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h index
> > > 8bb00a2..bb77aa3 100644
> > > --- a/include/sound/pcm.h
> > > +++ b/include/sound/pcm.h
> > > @@ -109,6 +109,7 @@ struct snd_pcm_ops {
> > >  #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
> > >  #define SNDRV_PCM_TRIGGER_SUSPEND	5
> > >  #define SNDRV_PCM_TRIGGER_RESUME	6
> > > +#define SNDRV_PCM_TRIGGER_DRAIN		7
> > >
> > >  #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
> > >
> > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index
> > > 166d59c..15fe682 100644
> > > --- a/sound/core/pcm_native.c
> > > +++ b/sound/core/pcm_native.c
> > > @@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct
> > > snd_pcm_substream *substream, int state)  {
> > >  	struct snd_pcm_runtime *runtime = substream->runtime;
> > >  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > > +
> > > +		if (substream->runtime->trigger_master == substream)
> > > +			substream->ops->trigger(substream,
> > > +					SNDRV_PCM_TRIGGER_DRAIN);
> > 
> > The return value check is missing.
> 
> Actually, the code doesn't care the return value and I don't want to let the
> trigger return value impact the snd_pcm_do_drain_init() flow just like
> snd_pcm_do_stop(). 

Well, the fundamental problem in this patch is that it was written
*just for your hardware* although this is a public API extension.
Consider again about it: if the return value of this trigger doesn't
matter for your hardware, the driver can simply return zero always.
Why do you need to drop the check in the common code?  Wouldn't any
driver return any error there?


> > Besides, this patch will break many drivers.  Many of them will spew errors if
> > an unexpected trigger command is issued.  So, you have to limit this trigger
> > only to some drivers.  A typical workaround is to add a new
> > SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as done
> > for resume and pause operations.
> 
> Yes, that's a good suggestion. Thanks for reminding me.

OTOH, the info flag will be exposed to user-space while this is a
kernel-space-only information.  So, in the code copying the info flag
(snd_pcm_hw_refine()) would need to drop this bit, too.

Alternatively, we may add just a new flag in struct snd_pcm or struct
snd_pcm_substream and let the driver setting it manually.  This might
need more changes in ASoC side, though, as the PCM creation isn't
directly done by the driver.


> > Also, do you want to see this trigger even if the stream isn't started before
> > drain is issued?  Also, why no need for this trigger for capture streams?
> 
> In this case, the trigger action will be started. For the stream not started
> case, if I skip the trigger , I don't know where I can have the chance to 
> trigger later. And I didn't find the suitable place to trigger my code. 
> Put the trigger at the end of the function snd_pcm_do_drain_init() or 
> in the function snd_pcm_post_drain_init() will be OK? 
> 
> I didn't include the capture case because we found the capture currently 
> don't need the trigger.
> 
> Our issue is the fw don't know when to stop fetching the data. For playback
> we need fill silence data when draining, otherwise the fw will get the old data.
> But for capture, after the driver has fetched the data, it will stopped and no
> old data will be fetched.

A similar problem here.  If you extend the API, don't build up only
from the bottom.  Rather define from a higher POV.

If calling the drain trigger for capture stream doesn't make *any*
sense for all hardware, then we shouldn't add it.  This would be the
only reason not to do it.  What about empty streams...?

That being said, we need to define the proper abstraction, what this
trigger does for which purpose, not specific to a certain hardware.
That is, the requirement goes up from your hardware level while the
design comes down from the abstraction level.

So, in general I found it good to have this kind of API extension.
But let's add it a bit more carefully.


thanks,

Takashi

Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[Yang, Libin]

Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Friday, December 19, 2014 4:48 PM
> To: Yang, Libin
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org;
> liam.r.girdwood@linux.intel.com; Jie, Yang
> Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger
> 
> At Fri, 19 Dec 2014 06:29:33 +0000,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > Thanks for review, please see my comments.
> >
> >
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:tiwai@suse.de]
> > > Sent: Thursday, December 18, 2014 4:30 PM
> > > To: Yang, Libin
> > > Cc: broonie@kernel.org; alsa-devel@alsa-project.org;
> > > liam.r.girdwood@linux.intel.com; Jie, Yang
> > > Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN
> trigger
> > >
> > > At Thu, 18 Dec 2014 15:36:03 +0800,
> > > libin.yang@intel.com wrote:
> > > >
> > > > From: Libin Yang <libin.yang@intel.com>
> > > >
> > > > Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.
> > > >
> > > > Some audio devices require notification of drain events in order
> > > > to properly drain and shutdown an audio stream.
> > > >
> > > > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > > > ---
> > > >  include/sound/pcm.h     | 1 +
> > > >  sound/core/pcm_native.c | 5 +++++
> > > >  2 files changed, 6 insertions(+)
> > > >
> > > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h index
> > > > 8bb00a2..bb77aa3 100644
> > > > --- a/include/sound/pcm.h
> > > > +++ b/include/sound/pcm.h
> > > > @@ -109,6 +109,7 @@ struct snd_pcm_ops {
> > > >  #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
> > > >  #define SNDRV_PCM_TRIGGER_SUSPEND	5
> > > >  #define SNDRV_PCM_TRIGGER_RESUME	6
> > > > +#define SNDRV_PCM_TRIGGER_DRAIN		7
> > > >
> > > >  #define SNDRV_PCM_POS_XRUN
> 	((snd_pcm_uframes_t)-1)
> > > >
> > > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > > > index
> > > > 166d59c..15fe682 100644
> > > > --- a/sound/core/pcm_native.c
> > > > +++ b/sound/core/pcm_native.c
> > > > @@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct
> > > > snd_pcm_substream *substream, int state)  {
> > > >  	struct snd_pcm_runtime *runtime = substream->runtime;
> > > >  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > > > +
> > > > +		if (substream->runtime->trigger_master == substream)
> > > > +			substream->ops->trigger(substream,
> > > > +					SNDRV_PCM_TRIGGER_DRAIN);
> > >
> > > The return value check is missing.
> >
> > Actually, the code doesn't care the return value and I don't want to
> > let the trigger return value impact the snd_pcm_do_drain_init() flow
> > just like snd_pcm_do_stop().
> 
> Well, the fundamental problem in this patch is that it was written *just for
> your hardware* although this is a public API extension.
> Consider again about it: if the return value of this trigger doesn't matter for
> your hardware, the driver can simply return zero always.
> Why do you need to drop the check in the common code?  Wouldn't any
> driver return any error there?

Right, I will add the return value check.

> 
> 
> > > Besides, this patch will break many drivers.  Many of them will spew
> > > errors if an unexpected trigger command is issued.  So, you have to
> > > limit this trigger only to some drivers.  A typical workaround is to
> > > add a new
> > > SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as
> > > done for resume and pause operations.
> >
> > Yes, that's a good suggestion. Thanks for reminding me.
> 
> OTOH, the info flag will be exposed to user-space while this is a kernel-space-
> only information.  So, in the code copying the info flag
> (snd_pcm_hw_refine()) would need to drop this bit, too.

Thanks, I will drop this bit in snd_pcm_hw_refine().

> 
> Alternatively, we may add just a new flag in struct snd_pcm or struct
> snd_pcm_substream and let the driver setting it manually.  This might need
> more changes in ASoC side, though, as the PCM creation isn't directly done
> by the driver.

Yes, it is a little complex for asoc if using this method. 
Not sure if it is OK we set the flag in snd_soc_ops->startup() each time 
when the function is called.

What do you think we just use the first method? 

> 
> 
> > > Also, do you want to see this trigger even if the stream isn't
> > > started before drain is issued?  Also, why no need for this trigger for
> capture streams?
> >
> > In this case, the trigger action will be started. For the stream not
> > started case, if I skip the trigger , I don't know where I can have
> > the chance to trigger later. And I didn't find the suitable place to trigger my
> code.
> > Put the trigger at the end of the function snd_pcm_do_drain_init() or
> > in the function snd_pcm_post_drain_init() will be OK?
> >
> > I didn't include the capture case because we found the capture
> > currently don't need the trigger.
> >
> > Our issue is the fw don't know when to stop fetching the data. For
> > playback we need fill silence data when draining, otherwise the fw will get
> the old data.
> > But for capture, after the driver has fetched the data, it will
> > stopped and no old data will be fetched.
> 
> A similar problem here.  If you extend the API, don't build up only from the
> bottom.  Rather define from a higher POV.
> 
> If calling the drain trigger for capture stream doesn't make *any* sense for all
> hardware, then we shouldn't add it.  This would be the only reason not to do
> it.  What about empty streams...?
> 
> That being said, we need to define the proper abstraction, what this trigger
> does for which purpose, not specific to a certain hardware.
> That is, the requirement goes up from your hardware level while the design
> comes down from the abstraction level.

Right. I will include the capture.

My patch will be like:

@@ -1566,6 +1567,11 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 			snd_pcm_post_stop(substream, new_state);
 		}
 	}
+
+	if (substream->runtime->trigger_master == substream &&
+		(substream->runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))
+		return substream->ops->trigger(substream,
+				SNDRV_PCM_TRIGGER_DRAIN);
+
 	return 0;
 }

I moved the trigger at the end of the function. Or do you think we need trigger 
before snd_pcm_do_stop() for capture case based on your knowledge?

And in the snd_pcm_hw_refine():

@@ -420,7 +420,8 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 
 	hw = &substream->runtime->hw;
 	if (!params->info) {
-		params->info = hw->info & ~SNDRV_PCM_INFO_FIFO_IN_FRAMES;
+		params->info = hw->info & ~SNDRV_PCM_INFO_FIFO_IN_FRAMES
+			& ~SNDRV_PCM_INFO_DRAIN_TRIGGER;
 		if (!hw_support_mmap(substream))
 			params->info &= ~(SNDRV_PCM_INFO_MMAP |
 					  SNDRV_PCM_INFO_MMAP_VALID);

Do you think whether it is OK?

> 
> So, in general I found it good to have this kind of API extension.
> But let's add it a bit more carefully.
> 
> 
> thanks,
> 
> Takashi

Regards,
Libin

Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[Takashi Iwai]

At Mon, 22 Dec 2014 07:42:38 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> > > > Besides, this patch will break many drivers.  Many of them will spew
> > > > errors if an unexpected trigger command is issued.  So, you have to
> > > > limit this trigger only to some drivers.  A typical workaround is to
> > > > add a new
> > > > SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as
> > > > done for resume and pause operations.
> > >
> > > Yes, that's a good suggestion. Thanks for reminding me.
> > 
> > OTOH, the info flag will be exposed to user-space while this is a kernel-space-
> > only information.  So, in the code copying the info flag
> > (snd_pcm_hw_refine()) would need to drop this bit, too.
> 
> Thanks, I will drop this bit in snd_pcm_hw_refine().
> 
> > 
> > Alternatively, we may add just a new flag in struct snd_pcm or struct
> > snd_pcm_substream and let the driver setting it manually.  This might need
> > more changes in ASoC side, though, as the PCM creation isn't directly done
> > by the driver.
> 
> Yes, it is a little complex for asoc if using this method. 
> Not sure if it is OK we set the flag in snd_soc_ops->startup() each time 
> when the function is called.
> 
> What do you think we just use the first method? 
> 
> > 
> > 
> > > > Also, do you want to see this trigger even if the stream isn't
> > > > started before drain is issued?  Also, why no need for this trigger for
> > capture streams?
> > >
> > > In this case, the trigger action will be started. For the stream not
> > > started case, if I skip the trigger , I don't know where I can have
> > > the chance to trigger later. And I didn't find the suitable place to trigger my
> > code.
> > > Put the trigger at the end of the function snd_pcm_do_drain_init() or
> > > in the function snd_pcm_post_drain_init() will be OK?
> > >
> > > I didn't include the capture case because we found the capture
> > > currently don't need the trigger.
> > >
> > > Our issue is the fw don't know when to stop fetching the data. For
> > > playback we need fill silence data when draining, otherwise the fw will get
> > the old data.
> > > But for capture, after the driver has fetched the data, it will
> > > stopped and no old data will be fetched.
> > 
> > A similar problem here.  If you extend the API, don't build up only from the
> > bottom.  Rather define from a higher POV.
> > 
> > If calling the drain trigger for capture stream doesn't make *any* sense for all
> > hardware, then we shouldn't add it.  This would be the only reason not to do
> > it.  What about empty streams...?
> > 
> > That being said, we need to define the proper abstraction, what this trigger
> > does for which purpose, not specific to a certain hardware.
> > That is, the requirement goes up from your hardware level while the design
> > comes down from the abstraction level.
> 
> Right. I will include the capture.
> 
> My patch will be like:
> 
> @@ -1566,6 +1567,11 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
>  			snd_pcm_post_stop(substream, new_state);
>  		}
>  	}
> +
> +	if (substream->runtime->trigger_master == substream &&
> +		(substream->runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))
> +		return substream->ops->trigger(substream,
> +				SNDRV_PCM_TRIGGER_DRAIN);
> +
>  	return 0;
>  }
> 
> I moved the trigger at the end of the function. Or do you think we need trigger 
> before snd_pcm_do_stop() for capture case based on your knowledge?

The place would be OK, but you can't call it unconditionally there.
In some cases (e.g. XRUN), the trigger won't be called at all.
The trigger state change is a bit tricky, so be careful to track the
state change there.

> 
> And in the snd_pcm_hw_refine():
> 
> @@ -420,7 +420,8 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  
>  	hw = &substream->runtime->hw;
>  	if (!params->info) {
> -		params->info = hw->info & ~SNDRV_PCM_INFO_FIFO_IN_FRAMES;
> +		params->info = hw->info & ~SNDRV_PCM_INFO_FIFO_IN_FRAMES
> +			& ~SNDRV_PCM_INFO_DRAIN_TRIGGER;
>  		if (!hw_support_mmap(substream))
>  			params->info &= ~(SNDRV_PCM_INFO_MMAP |
>  					  SNDRV_PCM_INFO_MMAP_VALID);
> 
> Do you think whether it is OK?

Yes.


thanks,

Takashi

Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[Yang, Libin]

Hi Takashi,

Regards,
Libin


> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Thursday, December 25, 2014 6:11 PM
> To: Yang, Libin
> Cc: broonie@kernel.org; alsa-devel@alsa-project.org;
> liam.r.girdwood@linux.intel.com; Jie, Yang
> Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger
> 
> At Mon, 22 Dec 2014 07:42:38 +0000,
> Yang, Libin wrote:
> >
> > Hi Takashi,
> >
> > > > > Besides, this patch will break many drivers.  Many of them will
> > > > > spew errors if an unexpected trigger command is issued.  So, you
> > > > > have to limit this trigger only to some drivers.  A typical
> > > > > workaround is to add a new
> > > > > SNDRV_PCM_INFO_* and issue the trigger only when the flag is
> > > > > set, as done for resume and pause operations.
> > > >
> > > > Yes, that's a good suggestion. Thanks for reminding me.
> > >
> > > OTOH, the info flag will be exposed to user-space while this is a
> > > kernel-space- only information.  So, in the code copying the info
> > > flag
> > > (snd_pcm_hw_refine()) would need to drop this bit, too.
> >
> > Thanks, I will drop this bit in snd_pcm_hw_refine().
> >
> > >
> > > Alternatively, we may add just a new flag in struct snd_pcm or
> > > struct snd_pcm_substream and let the driver setting it manually.
> > > This might need more changes in ASoC side, though, as the PCM
> > > creation isn't directly done by the driver.
> >
> > Yes, it is a little complex for asoc if using this method.
> > Not sure if it is OK we set the flag in snd_soc_ops->startup() each
> > time when the function is called.
> >
> > What do you think we just use the first method?
> >
> > >
> > >
> > > > > Also, do you want to see this trigger even if the stream isn't
> > > > > started before drain is issued?  Also, why no need for this
> > > > > trigger for
> > > capture streams?
> > > >
> > > > In this case, the trigger action will be started. For the stream
> > > > not started case, if I skip the trigger , I don't know where I can
> > > > have the chance to trigger later. And I didn't find the suitable
> > > > place to trigger my
> > > code.
> > > > Put the trigger at the end of the function snd_pcm_do_drain_init()
> > > > or in the function snd_pcm_post_drain_init() will be OK?
> > > >
> > > > I didn't include the capture case because we found the capture
> > > > currently don't need the trigger.
> > > >
> > > > Our issue is the fw don't know when to stop fetching the data. For
> > > > playback we need fill silence data when draining, otherwise the fw
> > > > will get
> > > the old data.
> > > > But for capture, after the driver has fetched the data, it will
> > > > stopped and no old data will be fetched.
> > >
> > > A similar problem here.  If you extend the API, don't build up only
> > > from the bottom.  Rather define from a higher POV.
> > >
> > > If calling the drain trigger for capture stream doesn't make *any*
> > > sense for all hardware, then we shouldn't add it.  This would be the
> > > only reason not to do it.  What about empty streams...?
> > >
> > > That being said, we need to define the proper abstraction, what this
> > > trigger does for which purpose, not specific to a certain hardware.
> > > That is, the requirement goes up from your hardware level while the
> > > design comes down from the abstraction level.
> >
> > Right. I will include the capture.
> >
> > My patch will be like:
> >
> > @@ -1566,6 +1567,11 @@ static int snd_pcm_do_drain_init(struct
> snd_pcm_substream *substream, int state)
> >  			snd_pcm_post_stop(substream, new_state);
> >  		}
> >  	}
> > +
> > +	if (substream->runtime->trigger_master == substream &&
> > +		(substream->runtime->hw.info &
> SNDRV_PCM_INFO_DRAIN_TRIGGER))
> > +		return substream->ops->trigger(substream,
> > +				SNDRV_PCM_TRIGGER_DRAIN);
> > +
> >  	return 0;
> >  }
> >
> > I moved the trigger at the end of the function. Or do you think we
> > need trigger before snd_pcm_do_stop() for capture case based on your
> knowledge?
> 
> The place would be OK, but you can't call it unconditionally there.
> In some cases (e.g. XRUN), the trigger won't be called at all.
> The trigger state change is a bit tricky, so be careful to track the state change
> there.
> 

Right. What do you think the following code?

@@ -1518,6 +1519,8 @@ static int snd_pcm_pre_drain_init(struct snd_pcm_substream *substream, int state
 static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
+	int trigger = 0;
+
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		switch (runtime->status->state) {
 		case SNDRV_PCM_STATE_PREPARED:
@@ -1525,10 +1528,12 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 			if (! snd_pcm_playback_empty(substream)) {
 				snd_pcm_do_start(substream, SNDRV_PCM_STATE_DRAINING);
 				snd_pcm_post_start(substream, SNDRV_PCM_STATE_DRAINING);
+				trigger = 1;
 			}
 			break;
 		case SNDRV_PCM_STATE_RUNNING:
 			runtime->status->state = SNDRV_PCM_STATE_DRAINING;
+			trigger = 1;
 			break;
 		case SNDRV_PCM_STATE_XRUN:
 			runtime->status->state = SNDRV_PCM_STATE_SETUP;
@@ -1545,6 +1550,12 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 			snd_pcm_post_stop(substream, new_state);
 		}
 	}
+
+	if (substream->runtime->trigger_master == substream && trigger &&
+		(substream->runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))
+		return substream->ops->trigger(substream,
+				SNDRV_PCM_TRIGGER_DRAIN);
+
 	return 0;
 }

> >
> > And in the snd_pcm_hw_refine():
> >
> > @@ -420,7 +420,8 @@ int snd_pcm_hw_refine(struct
> snd_pcm_substream
> > *substream,
> >
> >  	hw = &substream->runtime->hw;
> >  	if (!params->info) {
> > -		params->info = hw->info &
> ~SNDRV_PCM_INFO_FIFO_IN_FRAMES;
> > +		params->info = hw->info &
> ~SNDRV_PCM_INFO_FIFO_IN_FRAMES
> > +			& ~SNDRV_PCM_INFO_DRAIN_TRIGGER;
> >  		if (!hw_support_mmap(substream))
> >  			params->info &= ~(SNDRV_PCM_INFO_MMAP |
> >  					  SNDRV_PCM_INFO_MMAP_VALID);
> >
> > Do you think whether it is OK?
> 
> Yes.
> 
> 
> thanks,
> 
> Takashi

Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

[Takashi Iwai]

At Tue, 30 Dec 2014 08:42:28 +0000,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> Regards,
> Libin
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Thursday, December 25, 2014 6:11 PM
> > To: Yang, Libin
> > Cc: broonie@kernel.org; alsa-devel@alsa-project.org;
> > liam.r.girdwood@linux.intel.com; Jie, Yang
> > Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger
> > 
> > At Mon, 22 Dec 2014 07:42:38 +0000,
> > Yang, Libin wrote:
> > >
> > > Hi Takashi,
> > >
> > > > > > Besides, this patch will break many drivers.  Many of them will
> > > > > > spew errors if an unexpected trigger command is issued.  So, you
> > > > > > have to limit this trigger only to some drivers.  A typical
> > > > > > workaround is to add a new
> > > > > > SNDRV_PCM_INFO_* and issue the trigger only when the flag is
> > > > > > set, as done for resume and pause operations.
> > > > >
> > > > > Yes, that's a good suggestion. Thanks for reminding me.
> > > >
> > > > OTOH, the info flag will be exposed to user-space while this is a
> > > > kernel-space- only information.  So, in the code copying the info
> > > > flag
> > > > (snd_pcm_hw_refine()) would need to drop this bit, too.
> > >
> > > Thanks, I will drop this bit in snd_pcm_hw_refine().
> > >
> > > >
> > > > Alternatively, we may add just a new flag in struct snd_pcm or
> > > > struct snd_pcm_substream and let the driver setting it manually.
> > > > This might need more changes in ASoC side, though, as the PCM
> > > > creation isn't directly done by the driver.
> > >
> > > Yes, it is a little complex for asoc if using this method.
> > > Not sure if it is OK we set the flag in snd_soc_ops->startup() each
> > > time when the function is called.
> > >
> > > What do you think we just use the first method?
> > >
> > > >
> > > >
> > > > > > Also, do you want to see this trigger even if the stream isn't
> > > > > > started before drain is issued?  Also, why no need for this
> > > > > > trigger for
> > > > capture streams?
> > > > >
> > > > > In this case, the trigger action will be started. For the stream
> > > > > not started case, if I skip the trigger , I don't know where I can
> > > > > have the chance to trigger later. And I didn't find the suitable
> > > > > place to trigger my
> > > > code.
> > > > > Put the trigger at the end of the function snd_pcm_do_drain_init()
> > > > > or in the function snd_pcm_post_drain_init() will be OK?
> > > > >
> > > > > I didn't include the capture case because we found the capture
> > > > > currently don't need the trigger.
> > > > >
> > > > > Our issue is the fw don't know when to stop fetching the data. For
> > > > > playback we need fill silence data when draining, otherwise the fw
> > > > > will get
> > > > the old data.
> > > > > But for capture, after the driver has fetched the data, it will
> > > > > stopped and no old data will be fetched.
> > > >
> > > > A similar problem here.  If you extend the API, don't build up only
> > > > from the bottom.  Rather define from a higher POV.
> > > >
> > > > If calling the drain trigger for capture stream doesn't make *any*
> > > > sense for all hardware, then we shouldn't add it.  This would be the
> > > > only reason not to do it.  What about empty streams...?
> > > >
> > > > That being said, we need to define the proper abstraction, what this
> > > > trigger does for which purpose, not specific to a certain hardware.
> > > > That is, the requirement goes up from your hardware level while the
> > > > design comes down from the abstraction level.
> > >
> > > Right. I will include the capture.
> > >
> > > My patch will be like:
> > >
> > > @@ -1566,6 +1567,11 @@ static int snd_pcm_do_drain_init(struct
> > snd_pcm_substream *substream, int state)
> > >  			snd_pcm_post_stop(substream, new_state);
> > >  		}
> > >  	}
> > > +
> > > +	if (substream->runtime->trigger_master == substream &&
> > > +		(substream->runtime->hw.info &
> > SNDRV_PCM_INFO_DRAIN_TRIGGER))
> > > +		return substream->ops->trigger(substream,
> > > +				SNDRV_PCM_TRIGGER_DRAIN);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > I moved the trigger at the end of the function. Or do you think we
> > > need trigger before snd_pcm_do_stop() for capture case based on your
> > knowledge?
> > 
> > The place would be OK, but you can't call it unconditionally there.
> > In some cases (e.g. XRUN), the trigger won't be called at all.
> > The trigger state change is a bit tricky, so be careful to track the state change
> > there.
> > 
> 
> Right. What do you think the following code?
> 
> @@ -1518,6 +1519,8 @@ static int snd_pcm_pre_drain_init(struct snd_pcm_substream *substream, int state
>  static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
> +	int trigger = 0;
> +
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>  		switch (runtime->status->state) {
>  		case SNDRV_PCM_STATE_PREPARED:
> @@ -1525,10 +1528,12 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
>  			if (! snd_pcm_playback_empty(substream)) {
>  				snd_pcm_do_start(substream, SNDRV_PCM_STATE_DRAINING);
>  				snd_pcm_post_start(substream, SNDRV_PCM_STATE_DRAINING);
> +				trigger = 1;
>  			}
>  			break;
>  		case SNDRV_PCM_STATE_RUNNING:
>  			runtime->status->state = SNDRV_PCM_STATE_DRAINING;
> +			trigger = 1;
>  			break;
>  		case SNDRV_PCM_STATE_XRUN:
>  			runtime->status->state = SNDRV_PCM_STATE_SETUP;
> @@ -1545,6 +1550,12 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
>  			snd_pcm_post_stop(substream, new_state);
>  		}
>  	}
> +
> +	if (substream->runtime->trigger_master == substream && trigger &&
> +		(substream->runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))
> +		return substream->ops->trigger(substream,
> +				SNDRV_PCM_TRIGGER_DRAIN);
> +

Better to use bool in such a case.
In anyway, an easier way would be to check like

	if (runtime->status->state == SNDRV_PCM_STATE_DRAINING &&
	    runtime->trigger_master == substream &&
	    (runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))


Takashi