[PATCH] rme96 synchronization support

[PATCH] rme96 synchronization support

[Knut Petersen]

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

Hi everybody!

As I need support for synchronized pcm streams on an
RME Digi96/8 PAD I wrote the attached patch.

It does work here. My own program, trying to get synchronized
capture/playback streams, succeeds.

Programs that use either the playback or the capture part of the
hardware are not affected by start / pause/ stop operations of applications
using the other part of the hardware (tested with aplay, arecord, and ecasound).

I wrote the test code and the patch, so it is not impossible that I acted on
wrong assumptions in both places. Please have a close look at the patch.

cu,
  knut

[-- Attachment #2: 0001-alsa-Add-support-for-synchronized-start-pause-stop.patch --]
[-- Type: text/x-patch, Size: 8810 bytes --]

>From 861be0a7216b9adfdc1632e6812a71417819c7f0 Mon Sep 17 00:00:00 2001
From: Knut Petersen <Knut_Petersen@t-online.de>
Date: Mon, 5 Aug 2013 13:47:59 +0200
Subject: [PATCH] alsa: Add support for synchronized start/pause/stop of pcm
 streams to the rme96 driver.

Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
---
 sound/pci/rme96.c | 110 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 86 insertions(+), 24 deletions(-)

diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index 5fb88ac..f7e3bd7 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -344,6 +344,7 @@ static struct snd_pcm_hardware snd_rme96_playback_spdif_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+                              SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -373,6 +374,7 @@ static struct snd_pcm_hardware snd_rme96_capture_spdif_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+                              SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -402,6 +404,7 @@ static struct snd_pcm_hardware snd_rme96_playback_adat_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+                              SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -427,6 +430,7 @@ static struct snd_pcm_hardware snd_rme96_capture_adat_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+                              SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -1045,53 +1049,93 @@ snd_rme96_capture_hw_params(struct snd_pcm_substream *substream,
 }
 
 static void
-snd_rme96_playback_start(struct rme96 *rme96,
+snd_rme96_playcap_start(struct rme96 *rme96,
 			 int from_pause)
 {
 	if (!from_pause) {
 		writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
+		writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
 	}
-
-	rme96->wcreg |= RME96_WCR_START;
+	rme96->wcreg |= (RME96_WCR_START | RME96_WCR_START_2);
 	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
 }
 
 static void
+snd_rme96_playback_start(struct rme96 *rme96,
+			 int from_pause)
+{
+	if ((rme96->playback_substream && rme96->capture_substream) && 
+	    (rme96->playback_substream->group == rme96->capture_substream->group)) {
+		snd_rme96_playcap_start(rme96,from_pause);
+	} else {
+		if (!from_pause) {
+			writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
+		}
+		rme96->wcreg |= RME96_WCR_START;
+		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
+	}
+}
+
+static void
 snd_rme96_capture_start(struct rme96 *rme96,
 			int from_pause)
 {
-	if (!from_pause) {
-		writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
+	if ((rme96->playback_substream && rme96->capture_substream) && 
+	    (rme96->playback_substream->group == rme96->capture_substream->group)) {
+		snd_rme96_playcap_start(rme96,from_pause);
+	} else {
+		if (!from_pause) {
+			writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
+		}
+		rme96->wcreg |= RME96_WCR_START_2;
+		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
 	}
-
-	rme96->wcreg |= RME96_WCR_START_2;
-	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
 }
 
 static void
-snd_rme96_playback_stop(struct rme96 *rme96)
+snd_rme96_playcap_stop(struct rme96 *rme96)
 {
-	/*
-	 * Check if there is an unconfirmed IRQ, if so confirm it, or else
-	 * the hardware will not stop generating interrupts
-	 */
 	rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
 	if (rme96->rcreg & RME96_RCR_IRQ) {
 		writel(0, rme96->iobase + RME96_IO_CONFIRM_PLAY_IRQ);
-	}	
-	rme96->wcreg &= ~RME96_WCR_START;
+	}
+	if (rme96->rcreg & RME96_RCR_IRQ_2) {
+		writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
+	}
+	rme96->wcreg &= ~(RME96_WCR_START | RME96_WCR_START_2);
 	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
 }
 
 static void
+snd_rme96_playback_stop(struct rme96 *rme96)
+{
+	if ((rme96->playback_substream && rme96->capture_substream) && 
+	    (rme96->playback_substream->group == rme96->capture_substream->group)) {
+		snd_rme96_playcap_stop(rme96);
+	} else {
+		rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
+		if (rme96->rcreg & RME96_RCR_IRQ) {
+			writel(0, rme96->iobase + RME96_IO_CONFIRM_PLAY_IRQ);
+		}
+		rme96->wcreg &= ~RME96_WCR_START;
+		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
+	}
+}
+
+static void
 snd_rme96_capture_stop(struct rme96 *rme96)
 {
-	rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
-	if (rme96->rcreg & RME96_RCR_IRQ_2) {
-		writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
-	}	
-	rme96->wcreg &= ~RME96_WCR_START_2;
-	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
+	if ((rme96->playback_substream && rme96->capture_substream) && 
+	    (rme96->playback_substream->group == rme96->capture_substream->group)) {
+		snd_rme96_playcap_stop(rme96);
+	} else {
+		rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
+		if (rme96->rcreg & RME96_RCR_IRQ_2) {
+			writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
+		}
+		rme96->wcreg &= ~RME96_WCR_START_2;
+		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
+	}
 }
 
 static irqreturn_t
@@ -1155,6 +1199,7 @@ snd_rme96_playback_spdif_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	spin_lock_irq(&rme96->lock);	
         if (rme96->playback_substream != NULL) {
 		spin_unlock_irq(&rme96->lock);
@@ -1191,6 +1236,7 @@ snd_rme96_capture_spdif_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	runtime->hw = snd_rme96_capture_spdif_info;
         if (snd_rme96_getinputtype(rme96) != RME96_INPUT_ANALOG &&
             (rate = snd_rme96_capture_getrate(rme96, &isadat)) > 0)
@@ -1222,6 +1268,7 @@ snd_rme96_playback_adat_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;        
 	
+	snd_pcm_set_sync(substream);
 	spin_lock_irq(&rme96->lock);	
         if (rme96->playback_substream != NULL) {
 		spin_unlock_irq(&rme96->lock);
@@ -1253,6 +1300,7 @@ snd_rme96_capture_adat_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	runtime->hw = snd_rme96_capture_adat_info;
         if (snd_rme96_getinputtype(rme96) == RME96_INPUT_ANALOG) {
                 /* makes no sense to use analog input. Note that analog
@@ -1306,7 +1354,7 @@ static int
 snd_rme96_capture_close(struct snd_pcm_substream *substream)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
-	
+
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISRECORDING(rme96)) {
 		snd_rme96_capture_stop(rme96);
@@ -1321,7 +1369,7 @@ static int
 snd_rme96_playback_prepare(struct snd_pcm_substream *substream)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
-	
+
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISPLAYING(rme96)) {
 		snd_rme96_playback_stop(rme96);
@@ -1335,7 +1383,7 @@ static int
 snd_rme96_capture_prepare(struct snd_pcm_substream *substream)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
-	
+
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISRECORDING(rme96)) {
 		snd_rme96_capture_stop(rme96);
@@ -1350,6 +1398,13 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 			   int cmd)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
+	struct snd_pcm_substream *s;
+
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (snd_pcm_substream_chip(s) == rme96) {
+			snd_pcm_trigger_done(s, substream);
+		}
+	}
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
@@ -1393,6 +1448,13 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 			  int cmd)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
+	struct snd_pcm_substream *s;
+
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (snd_pcm_substream_chip(s) == rme96) {
+			snd_pcm_trigger_done(s, substream);
+		}
+	}
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-- 
1.8.1.4


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

Re: [PATCH] rme96 synchronization support

[Clemens Ladisch]

Knut Petersen wrote:
>  			      SNDRV_PCM_INFO_MMAP_VALID |
> +                              SNDRV_PCM_INFO_SYNC_START |

Please use tabs like in other similar parts of the file.

>  static void
> -snd_rme96_playback_start(struct rme96 *rme96,
> +snd_rme96_playcap_start(struct rme96 *rme96,
>  			 int from_pause)
>  {
>  	if (!from_pause) {
>  		writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
> +		writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
>  	}
> -
> -	rme96->wcreg |= RME96_WCR_START;
> +	rme96->wcreg |= (RME96_WCR_START | RME96_WCR_START_2);
>  	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>  }
>
>  static void
> +snd_rme96_playback_start(struct rme96 *rme96,
> +			 int from_pause)
> +{
> +	if ((rme96->playback_substream && rme96->capture_substream) &&
> +	    (rme96->playback_substream->group == rme96->capture_substream->group)) {
> +		snd_rme96_playcap_start(rme96,from_pause);
> +	} else {
> +		if (!from_pause) {
> +			writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
> +		}
> +		rme96->wcreg |= RME96_WCR_START;
> +		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> +	}
> +}

This is hard to maintain because there is duplicated code.

The proper way to write a synchronized start/stop trigger is to collect
the needed register bits first, and then apply them at once to the
register; this works for both single and grouped usage:

trigger_start()
{
	unsigned int wc_bits = 0;

	snd_pcm_group_for_each_entry(s, substream) {
		if (s == playback_substream) {
			bits |= RME96_WCR_START;
			snd_pcm_trigger_done(s, substream);
		} else if (s == capture_substream) {
			bits |= RME96_WCR_START_2;
			snd_pcm_trigger_done(s, substream);
		}
	}
	rme96->wcreg |= bits;
	writel(rme96->wcreg, ...);
}

> -	
> +

Are these whitespace changes deliberate?


Regards,
Clemens

Re: [PATCH] rme96 synchronization support

[Knut Petersen]

On 05.08.2013 15:18, Clemens Ladisch wrote:
> Knut Petersen wrote:
>>   			      SNDRV_PCM_INFO_MMAP_VALID |
>> +                              SNDRV_PCM_INFO_SYNC_START |
> Please use tabs like in other similar parts of the file.

ack

>>   static void
>> +snd_rme96_playback_start(struct rme96 *rme96,
>> +			 int from_pause)
>> +{
>> +	if ((rme96->playback_substream && rme96->capture_substream) &&
>> +	    (rme96->playback_substream->group == rme96->capture_substream->group)) {
>> +		snd_rme96_playcap_start(rme96,from_pause);
>> +	} else {
>> +		if (!from_pause) {
>> +			writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
>> +		}
>> +		rme96->wcreg |= RME96_WCR_START;
>> +		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>> +	}
>> +}
> This is hard to maintain because there is duplicated code.
>
> The proper way to write a synchronized start/stop trigger is to collect
> the needed register bits first, and then apply them at once to the
> register; this works for both single and grouped usage:

Yes, there is duplicated code ... I´ll remove that in a V2 of the patch.
But I do not think that it is less readable/maintainable if bit masks are
prepared at the place of use ...

>> -	
>> +
> Are these whitespace changes deliberate?

No, they are artifacts of inserting/removing debug code at a place
where originally a line with nothing but a tabulator is present.

Your comments were all about coding style. Have you had a look
at the content of the code?

cu,
  Knut

Re: [PATCH] rme96 synchronization support

[Takashi Iwai]

At Tue, 06 Aug 2013 19:42:17 +0200,
Knut Petersen wrote:
> 
> On 05.08.2013 15:18, Clemens Ladisch wrote:
> > Knut Petersen wrote:
> >>   			      SNDRV_PCM_INFO_MMAP_VALID |
> >> +                              SNDRV_PCM_INFO_SYNC_START |
> > Please use tabs like in other similar parts of the file.
> 
> ack
> 
> >>   static void
> >> +snd_rme96_playback_start(struct rme96 *rme96,
> >> +			 int from_pause)
> >> +{
> >> +	if ((rme96->playback_substream && rme96->capture_substream) &&
> >> +	    (rme96->playback_substream->group == rme96->capture_substream->group)) {
> >> +		snd_rme96_playcap_start(rme96,from_pause);
> >> +	} else {
> >> +		if (!from_pause) {
> >> +			writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
> >> +		}
> >> +		rme96->wcreg |= RME96_WCR_START;
> >> +		writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> >> +	}
> >> +}
> > This is hard to maintain because there is duplicated code.
> >
> > The proper way to write a synchronized start/stop trigger is to collect
> > the needed register bits first, and then apply them at once to the
> > register; this works for both single and grouped usage:
> 
> Yes, there is duplicated code ... I´ll remove that in a V2 of the patch.
> But I do not think that it is less readable/maintainable if bit masks are
> prepared at the place of use ...
> 
> >> -	
> >> +
> > Are these whitespace changes deliberate?
> 
> No, they are artifacts of inserting/removing debug code at a place
> where originally a line with nothing but a tabulator is present.
> 
> Your comments were all about coding style. Have you had a look
> at the content of the code?

Clemens commented on your code: make a single trigger function that
can handle both playback and capture (one or both) streams.  It makes
the code more maintainable.

Hint: even for clearing the position register, you can do it in a
single place like below:

	if (!from_pause) {
		if (bits & RME96_WCR_START)
	 		writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
		if (bits & RME96_WCR_START_2)
			writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
	}


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] rme96 synchronization support

[Knut Petersen]

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

Hi everybody!

Version 2 of the patch adding pcm stream synchronization support
to the rme96 driver.

cu,
  Knut

[-- Attachment #2: 0001-alsa-rme96-Add-support-for-synchronized-start-pause-.patch --]
[-- Type: text/x-patch, Size: 12109 bytes --]

>From 015bc7f9ebcbf01ce209a4f859bbfa9c31885483 Mon Sep 17 00:00:00 2001
From: Knut Petersen <Knut_Petersen@t-online.de>
Date: Thu, 8 Aug 2013 11:59:27 +0200
Subject: [PATCH] alsa/rme96: Add support for synchronized start/pause/stop of
 pcm streams to the rme96 driver. 2nd version.

Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
---
 sound/pci/rme96.c | 145 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 90 insertions(+), 55 deletions(-)

diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index 5fb88ac..35651c5 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -198,6 +198,24 @@ MODULE_PARM_DESC(enable, "Enable RME Digi96 soundcard.");
 #define RME96_AD1852_VOL_BITS 14
 #define RME96_AD1855_VOL_BITS 10
 
+/* Defines for snd_rme96_trigger */
+#define RME96_TB_START_PLAYBACK 1
+#define RME96_TB_START_CAPTURE 2
+#define RME96_TB_STOP_PLAYBACK 4
+#define RME96_TB_STOP_CAPTURE 8
+#define RME96_TB_RESET_PLAYPOS 16
+#define RME96_TB_RESET_CAPTUREPOS 32
+#define RME96_TB_CLEAR_PLAYBACK_IRQ 64
+#define RME96_TB_CLEAR_CAPTURE_IRQ 128
+#define RME96_RESUME_PLAYBACK (RME96_TB_START_PLAYBACK)
+#define RME96_RESUME_CAPTURE  (RME96_TB_START_CAPTURE)
+#define RME96_RESUME_BOTH     (RME96_RESUME_PLAYBACK   | RME96_RESUME_CAPTURE)
+#define RME96_START_PLAYBACK  (RME96_TB_START_PLAYBACK | RME96_TB_RESET_PLAYPOS)
+#define RME96_START_CAPTURE   (RME96_TB_START_CAPTURE  | RME96_TB_RESET_CAPTUREPOS)
+#define RME96_START_BOTH      (RME96_START_PLAYBACK    | RME96_START_CAPTURE)
+#define RME96_STOP_PLAYBACK   (RME96_TB_STOP_PLAYBACK  | RME96_TB_CLEAR_PLAYBACK_IRQ)
+#define RME96_STOP_CAPTURE    (RME96_TB_STOP_CAPTURE   | RME96_TB_CLEAR_CAPTURE_IRQ)
+#define RME96_STOP_BOTH       (RME96_STOP_PLAYBACK     | RME96_STOP_CAPTURE)
 
 struct rme96 {
 	spinlock_t    lock;
@@ -344,6 +362,7 @@ static struct snd_pcm_hardware snd_rme96_playback_spdif_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+			      SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -373,6 +392,7 @@ static struct snd_pcm_hardware snd_rme96_capture_spdif_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+			      SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -402,6 +422,7 @@ static struct snd_pcm_hardware snd_rme96_playback_adat_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+			      SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -427,6 +448,7 @@ static struct snd_pcm_hardware snd_rme96_capture_adat_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+			      SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -1045,54 +1067,43 @@ snd_rme96_capture_hw_params(struct snd_pcm_substream *substream,
 }
 
 static void
-snd_rme96_playback_start(struct rme96 *rme96,
-			 int from_pause)
+snd_rme96_trigger(struct rme96 *rme96,
+		  int op)
 {
-	if (!from_pause) {
+	if (op & RME96_TB_RESET_PLAYPOS) {
 		writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
 	}
-
-	rme96->wcreg |= RME96_WCR_START;
-	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
-}
-
-static void
-snd_rme96_capture_start(struct rme96 *rme96,
-			int from_pause)
-{
-	if (!from_pause) {
+	if (op & RME96_TB_RESET_CAPTUREPOS) {
 		writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
 	}
-
-	rme96->wcreg |= RME96_WCR_START_2;
+	if(op & RME96_TB_CLEAR_PLAYBACK_IRQ) {
+		rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
+		if (rme96->rcreg & RME96_RCR_IRQ) {
+			writel(0, rme96->iobase + RME96_IO_CONFIRM_PLAY_IRQ);
+		}
+	}
+	if(op & RME96_TB_CLEAR_CAPTURE_IRQ) {
+		rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
+		if (rme96->rcreg & RME96_RCR_IRQ_2) {
+			writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
+		}
+	}
+	if (op & RME96_TB_START_PLAYBACK) {
+		rme96->wcreg |= RME96_WCR_START;
+	}
+	if (op & RME96_TB_STOP_PLAYBACK) {
+		rme96->wcreg &= ~RME96_WCR_START;
+	}
+	if (op & RME96_TB_START_CAPTURE) {
+		rme96->wcreg |= RME96_WCR_START_2;
+	}
+	if (op & RME96_TB_STOP_CAPTURE) {
+		rme96->wcreg &= ~RME96_WCR_START_2;
+	}
 	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
 }
 
-static void
-snd_rme96_playback_stop(struct rme96 *rme96)
-{
-	/*
-	 * Check if there is an unconfirmed IRQ, if so confirm it, or else
-	 * the hardware will not stop generating interrupts
-	 */
-	rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
-	if (rme96->rcreg & RME96_RCR_IRQ) {
-		writel(0, rme96->iobase + RME96_IO_CONFIRM_PLAY_IRQ);
-	}	
-	rme96->wcreg &= ~RME96_WCR_START;
-	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
-}
 
-static void
-snd_rme96_capture_stop(struct rme96 *rme96)
-{
-	rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
-	if (rme96->rcreg & RME96_RCR_IRQ_2) {
-		writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
-	}	
-	rme96->wcreg &= ~RME96_WCR_START_2;
-	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
-}
 
 static irqreturn_t
 snd_rme96_interrupt(int irq,
@@ -1155,6 +1166,7 @@ snd_rme96_playback_spdif_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	spin_lock_irq(&rme96->lock);	
         if (rme96->playback_substream != NULL) {
 		spin_unlock_irq(&rme96->lock);
@@ -1191,6 +1203,7 @@ snd_rme96_capture_spdif_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	runtime->hw = snd_rme96_capture_spdif_info;
         if (snd_rme96_getinputtype(rme96) != RME96_INPUT_ANALOG &&
             (rate = snd_rme96_capture_getrate(rme96, &isadat)) > 0)
@@ -1222,6 +1235,7 @@ snd_rme96_playback_adat_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;        
 	
+	snd_pcm_set_sync(substream);
 	spin_lock_irq(&rme96->lock);	
         if (rme96->playback_substream != NULL) {
 		spin_unlock_irq(&rme96->lock);
@@ -1253,6 +1267,7 @@ snd_rme96_capture_adat_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	runtime->hw = snd_rme96_capture_adat_info;
         if (snd_rme96_getinputtype(rme96) == RME96_INPUT_ANALOG) {
                 /* makes no sense to use analog input. Note that analog
@@ -1288,7 +1303,7 @@ snd_rme96_playback_close(struct snd_pcm_substream *substream)
 
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISPLAYING(rme96)) {
-		snd_rme96_playback_stop(rme96);
+		snd_rme96_trigger(rme96,RME96_STOP_PLAYBACK);
 	}
 	rme96->playback_substream = NULL;
 	rme96->playback_periodsize = 0;
@@ -1309,7 +1324,7 @@ snd_rme96_capture_close(struct snd_pcm_substream *substream)
 	
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISRECORDING(rme96)) {
-		snd_rme96_capture_stop(rme96);
+		snd_rme96_trigger(rme96,RME96_STOP_CAPTURE);
 	}
 	rme96->capture_substream = NULL;
 	rme96->capture_periodsize = 0;
@@ -1324,7 +1339,7 @@ snd_rme96_playback_prepare(struct snd_pcm_substream *substream)
 	
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISPLAYING(rme96)) {
-		snd_rme96_playback_stop(rme96);
+		snd_rme96_trigger(rme96,RME96_STOP_PLAYBACK);
 	}
 	writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
 	spin_unlock_irq(&rme96->lock);
@@ -1338,7 +1353,7 @@ snd_rme96_capture_prepare(struct snd_pcm_substream *substream)
 	
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISRECORDING(rme96)) {
-		snd_rme96_capture_stop(rme96);
+		snd_rme96_trigger(rme96,RME96_STOP_CAPTURE);
 	}
 	writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
 	spin_unlock_irq(&rme96->lock);
@@ -1350,6 +1365,17 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 			   int cmd)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
+	struct snd_pcm_substream *s;
+	bool sync;
+
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (snd_pcm_substream_chip(s) == rme96) {
+			snd_pcm_trigger_done(s, substream);
+		}
+	}
+
+	sync = (rme96->playback_substream && rme96->capture_substream) &&
+	       (rme96->playback_substream->group == rme96->capture_substream->group);
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
@@ -1357,7 +1383,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 			if (substream != rme96->playback_substream) {
 				return -EBUSY;
 			}
-			snd_rme96_playback_start(rme96, 0);
+			snd_rme96_trigger(rme96, sync ? RME96_START_BOTH : RME96_START_PLAYBACK);
 		}
 		break;
 
@@ -1366,19 +1392,19 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 			if (substream != rme96->playback_substream) {
 				return -EBUSY;
 			}
-			snd_rme96_playback_stop(rme96);
+			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH :  RME96_STOP_PLAYBACK);
 		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		if (RME96_ISPLAYING(rme96)) {
-			snd_rme96_playback_stop(rme96);
+			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH : RME96_STOP_PLAYBACK);
 		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (!RME96_ISPLAYING(rme96)) {
-			snd_rme96_playback_start(rme96, 1);
+			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH : RME96_RESUME_PLAYBACK);
 		}
 		break;
 		
@@ -1393,6 +1419,17 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 			  int cmd)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
+	struct snd_pcm_substream *s;
+	bool sync;
+
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (snd_pcm_substream_chip(s) == rme96) {
+			snd_pcm_trigger_done(s, substream);
+		}
+	}
+
+	sync = (rme96->playback_substream && rme96->capture_substream) &&
+	       (rme96->playback_substream->group == rme96->capture_substream->group);
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
@@ -1400,7 +1437,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 			if (substream != rme96->capture_substream) {
 				return -EBUSY;
 			}
-			snd_rme96_capture_start(rme96, 0);
+			snd_rme96_trigger(rme96, sync ? RME96_START_BOTH : RME96_START_CAPTURE);
 		}
 		break;
 
@@ -1409,19 +1446,19 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 			if (substream != rme96->capture_substream) {
 				return -EBUSY;
 			}
-			snd_rme96_capture_stop(rme96);
+			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH : RME96_STOP_CAPTURE);
 		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		if (RME96_ISRECORDING(rme96)) {
-			snd_rme96_capture_stop(rme96);
+			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH : RME96_STOP_CAPTURE);
 		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (!RME96_ISRECORDING(rme96)) {
-			snd_rme96_capture_start(rme96, 1);
+			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH : RME96_RESUME_CAPTURE);
 		}
 		break;
 		
@@ -1505,8 +1542,7 @@ snd_rme96_free(void *private_data)
 	        return;
 	}
 	if (rme96->irq >= 0) {
-		snd_rme96_playback_stop(rme96);
-		snd_rme96_capture_stop(rme96);
+		snd_rme96_trigger(rme96,RME96_STOP_BOTH);
 		rme96->areg &= ~RME96_AR_DAC_EN;
 		writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
 		free_irq(rme96->irq, (void *)rme96);
@@ -1606,8 +1642,7 @@ snd_rme96_create(struct rme96 *rme96)
 	rme96->capture_periodsize = 0;
 	
 	/* make sure playback/capture is stopped, if by some reason active */
-	snd_rme96_playback_stop(rme96);
-	snd_rme96_capture_stop(rme96);
+	snd_rme96_trigger(rme96,RME96_STOP_BOTH);
 	
 	/* set default values in registers */
 	rme96->wcreg =
-- 
1.8.1.4


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

Re: [PATCH] rme96 synchronization support

[Takashi Iwai]

At Thu, 08 Aug 2013 12:52:20 +0200,
Knut Petersen wrote:
> 
> Hi everybody!
> 
> Version 2 of the patch adding pcm stream synchronization support
> to the rme96 driver.

Well, looking at rme96.c, the rme96 driver code can be cleaned up and
lots of redundant checks can be reduced, then the sync-support patch
will be the half size.  For example, it's almost useless to check
RME96_ISPLAYING() in trigger, or doing in close or prepare callback.
(It's still valid to do it in snd_rme96_create(), though, since the
 register values are unknown at the first initialization state,
 though.)

But it's no serious problem, and we can live with the current code.
So the only problem is the coding-style issues in your patch.

Please fix coding-style issues reported by scripts/checkpatch.pl.
Shorten your subject line, give more texts in the patch description
instead.  After these fixes, please resubmit.


thanks,

Takashi

> 
> cu,
>   Knut
> [2 0001-alsa-rme96-Add-support-for-synchronized-start-pause-.patch <text/x-patch (7bit)>]
> >From 015bc7f9ebcbf01ce209a4f859bbfa9c31885483 Mon Sep 17 00:00:00 2001
> From: Knut Petersen <Knut_Petersen@t-online.de>
> Date: Thu, 8 Aug 2013 11:59:27 +0200
> Subject: [PATCH] alsa/rme96: Add support for synchronized start/pause/stop of
>  pcm streams to the rme96 driver. 2nd version.
> 
> Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
> ---
>  sound/pci/rme96.c | 145 +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 90 insertions(+), 55 deletions(-)
> 
> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
> index 5fb88ac..35651c5 100644
> --- a/sound/pci/rme96.c
> +++ b/sound/pci/rme96.c
> @@ -198,6 +198,24 @@ MODULE_PARM_DESC(enable, "Enable RME Digi96 soundcard.");
>  #define RME96_AD1852_VOL_BITS 14
>  #define RME96_AD1855_VOL_BITS 10
>  
> +/* Defines for snd_rme96_trigger */
> +#define RME96_TB_START_PLAYBACK 1
> +#define RME96_TB_START_CAPTURE 2
> +#define RME96_TB_STOP_PLAYBACK 4
> +#define RME96_TB_STOP_CAPTURE 8
> +#define RME96_TB_RESET_PLAYPOS 16
> +#define RME96_TB_RESET_CAPTUREPOS 32
> +#define RME96_TB_CLEAR_PLAYBACK_IRQ 64
> +#define RME96_TB_CLEAR_CAPTURE_IRQ 128
> +#define RME96_RESUME_PLAYBACK (RME96_TB_START_PLAYBACK)
> +#define RME96_RESUME_CAPTURE  (RME96_TB_START_CAPTURE)
> +#define RME96_RESUME_BOTH     (RME96_RESUME_PLAYBACK   | RME96_RESUME_CAPTURE)
> +#define RME96_START_PLAYBACK  (RME96_TB_START_PLAYBACK | RME96_TB_RESET_PLAYPOS)
> +#define RME96_START_CAPTURE   (RME96_TB_START_CAPTURE  | RME96_TB_RESET_CAPTUREPOS)
> +#define RME96_START_BOTH      (RME96_START_PLAYBACK    | RME96_START_CAPTURE)
> +#define RME96_STOP_PLAYBACK   (RME96_TB_STOP_PLAYBACK  | RME96_TB_CLEAR_PLAYBACK_IRQ)
> +#define RME96_STOP_CAPTURE    (RME96_TB_STOP_CAPTURE   | RME96_TB_CLEAR_CAPTURE_IRQ)
> +#define RME96_STOP_BOTH       (RME96_STOP_PLAYBACK     | RME96_STOP_CAPTURE)
>  
>  struct rme96 {
>  	spinlock_t    lock;
> @@ -344,6 +362,7 @@ static struct snd_pcm_hardware snd_rme96_playback_spdif_info =
>  {
>  	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
>  			      SNDRV_PCM_INFO_MMAP_VALID |
> +			      SNDRV_PCM_INFO_SYNC_START |
>  			      SNDRV_PCM_INFO_INTERLEAVED |
>  			      SNDRV_PCM_INFO_PAUSE),
>  	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
> @@ -373,6 +392,7 @@ static struct snd_pcm_hardware snd_rme96_capture_spdif_info =
>  {
>  	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
>  			      SNDRV_PCM_INFO_MMAP_VALID |
> +			      SNDRV_PCM_INFO_SYNC_START |
>  			      SNDRV_PCM_INFO_INTERLEAVED |
>  			      SNDRV_PCM_INFO_PAUSE),
>  	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
> @@ -402,6 +422,7 @@ static struct snd_pcm_hardware snd_rme96_playback_adat_info =
>  {
>  	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
>  			      SNDRV_PCM_INFO_MMAP_VALID |
> +			      SNDRV_PCM_INFO_SYNC_START |
>  			      SNDRV_PCM_INFO_INTERLEAVED |
>  			      SNDRV_PCM_INFO_PAUSE),
>  	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
> @@ -427,6 +448,7 @@ static struct snd_pcm_hardware snd_rme96_capture_adat_info =
>  {
>  	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
>  			      SNDRV_PCM_INFO_MMAP_VALID |
> +			      SNDRV_PCM_INFO_SYNC_START |
>  			      SNDRV_PCM_INFO_INTERLEAVED |
>  			      SNDRV_PCM_INFO_PAUSE),
>  	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
> @@ -1045,54 +1067,43 @@ snd_rme96_capture_hw_params(struct snd_pcm_substream *substream,
>  }
>  
>  static void
> -snd_rme96_playback_start(struct rme96 *rme96,
> -			 int from_pause)
> +snd_rme96_trigger(struct rme96 *rme96,
> +		  int op)
>  {
> -	if (!from_pause) {
> +	if (op & RME96_TB_RESET_PLAYPOS) {
>  		writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
>  	}
> -
> -	rme96->wcreg |= RME96_WCR_START;
> -	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> -}
> -
> -static void
> -snd_rme96_capture_start(struct rme96 *rme96,
> -			int from_pause)
> -{
> -	if (!from_pause) {
> +	if (op & RME96_TB_RESET_CAPTUREPOS) {
>  		writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
>  	}
> -
> -	rme96->wcreg |= RME96_WCR_START_2;
> +	if(op & RME96_TB_CLEAR_PLAYBACK_IRQ) {
> +		rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
> +		if (rme96->rcreg & RME96_RCR_IRQ) {
> +			writel(0, rme96->iobase + RME96_IO_CONFIRM_PLAY_IRQ);
> +		}
> +	}
> +	if(op & RME96_TB_CLEAR_CAPTURE_IRQ) {
> +		rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
> +		if (rme96->rcreg & RME96_RCR_IRQ_2) {
> +			writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
> +		}
> +	}
> +	if (op & RME96_TB_START_PLAYBACK) {
> +		rme96->wcreg |= RME96_WCR_START;
> +	}
> +	if (op & RME96_TB_STOP_PLAYBACK) {
> +		rme96->wcreg &= ~RME96_WCR_START;
> +	}
> +	if (op & RME96_TB_START_CAPTURE) {
> +		rme96->wcreg |= RME96_WCR_START_2;
> +	}
> +	if (op & RME96_TB_STOP_CAPTURE) {
> +		rme96->wcreg &= ~RME96_WCR_START_2;
> +	}
>  	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
>  }
>  
> -static void
> -snd_rme96_playback_stop(struct rme96 *rme96)
> -{
> -	/*
> -	 * Check if there is an unconfirmed IRQ, if so confirm it, or else
> -	 * the hardware will not stop generating interrupts
> -	 */
> -	rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
> -	if (rme96->rcreg & RME96_RCR_IRQ) {
> -		writel(0, rme96->iobase + RME96_IO_CONFIRM_PLAY_IRQ);
> -	}	
> -	rme96->wcreg &= ~RME96_WCR_START;
> -	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> -}
>  
> -static void
> -snd_rme96_capture_stop(struct rme96 *rme96)
> -{
> -	rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
> -	if (rme96->rcreg & RME96_RCR_IRQ_2) {
> -		writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
> -	}	
> -	rme96->wcreg &= ~RME96_WCR_START_2;
> -	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
> -}
>  
>  static irqreturn_t
>  snd_rme96_interrupt(int irq,
> @@ -1155,6 +1166,7 @@ snd_rme96_playback_spdif_open(struct snd_pcm_substream *substream)
>  	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  
> +	snd_pcm_set_sync(substream);
>  	spin_lock_irq(&rme96->lock);	
>          if (rme96->playback_substream != NULL) {
>  		spin_unlock_irq(&rme96->lock);
> @@ -1191,6 +1203,7 @@ snd_rme96_capture_spdif_open(struct snd_pcm_substream *substream)
>  	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  
> +	snd_pcm_set_sync(substream);
>  	runtime->hw = snd_rme96_capture_spdif_info;
>          if (snd_rme96_getinputtype(rme96) != RME96_INPUT_ANALOG &&
>              (rate = snd_rme96_capture_getrate(rme96, &isadat)) > 0)
> @@ -1222,6 +1235,7 @@ snd_rme96_playback_adat_open(struct snd_pcm_substream *substream)
>  	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>  	struct snd_pcm_runtime *runtime = substream->runtime;        
>  	
> +	snd_pcm_set_sync(substream);
>  	spin_lock_irq(&rme96->lock);	
>          if (rme96->playback_substream != NULL) {
>  		spin_unlock_irq(&rme96->lock);
> @@ -1253,6 +1267,7 @@ snd_rme96_capture_adat_open(struct snd_pcm_substream *substream)
>  	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  
> +	snd_pcm_set_sync(substream);
>  	runtime->hw = snd_rme96_capture_adat_info;
>          if (snd_rme96_getinputtype(rme96) == RME96_INPUT_ANALOG) {
>                  /* makes no sense to use analog input. Note that analog
> @@ -1288,7 +1303,7 @@ snd_rme96_playback_close(struct snd_pcm_substream *substream)
>  
>  	spin_lock_irq(&rme96->lock);	
>  	if (RME96_ISPLAYING(rme96)) {
> -		snd_rme96_playback_stop(rme96);
> +		snd_rme96_trigger(rme96,RME96_STOP_PLAYBACK);
>  	}
>  	rme96->playback_substream = NULL;
>  	rme96->playback_periodsize = 0;
> @@ -1309,7 +1324,7 @@ snd_rme96_capture_close(struct snd_pcm_substream *substream)
>  	
>  	spin_lock_irq(&rme96->lock);	
>  	if (RME96_ISRECORDING(rme96)) {
> -		snd_rme96_capture_stop(rme96);
> +		snd_rme96_trigger(rme96,RME96_STOP_CAPTURE);
>  	}
>  	rme96->capture_substream = NULL;
>  	rme96->capture_periodsize = 0;
> @@ -1324,7 +1339,7 @@ snd_rme96_playback_prepare(struct snd_pcm_substream *substream)
>  	
>  	spin_lock_irq(&rme96->lock);	
>  	if (RME96_ISPLAYING(rme96)) {
> -		snd_rme96_playback_stop(rme96);
> +		snd_rme96_trigger(rme96,RME96_STOP_PLAYBACK);
>  	}
>  	writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
>  	spin_unlock_irq(&rme96->lock);
> @@ -1338,7 +1353,7 @@ snd_rme96_capture_prepare(struct snd_pcm_substream *substream)
>  	
>  	spin_lock_irq(&rme96->lock);	
>  	if (RME96_ISRECORDING(rme96)) {
> -		snd_rme96_capture_stop(rme96);
> +		snd_rme96_trigger(rme96,RME96_STOP_CAPTURE);
>  	}
>  	writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
>  	spin_unlock_irq(&rme96->lock);
> @@ -1350,6 +1365,17 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
>  			   int cmd)
>  {
>  	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
> +	struct snd_pcm_substream *s;
> +	bool sync;
> +
> +	snd_pcm_group_for_each_entry(s, substream) {
> +		if (snd_pcm_substream_chip(s) == rme96) {
> +			snd_pcm_trigger_done(s, substream);
> +		}
> +	}
> +
> +	sync = (rme96->playback_substream && rme96->capture_substream) &&
> +	       (rme96->playback_substream->group == rme96->capture_substream->group);
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
> @@ -1357,7 +1383,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
>  			if (substream != rme96->playback_substream) {
>  				return -EBUSY;
>  			}
> -			snd_rme96_playback_start(rme96, 0);
> +			snd_rme96_trigger(rme96, sync ? RME96_START_BOTH : RME96_START_PLAYBACK);
>  		}
>  		break;
>  
> @@ -1366,19 +1392,19 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
>  			if (substream != rme96->playback_substream) {
>  				return -EBUSY;
>  			}
> -			snd_rme96_playback_stop(rme96);
> +			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH :  RME96_STOP_PLAYBACK);
>  		}
>  		break;
>  
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  		if (RME96_ISPLAYING(rme96)) {
> -			snd_rme96_playback_stop(rme96);
> +			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH : RME96_STOP_PLAYBACK);
>  		}
>  		break;
>  
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		if (!RME96_ISPLAYING(rme96)) {
> -			snd_rme96_playback_start(rme96, 1);
> +			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH : RME96_RESUME_PLAYBACK);
>  		}
>  		break;
>  		
> @@ -1393,6 +1419,17 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
>  			  int cmd)
>  {
>  	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
> +	struct snd_pcm_substream *s;
> +	bool sync;
> +
> +	snd_pcm_group_for_each_entry(s, substream) {
> +		if (snd_pcm_substream_chip(s) == rme96) {
> +			snd_pcm_trigger_done(s, substream);
> +		}
> +	}
> +
> +	sync = (rme96->playback_substream && rme96->capture_substream) &&
> +	       (rme96->playback_substream->group == rme96->capture_substream->group);
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_START:
> @@ -1400,7 +1437,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
>  			if (substream != rme96->capture_substream) {
>  				return -EBUSY;
>  			}
> -			snd_rme96_capture_start(rme96, 0);
> +			snd_rme96_trigger(rme96, sync ? RME96_START_BOTH : RME96_START_CAPTURE);
>  		}
>  		break;
>  
> @@ -1409,19 +1446,19 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
>  			if (substream != rme96->capture_substream) {
>  				return -EBUSY;
>  			}
> -			snd_rme96_capture_stop(rme96);
> +			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH : RME96_STOP_CAPTURE);
>  		}
>  		break;
>  
>  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  		if (RME96_ISRECORDING(rme96)) {
> -			snd_rme96_capture_stop(rme96);
> +			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH : RME96_STOP_CAPTURE);
>  		}
>  		break;
>  
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		if (!RME96_ISRECORDING(rme96)) {
> -			snd_rme96_capture_start(rme96, 1);
> +			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH : RME96_RESUME_CAPTURE);
>  		}
>  		break;
>  		
> @@ -1505,8 +1542,7 @@ snd_rme96_free(void *private_data)
>  	        return;
>  	}
>  	if (rme96->irq >= 0) {
> -		snd_rme96_playback_stop(rme96);
> -		snd_rme96_capture_stop(rme96);
> +		snd_rme96_trigger(rme96,RME96_STOP_BOTH);
>  		rme96->areg &= ~RME96_AR_DAC_EN;
>  		writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
>  		free_irq(rme96->irq, (void *)rme96);
> @@ -1606,8 +1642,7 @@ snd_rme96_create(struct rme96 *rme96)
>  	rme96->capture_periodsize = 0;
>  	
>  	/* make sure playback/capture is stopped, if by some reason active */
> -	snd_rme96_playback_stop(rme96);
> -	snd_rme96_capture_stop(rme96);
> +	snd_rme96_trigger(rme96,RME96_STOP_BOTH);
>  	
>  	/* set default values in registers */
>  	rme96->wcreg =
> -- 
> 1.8.1.4
> 

Re: [PATCH] rme96 synchronization support

[Knut Petersen]

On 13.08.2013 09:19, Takashi Iwai wrote:
> Well, looking at rme96.c, the rme96 driver code can be cleaned up and
> lots of redundant checks can be reduced, then the sync-support patch
> will be the half size.  For example, it's almost useless to check
> RME96_ISPLAYING() in trigger, or doing in close or prepare callback.
> (It's still valid to do it in snd_rme96_create(), though, since the
>   register values are unknown at the first initialization state,
>   though.)
>
> But it's no serious problem, and we can live with the current code.
> So the only problem is the coding-style issues in your patch.

Ok, I admit that I never used scripts/checkpatch.pl ;-) Alsa, feeding the current
rme96.c code to scripts/checkpatch.pl gives  235 errors and 205 warnings:

99 * ERROR: trailing whitespace
88 * ERROR: code indent should use tabs where possible
76 * WARNING: please, no spaces at the start of a line
72 * WARNING: line over 80 characters
39 * WARNING: braces {} are not necessary for single statement blocks
21 * ERROR: do not use assignment in if condition
18 * ERROR: that open brace { should be on the previous line
13 * WARNING: braces {} are not necessary for any arm of this statement
8 * ERROR: space required after that ',' (ctx:VxV)
3 * WARNING: quoted string split across lines
1 * ERROR: space prohibited after that '!' (ctx:BxW)
1 * WARNING: Use #include <linux/io.h> instead of <asm/io.h>
1 * WARNING: Avoid CamelCase: <snd_BUG>

Comments to the list of errors and warnings:

The CamelCase snd_BUG() isn´t a problem of rme96.c.

I don´t agree that code like "if ((err = pci_request_regions(pci, "RME96")) < 0)"
is a problem.

I don´t agree with the script that lines over 80 characters are _always_ a problem.

Nevertheless, I really don´t like code alternating between different styles, that´s the reason
for some unnecessary braces {} I used in the submitted patches.

So I suggest that I will (re)submit three patches:
- first a patch cleaning the current rme96.c source
- the PM and SYNC patches based on the first patch.

After that additional patches removing useless checks, etc could follow. But this
should be patch 4(+) as it´s not always obvious for me if a check is really useless.

Do you agree?

cu,
  Knut

Re: [PATCH] rme96 synchronization support

[Takashi Iwai]

At Tue, 13 Aug 2013 12:32:12 +0200,
Knut Petersen wrote:
> 
> On 13.08.2013 09:19, Takashi Iwai wrote:
> > Well, looking at rme96.c, the rme96 driver code can be cleaned up and
> > lots of redundant checks can be reduced, then the sync-support patch
> > will be the half size.  For example, it's almost useless to check
> > RME96_ISPLAYING() in trigger, or doing in close or prepare callback.
> > (It's still valid to do it in snd_rme96_create(), though, since the
> >   register values are unknown at the first initialization state,
> >   though.)
> >
> > But it's no serious problem, and we can live with the current code.
> > So the only problem is the coding-style issues in your patch.
> 
> Ok, I admit that I never used scripts/checkpatch.pl ;-) Alsa, feeding the current
> rme96.c code to scripts/checkpatch.pl gives  235 errors and 205 warnings:

You just need to fix the coding issue of your patch, not the whole
existing driver code.


Takashi

[PATCH] rme96 add stream synchronization and PM support

[Knut Petersen]

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

Hi everybody!

The attached patches add pcm stream synchronization support
as well as power management code to the rme96 driver.

scripts/checkpatch.pl does not show any warnings / errors, so
I hope that these attached versions of the patches are acceptable.

cu,
  Knut

[-- Attachment #2: 0001-alsa-rme96-Add-pcm-stream-synchronization.patch --]
[-- Type: text/x-patch, Size: 12579 bytes --]

>From 5edf3a59fffb7038a75c50c715ea649fd9a4cd8e Mon Sep 17 00:00:00 2001
From: Knut Petersen <Knut_Petersen@t-online.de>
Date: Tue, 13 Aug 2013 21:18:12 +0200
Subject: [PATCH 1/2] alsa/rme96: Add pcm stream synchronization

The hardware does support synchronized start/pause/stop of pcm streams,
so there is no reason not to add that feature after more than ten years.

Some minor coding style / white space fixes in the surroundings of the
changes.

Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
---
 sound/pci/rme96.c | 185 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 110 insertions(+), 75 deletions(-)

diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index 5fb88ac..3eb0bdd 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -198,6 +198,31 @@ MODULE_PARM_DESC(enable, "Enable RME Digi96 soundcard.");
 #define RME96_AD1852_VOL_BITS 14
 #define RME96_AD1855_VOL_BITS 10
 
+/* Defines for snd_rme96_trigger */
+#define RME96_TB_START_PLAYBACK 1
+#define RME96_TB_START_CAPTURE 2
+#define RME96_TB_STOP_PLAYBACK 4
+#define RME96_TB_STOP_CAPTURE 8
+#define RME96_TB_RESET_PLAYPOS 16
+#define RME96_TB_RESET_CAPTUREPOS 32
+#define RME96_TB_CLEAR_PLAYBACK_IRQ 64
+#define RME96_TB_CLEAR_CAPTURE_IRQ 128
+#define RME96_RESUME_PLAYBACK	(RME96_TB_START_PLAYBACK)
+#define RME96_RESUME_CAPTURE	(RME96_TB_START_CAPTURE)
+#define RME96_RESUME_BOTH	(RME96_RESUME_PLAYBACK \
+				| RME96_RESUME_CAPTURE)
+#define RME96_START_PLAYBACK	(RME96_TB_START_PLAYBACK \
+				| RME96_TB_RESET_PLAYPOS)
+#define RME96_START_CAPTURE	(RME96_TB_START_CAPTURE \
+				| RME96_TB_RESET_CAPTUREPOS)
+#define RME96_START_BOTH	(RME96_START_PLAYBACK \
+				| RME96_START_CAPTURE)
+#define RME96_STOP_PLAYBACK	(RME96_TB_STOP_PLAYBACK \
+				| RME96_TB_CLEAR_PLAYBACK_IRQ)
+#define RME96_STOP_CAPTURE	(RME96_TB_STOP_CAPTURE \
+				| RME96_TB_CLEAR_CAPTURE_IRQ)
+#define RME96_STOP_BOTH		(RME96_STOP_PLAYBACK \
+				| RME96_STOP_CAPTURE)
 
 struct rme96 {
 	spinlock_t    lock;
@@ -344,6 +369,7 @@ static struct snd_pcm_hardware snd_rme96_playback_spdif_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+			      SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -373,6 +399,7 @@ static struct snd_pcm_hardware snd_rme96_capture_spdif_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+			      SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -402,6 +429,7 @@ static struct snd_pcm_hardware snd_rme96_playback_adat_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+			      SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -427,6 +455,7 @@ static struct snd_pcm_hardware snd_rme96_capture_adat_info =
 {
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
+			      SNDRV_PCM_INFO_SYNC_START |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -1045,54 +1074,35 @@ snd_rme96_capture_hw_params(struct snd_pcm_substream *substream,
 }
 
 static void
-snd_rme96_playback_start(struct rme96 *rme96,
-			 int from_pause)
+snd_rme96_trigger(struct rme96 *rme96,
+		  int op)
 {
-	if (!from_pause) {
+	if (op & RME96_TB_RESET_PLAYPOS)
 		writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
-	}
-
-	rme96->wcreg |= RME96_WCR_START;
-	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
-}
-
-static void
-snd_rme96_capture_start(struct rme96 *rme96,
-			int from_pause)
-{
-	if (!from_pause) {
+	if (op & RME96_TB_RESET_CAPTUREPOS)
 		writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
-	}
-
-	rme96->wcreg |= RME96_WCR_START_2;
+	if (op & RME96_TB_CLEAR_PLAYBACK_IRQ) {
+		rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
+		if (rme96->rcreg & RME96_RCR_IRQ)
+			writel(0, rme96->iobase + RME96_IO_CONFIRM_PLAY_IRQ);
+	}
+	if (op & RME96_TB_CLEAR_CAPTURE_IRQ) {
+		rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
+		if (rme96->rcreg & RME96_RCR_IRQ_2)
+			writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
+	}
+	if (op & RME96_TB_START_PLAYBACK)
+		rme96->wcreg |= RME96_WCR_START;
+	if (op & RME96_TB_STOP_PLAYBACK)
+		rme96->wcreg &= ~RME96_WCR_START;
+	if (op & RME96_TB_START_CAPTURE)
+		rme96->wcreg |= RME96_WCR_START_2;
+	if (op & RME96_TB_STOP_CAPTURE)
+		rme96->wcreg &= ~RME96_WCR_START_2;
 	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
 }
 
-static void
-snd_rme96_playback_stop(struct rme96 *rme96)
-{
-	/*
-	 * Check if there is an unconfirmed IRQ, if so confirm it, or else
-	 * the hardware will not stop generating interrupts
-	 */
-	rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
-	if (rme96->rcreg & RME96_RCR_IRQ) {
-		writel(0, rme96->iobase + RME96_IO_CONFIRM_PLAY_IRQ);
-	}	
-	rme96->wcreg &= ~RME96_WCR_START;
-	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
-}
 
-static void
-snd_rme96_capture_stop(struct rme96 *rme96)
-{
-	rme96->rcreg = readl(rme96->iobase + RME96_IO_CONTROL_REGISTER);
-	if (rme96->rcreg & RME96_RCR_IRQ_2) {
-		writel(0, rme96->iobase + RME96_IO_CONFIRM_REC_IRQ);
-	}	
-	rme96->wcreg &= ~RME96_WCR_START_2;
-	writel(rme96->wcreg, rme96->iobase + RME96_IO_CONTROL_REGISTER);
-}
 
 static irqreturn_t
 snd_rme96_interrupt(int irq,
@@ -1155,6 +1165,7 @@ snd_rme96_playback_spdif_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	spin_lock_irq(&rme96->lock);	
         if (rme96->playback_substream != NULL) {
 		spin_unlock_irq(&rme96->lock);
@@ -1191,6 +1202,7 @@ snd_rme96_capture_spdif_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	runtime->hw = snd_rme96_capture_spdif_info;
         if (snd_rme96_getinputtype(rme96) != RME96_INPUT_ANALOG &&
             (rate = snd_rme96_capture_getrate(rme96, &isadat)) > 0)
@@ -1222,6 +1234,7 @@ snd_rme96_playback_adat_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;        
 	
+	snd_pcm_set_sync(substream);
 	spin_lock_irq(&rme96->lock);	
         if (rme96->playback_substream != NULL) {
 		spin_unlock_irq(&rme96->lock);
@@ -1253,6 +1266,7 @@ snd_rme96_capture_adat_open(struct snd_pcm_substream *substream)
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
+	snd_pcm_set_sync(substream);
 	runtime->hw = snd_rme96_capture_adat_info;
         if (snd_rme96_getinputtype(rme96) == RME96_INPUT_ANALOG) {
                 /* makes no sense to use analog input. Note that analog
@@ -1288,7 +1302,7 @@ snd_rme96_playback_close(struct snd_pcm_substream *substream)
 
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISPLAYING(rme96)) {
-		snd_rme96_playback_stop(rme96);
+		snd_rme96_trigger(rme96, RME96_STOP_PLAYBACK);
 	}
 	rme96->playback_substream = NULL;
 	rme96->playback_periodsize = 0;
@@ -1309,7 +1323,7 @@ snd_rme96_capture_close(struct snd_pcm_substream *substream)
 	
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISRECORDING(rme96)) {
-		snd_rme96_capture_stop(rme96);
+		snd_rme96_trigger(rme96, RME96_STOP_CAPTURE);
 	}
 	rme96->capture_substream = NULL;
 	rme96->capture_periodsize = 0;
@@ -1324,7 +1338,7 @@ snd_rme96_playback_prepare(struct snd_pcm_substream *substream)
 	
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISPLAYING(rme96)) {
-		snd_rme96_playback_stop(rme96);
+		snd_rme96_trigger(rme96, RME96_STOP_PLAYBACK);
 	}
 	writel(0, rme96->iobase + RME96_IO_RESET_PLAY_POS);
 	spin_unlock_irq(&rme96->lock);
@@ -1338,7 +1352,7 @@ snd_rme96_capture_prepare(struct snd_pcm_substream *substream)
 	
 	spin_lock_irq(&rme96->lock);	
 	if (RME96_ISRECORDING(rme96)) {
-		snd_rme96_capture_stop(rme96);
+		snd_rme96_trigger(rme96, RME96_STOP_CAPTURE);
 	}
 	writel(0, rme96->iobase + RME96_IO_RESET_REC_POS);
 	spin_unlock_irq(&rme96->lock);
@@ -1350,41 +1364,53 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 			   int cmd)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
+	struct snd_pcm_substream *s;
+	bool sync;
+
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (snd_pcm_substream_chip(s) == rme96)
+			snd_pcm_trigger_done(s, substream);
+	}
+
+	sync = (rme96->playback_substream && rme96->capture_substream) &&
+	       (rme96->playback_substream->group ==
+		rme96->capture_substream->group);
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		if (!RME96_ISPLAYING(rme96)) {
-			if (substream != rme96->playback_substream) {
+			if (substream != rme96->playback_substream)
 				return -EBUSY;
-			}
-			snd_rme96_playback_start(rme96, 0);
+			snd_rme96_trigger(rme96, sync ? RME96_START_BOTH
+						 : RME96_START_PLAYBACK);
 		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
 		if (RME96_ISPLAYING(rme96)) {
-			if (substream != rme96->playback_substream) {
+			if (substream != rme96->playback_substream)
 				return -EBUSY;
-			}
-			snd_rme96_playback_stop(rme96);
+			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH
+						 :  RME96_STOP_PLAYBACK);
 		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		if (RME96_ISPLAYING(rme96)) {
-			snd_rme96_playback_stop(rme96);
-		}
+		if (RME96_ISPLAYING(rme96))
+			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH
+						 : RME96_STOP_PLAYBACK);
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (!RME96_ISPLAYING(rme96)) {
-			snd_rme96_playback_start(rme96, 1);
-		}
+		if (!RME96_ISPLAYING(rme96))
+			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
+						 : RME96_RESUME_PLAYBACK);
 		break;
-		
+
 	default:
 		return -EINVAL;
 	}
+
 	return 0;
 }
 
@@ -1393,38 +1419,49 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 			  int cmd)
 {
 	struct rme96 *rme96 = snd_pcm_substream_chip(substream);
+	struct snd_pcm_substream *s;
+	bool sync;
+
+	snd_pcm_group_for_each_entry(s, substream) {
+		if (snd_pcm_substream_chip(s) == rme96)
+			snd_pcm_trigger_done(s, substream);
+	}
+
+	sync = (rme96->playback_substream && rme96->capture_substream) &&
+	       (rme96->playback_substream->group ==
+		rme96->capture_substream->group);
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 		if (!RME96_ISRECORDING(rme96)) {
-			if (substream != rme96->capture_substream) {
+			if (substream != rme96->capture_substream)
 				return -EBUSY;
-			}
-			snd_rme96_capture_start(rme96, 0);
+			snd_rme96_trigger(rme96, sync ? RME96_START_BOTH
+						 : RME96_START_CAPTURE);
 		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_STOP:
 		if (RME96_ISRECORDING(rme96)) {
-			if (substream != rme96->capture_substream) {
+			if (substream != rme96->capture_substream)
 				return -EBUSY;
-			}
-			snd_rme96_capture_stop(rme96);
+			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH
+						 : RME96_STOP_CAPTURE);
 		}
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		if (RME96_ISRECORDING(rme96)) {
-			snd_rme96_capture_stop(rme96);
-		}
+		if (RME96_ISRECORDING(rme96))
+			snd_rme96_trigger(rme96, sync ? RME96_STOP_BOTH
+						 : RME96_STOP_CAPTURE);
 		break;
 
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (!RME96_ISRECORDING(rme96)) {
-			snd_rme96_capture_start(rme96, 1);
-		}
+		if (!RME96_ISRECORDING(rme96))
+			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
+						 : RME96_RESUME_CAPTURE);
 		break;
-		
+
 	default:
 		return -EINVAL;
 	}
@@ -1505,8 +1542,7 @@ snd_rme96_free(void *private_data)
 	        return;
 	}
 	if (rme96->irq >= 0) {
-		snd_rme96_playback_stop(rme96);
-		snd_rme96_capture_stop(rme96);
+		snd_rme96_trigger(rme96, RME96_STOP_BOTH);
 		rme96->areg &= ~RME96_AR_DAC_EN;
 		writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
 		free_irq(rme96->irq, (void *)rme96);
@@ -1606,8 +1642,7 @@ snd_rme96_create(struct rme96 *rme96)
 	rme96->capture_periodsize = 0;
 	
 	/* make sure playback/capture is stopped, if by some reason active */
-	snd_rme96_playback_stop(rme96);
-	snd_rme96_capture_stop(rme96);
+	snd_rme96_trigger(rme96, RME96_STOP_BOTH);
 	
 	/* set default values in registers */
 	rme96->wcreg =
-- 
1.8.1.4


[-- Attachment #3: 0002-alsa-rme96-Add-PM-support.patch --]
[-- Type: text/x-patch, Size: 7144 bytes --]

>From becb984d46fc32313316a647a674c711b45ca044 Mon Sep 17 00:00:00 2001
From: Knut Petersen <Knut_Petersen@t-online.de>
Date: Tue, 13 Aug 2013 22:19:48 +0200
Subject: [PATCH 2/2] alsa/rme96: Add PM support

Without proper power management handling, the first use
of a Digi96/8 anytime after a suspend / resume cycle will
start playback with distortions.

Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
---
 sound/pci/rme96.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index 3eb0bdd..c309d24 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -239,6 +239,13 @@ struct rme96 {
 
 	u8 rev; /* card revision number */
 
+#ifdef CONFIG_PM
+	u32 playback_pointer;
+	u32 capture_pointer;
+	void *playback_suspend_buffer;
+	void *capture_suspend_buffer;
+#endif
+
 	struct snd_pcm_substream *playback_substream;
 	struct snd_pcm_substream *capture_substream;
 
@@ -370,6 +377,7 @@ static struct snd_pcm_hardware snd_rme96_playback_spdif_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -400,6 +408,7 @@ static struct snd_pcm_hardware snd_rme96_capture_spdif_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -430,6 +439,7 @@ static struct snd_pcm_hardware snd_rme96_playback_adat_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -456,6 +466,7 @@ static struct snd_pcm_hardware snd_rme96_capture_adat_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -1386,6 +1397,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 		}
 		break;
 
+	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 		if (RME96_ISPLAYING(rme96)) {
 			if (substream != rme96->playback_substream)
@@ -1401,6 +1413,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 						 : RME96_STOP_PLAYBACK);
 		break;
 
+	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (!RME96_ISPLAYING(rme96))
 			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
@@ -1441,6 +1454,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 		}
 		break;
 
+	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 		if (RME96_ISRECORDING(rme96)) {
 			if (substream != rme96->capture_substream)
@@ -1456,6 +1470,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 						 : RME96_STOP_CAPTURE);
 		break;
 
+	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (!RME96_ISRECORDING(rme96))
 			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
@@ -1556,6 +1571,10 @@ snd_rme96_free(void *private_data)
 		pci_release_regions(rme96->pci);
 		rme96->port = 0;
 	}
+#ifdef CONFIG_PM
+	vfree(rme96->playback_suspend_buffer);
+	vfree(rme96->capture_suspend_buffer);
+#endif
 	pci_disable_device(rme96->pci);
 }
 
@@ -2354,6 +2373,83 @@ snd_rme96_create_switches(struct snd_card *card,
  * Card initialisation
  */
 
+#ifdef CONFIG_PM
+
+static int
+snd_rme96_suspend(struct pci_dev *pci,
+		  pm_message_t state)
+{
+	struct snd_card *card = pci_get_drvdata(pci);
+	struct rme96 *rme96 = card->private_data;
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
+	snd_pcm_suspend(rme96->playback_substream);
+	snd_pcm_suspend(rme96->capture_substream);
+
+	/* save capture & playback pointers */
+	rme96->playback_pointer = readl(rme96->iobase + RME96_IO_GET_PLAY_POS)
+				  & RME96_RCR_AUDIO_ADDR_MASK;
+	rme96->capture_pointer = readl(rme96->iobase + RME96_IO_GET_REC_POS)
+				 & RME96_RCR_AUDIO_ADDR_MASK;
+
+	/* save playback and capture buffers */
+	memcpy_fromio(rme96->playback_suspend_buffer,
+		      rme96->iobase + RME96_IO_PLAY_BUFFER, RME96_BUFFER_SIZE);
+	memcpy_fromio(rme96->capture_suspend_buffer,
+		      rme96->iobase + RME96_IO_REC_BUFFER, RME96_BUFFER_SIZE);
+
+	/* disable the DAC  */
+	rme96->areg &= ~RME96_AR_DAC_EN;
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+
+	pci_disable_device(pci);
+	pci_save_state(pci);
+
+	return 0;
+}
+
+static int
+snd_rme96_resume(struct pci_dev *pci)
+{
+	struct snd_card *card = pci_get_drvdata(pci);
+	struct rme96 *rme96 = card->private_data;
+
+	pci_restore_state(pci);
+	pci_enable_device(pci);
+
+	/* reset playback and record buffer pointers */
+	writel(0, rme96->iobase + RME96_IO_SET_PLAY_POS
+		  + rme96->playback_pointer);
+	writel(0, rme96->iobase + RME96_IO_SET_REC_POS
+		  + rme96->capture_pointer);
+
+	/* restore playback and capture buffers */
+	memcpy_toio(rme96->iobase + RME96_IO_PLAY_BUFFER,
+		    rme96->playback_suspend_buffer, RME96_BUFFER_SIZE);
+	memcpy_toio(rme96->iobase + RME96_IO_REC_BUFFER,
+		    rme96->capture_suspend_buffer, RME96_BUFFER_SIZE);
+
+	/* reset the ADC */
+	writel(rme96->areg | RME96_AR_PD2,
+	       rme96->iobase + RME96_IO_ADDITIONAL_REG);
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+
+	/* reset and enable DAC, restore analog volume */
+	snd_rme96_reset_dac(rme96);
+	rme96->areg |= RME96_AR_DAC_EN;
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+	if (RME96_HAS_ANALOG_OUT(rme96)) {
+		usleep_range(3000, 10000);
+		snd_rme96_apply_dac_volume(rme96);
+	}
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
+
+	return 0;
+}
+
+#endif
+
 static void snd_rme96_card_free(struct snd_card *card)
 {
 	snd_rme96_free(card->private_data);
@@ -2390,6 +2486,21 @@ snd_rme96_probe(struct pci_dev *pci,
 		return err;
 	}
 	
+#ifdef CONFIG_PM
+	rme96->playback_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
+	if (!rme96->playback_suspend_buffer) {
+		snd_printk(KERN_ERR
+			   "Failed to allocate playback suspend buffer!\n");
+		return -ENOMEM;
+	}
+	rme96->capture_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
+	if (!rme96->capture_suspend_buffer) {
+		snd_printk(KERN_ERR
+			   "Failed to allocate capture suspend buffer!\n");
+		return -ENOMEM;
+	}
+#endif
+
 	strcpy(card->driver, "Digi96");
 	switch (rme96->pci->device) {
 	case PCI_DEVICE_ID_RME_DIGI96:
@@ -2433,6 +2544,10 @@ static struct pci_driver rme96_driver = {
 	.id_table = snd_rme96_ids,
 	.probe = snd_rme96_probe,
 	.remove = snd_rme96_remove,
+#ifdef CONFIG_PM
+	.suspend = snd_rme96_suspend,
+	.resume = snd_rme96_resume,
+#endif
 };
 
 module_pci_driver(rme96_driver);
-- 
1.8.1.4


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

Re: [PATCH] rme96 add stream synchronization and PM support

[Takashi Iwai]

At Tue, 13 Aug 2013 23:12:05 +0200,
Knut Petersen wrote:
> 
> @@ -2390,6 +2486,21 @@ snd_rme96_probe(struct pci_dev *pci,
>  		return err;
>  	}
>  	
> +#ifdef CONFIG_PM
> +	rme96->playback_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
> +	if (!rme96->playback_suspend_buffer) {
> +		snd_printk(KERN_ERR
> +			   "Failed to allocate playback suspend buffer!\n");
> +		return -ENOMEM;

Call snd_card_free().  Otherwise leaking.

> +	}
> +	rme96->capture_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
> +	if (!rme96->capture_suspend_buffer) {
> +		snd_printk(KERN_ERR
> +			   "Failed to allocate capture suspend buffer!\n");
> +		return -ENOMEM;

Ditto.


The first patch looks OK, so applied it.
Please resubmit only the second patch with the fixes.


thanks,

Takashi

Re: [PATCH] rme96 add stream synchronization and PM support

[Knut Petersen]

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

On 14.08.2013 17:06, Takashi Iwai wrote:
> Please resubmit only the second patch with the fixes.

Ah, yes. That should have been obvious.

After a 2nd thought I believe that  it is an even better idea
not to abort but to handle that error condition graciously.

cu,
  Knut

[-- Attachment #2: 0001-alsa-rme96-Add-PM-support-v2.patch --]
[-- Type: text/x-patch, Size: 7629 bytes --]

>From b0d42bb60a11ba82d26850a4a2ed0aad89d7df79 Mon Sep 17 00:00:00 2001
From: Knut Petersen <Knut_Petersen@t-online.de>
Date: Thu, 15 Aug 2013 07:39:36 +0200
Subject: [PATCH] alsa/rme96: Add PM support v2

Without proper power management handling, the first use
of a Digi96/8 anytime after a suspend / resume cycle will
start playback with distortions.

v2: If vmalloc() of playback and/or capture suspend buffer
fails, do not abort but snd_printk() a warning. Later on,
during resume, zero out hardware buffers if restoring is
impossible beause of unavailable suspend buffers.

Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
---
 sound/pci/rme96.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index 3eb0bdd..73934e9 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -239,6 +239,13 @@ struct rme96 {
 
 	u8 rev; /* card revision number */
 
+#ifdef CONFIG_PM
+	u32 playback_pointer;
+	u32 capture_pointer;
+	void *playback_suspend_buffer;
+	void *capture_suspend_buffer;
+#endif
+
 	struct snd_pcm_substream *playback_substream;
 	struct snd_pcm_substream *capture_substream;
 
@@ -370,6 +377,7 @@ static struct snd_pcm_hardware snd_rme96_playback_spdif_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -400,6 +408,7 @@ static struct snd_pcm_hardware snd_rme96_capture_spdif_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -430,6 +439,7 @@ static struct snd_pcm_hardware snd_rme96_playback_adat_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -456,6 +466,7 @@ static struct snd_pcm_hardware snd_rme96_capture_adat_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -1386,6 +1397,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 		}
 		break;
 
+	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 		if (RME96_ISPLAYING(rme96)) {
 			if (substream != rme96->playback_substream)
@@ -1401,6 +1413,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 						 : RME96_STOP_PLAYBACK);
 		break;
 
+	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (!RME96_ISPLAYING(rme96))
 			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
@@ -1441,6 +1454,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 		}
 		break;
 
+	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 		if (RME96_ISRECORDING(rme96)) {
 			if (substream != rme96->capture_substream)
@@ -1456,6 +1470,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 						 : RME96_STOP_CAPTURE);
 		break;
 
+	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (!RME96_ISRECORDING(rme96))
 			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
@@ -1556,6 +1571,10 @@ snd_rme96_free(void *private_data)
 		pci_release_regions(rme96->pci);
 		rme96->port = 0;
 	}
+#ifdef CONFIG_PM
+	vfree(rme96->playback_suspend_buffer);
+	vfree(rme96->capture_suspend_buffer);
+#endif
 	pci_disable_device(rme96->pci);
 }
 
@@ -2354,6 +2373,95 @@ snd_rme96_create_switches(struct snd_card *card,
  * Card initialisation
  */
 
+#ifdef CONFIG_PM
+
+static int
+snd_rme96_suspend(struct pci_dev *pci,
+		  pm_message_t state)
+{
+	struct snd_card *card = pci_get_drvdata(pci);
+	struct rme96 *rme96 = card->private_data;
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
+	snd_pcm_suspend(rme96->playback_substream);
+	snd_pcm_suspend(rme96->capture_substream);
+
+	/* save capture & playback pointers */
+	rme96->playback_pointer = readl(rme96->iobase + RME96_IO_GET_PLAY_POS)
+				  & RME96_RCR_AUDIO_ADDR_MASK;
+	rme96->capture_pointer = readl(rme96->iobase + RME96_IO_GET_REC_POS)
+				 & RME96_RCR_AUDIO_ADDR_MASK;
+
+	/* save playback and capture buffers */
+	if (rme96->playback_suspend_buffer)
+		memcpy_fromio(rme96->playback_suspend_buffer,
+			      rme96->iobase + RME96_IO_PLAY_BUFFER,
+			      RME96_BUFFER_SIZE);
+	if (rme96->capture_suspend_buffer)
+		memcpy_fromio(rme96->capture_suspend_buffer,
+			      rme96->iobase + RME96_IO_REC_BUFFER,
+			      RME96_BUFFER_SIZE);
+
+	/* disable the DAC  */
+	rme96->areg &= ~RME96_AR_DAC_EN;
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+
+	pci_disable_device(pci);
+	pci_save_state(pci);
+
+	return 0;
+}
+
+static int
+snd_rme96_resume(struct pci_dev *pci)
+{
+	struct snd_card *card = pci_get_drvdata(pci);
+	struct rme96 *rme96 = card->private_data;
+
+	pci_restore_state(pci);
+	pci_enable_device(pci);
+
+	/* reset playback and record buffer pointers */
+	writel(0, rme96->iobase + RME96_IO_SET_PLAY_POS
+		  + rme96->playback_pointer);
+	writel(0, rme96->iobase + RME96_IO_SET_REC_POS
+		  + rme96->capture_pointer);
+
+	/* restore playback and capture buffers */
+	if (rme96->playback_suspend_buffer)
+		memcpy_toio(rme96->iobase + RME96_IO_PLAY_BUFFER,
+			    rme96->playback_suspend_buffer, RME96_BUFFER_SIZE);
+	else
+		memset_io(rme96->iobase + RME96_IO_PLAY_BUFFER,
+			  0, RME96_BUFFER_SIZE);
+	if (rme96->capture_suspend_buffer)
+		memcpy_toio(rme96->iobase + RME96_IO_REC_BUFFER,
+			    rme96->capture_suspend_buffer, RME96_BUFFER_SIZE);
+	else
+		memset_io(rme96->iobase + RME96_IO_REC_BUFFER,
+			  0, RME96_BUFFER_SIZE);
+
+	/* reset the ADC */
+	writel(rme96->areg | RME96_AR_PD2,
+	       rme96->iobase + RME96_IO_ADDITIONAL_REG);
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+
+	/* reset and enable DAC, restore analog volume */
+	snd_rme96_reset_dac(rme96);
+	rme96->areg |= RME96_AR_DAC_EN;
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+	if (RME96_HAS_ANALOG_OUT(rme96)) {
+		usleep_range(3000, 10000);
+		snd_rme96_apply_dac_volume(rme96);
+	}
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
+
+	return 0;
+}
+
+#endif
+
 static void snd_rme96_card_free(struct snd_card *card)
 {
 	snd_rme96_free(card->private_data);
@@ -2390,6 +2498,15 @@ snd_rme96_probe(struct pci_dev *pci,
 		return err;
 	}
 	
+#ifdef CONFIG_PM
+	rme96->playback_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
+	if (!rme96->playback_suspend_buffer)
+		snd_printk(KERN_WARNING "no playback suspend buffer\n");
+	rme96->capture_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
+	if (!rme96->capture_suspend_buffer)
+		snd_printk(KERN_WARNING "no capture suspend buffer\n");
+#endif
+
 	strcpy(card->driver, "Digi96");
 	switch (rme96->pci->device) {
 	case PCI_DEVICE_ID_RME_DIGI96:
@@ -2433,6 +2550,10 @@ static struct pci_driver rme96_driver = {
 	.id_table = snd_rme96_ids,
 	.probe = snd_rme96_probe,
 	.remove = snd_rme96_remove,
+#ifdef CONFIG_PM
+	.suspend = snd_rme96_suspend,
+	.resume = snd_rme96_resume,
+#endif
 };
 
 module_pci_driver(rme96_driver);
-- 
1.8.1.4


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

Re: [PATCH] rme96 add stream synchronization and PM support

[Takashi Iwai]

At Thu, 15 Aug 2013 08:01:05 +0200,
Knut Petersen wrote:
> 
> On 14.08.2013 17:06, Takashi Iwai wrote:
> > Please resubmit only the second patch with the fixes.
> 
> Ah, yes. That should have been obvious.
> 
> After a 2nd thought I believe that  it is an even better idea
> not to abort but to handle that error condition graciously.

Why?  It's a serious error.


Takashi

Re: [PATCH] rme96 add stream synchronization and PM support

[Knut Petersen]

On 15.08.2013 08:22, Takashi Iwai wrote:
> At Thu, 15 Aug 2013 08:01:05 +0200,
> Knut Petersen wrote:
>> On 14.08.2013 17:06, Takashi Iwai wrote:
>>> Please resubmit only the second patch with the fixes.
>> Ah, yes. That should have been obvious.
>>
>> After a 2nd thought I believe that  it is an even better idea
>> not to abort but to handle that error condition graciously.
> Why?  It's a serious error.

The probability that two 64k vmallocs will fail is pretty low,
and the probability that after that failure there is enough memory
for an audio application to be executed successfully is even lower.

Nevertheless, in that unlikely case I would prefer max 0.37 seconds
of silence (=skipped audio) during resume to a failed start of a
recording caused by an unloaded module. Limited functionality
is better than complete failure.


cu,
  Knut

Re: [PATCH] rme96 add stream synchronization and PM support

[Takashi Iwai]

At Thu, 15 Aug 2013 09:16:57 +0200,
Knut Petersen wrote:
> 
> On 15.08.2013 08:22, Takashi Iwai wrote:
> > At Thu, 15 Aug 2013 08:01:05 +0200,
> > Knut Petersen wrote:
> >> On 14.08.2013 17:06, Takashi Iwai wrote:
> >>> Please resubmit only the second patch with the fixes.
> >> Ah, yes. That should have been obvious.
> >>
> >> After a 2nd thought I believe that  it is an even better idea
> >> not to abort but to handle that error condition graciously.
> > Why?  It's a serious error.
> 
> The probability that two 64k vmallocs will fail is pretty low,
> and the probability that after that failure there is enough memory
> for an audio application to be executed successfully is even lower.

Yes, then why you must continue the driver probing even though it's
not fully functional in such an extremely bad situation?  If the
driver is critically important for the further operation, it'd make
sense.  But it's not the case.

> Nevertheless, in that unlikely case I would prefer max 0.37 seconds
> of silence (=skipped audio) during resume to a failed start of a
> recording caused by an unloaded module. Limited functionality
> is better than complete failure.

Even if so, your patch is still missing the handling of failure case.
Without the backup buffers, the driver cannot provide the full resume
functionality, but it still advertises as if it can.  It'd make the
suspend code conditional, setting the RESUME info flag dynamically,
etc.

It'd be much simpler to just let the driver aborting the problem in
the error of memory allocations.


Takashi

Re: [PATCH] rme96 add stream synchronization and PM support

[Knut Petersen]

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

On 15.08.2013 10:37, Takashi Iwai wrote:
> It'd be much simpler to just let the driver aborting the problem in the error of memory allocations.

Ok, so here is v3:

- Add PM,
- abort modprobe if vmalloc() of suspend buffers fail,
- do not leak memory in that case.

cu,
  Knut

[-- Attachment #2: 0001-alsa-rme96-Add-PM-support-v3.patch --]
[-- Type: text/x-patch, Size: 7319 bytes --]

>From d26114c1dc66c1396055f5cd44150fa66604a739 Mon Sep 17 00:00:00 2001
From: Knut Petersen <Knut_Petersen@t-online.de>
Date: Wed, 21 Aug 2013 09:18:54 +0200
Subject: [PATCH] alsa/rme96: Add PM support v3

Without proper power management handling, the first use
of a Digi96/8 anytime after a suspend / resume cycle will
start playback with distortions.

v3: Abort if vmalloc() of suspend buffers fail, but do not
leak memory in that case.

Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
---
 sound/pci/rme96.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
index 3eb0bdd..2acf565 100644
--- a/sound/pci/rme96.c
+++ b/sound/pci/rme96.c
@@ -239,6 +239,13 @@ struct rme96 {
 
 	u8 rev; /* card revision number */
 
+#ifdef CONFIG_PM
+	u32 playback_pointer;
+	u32 capture_pointer;
+	void *playback_suspend_buffer;
+	void *capture_suspend_buffer;
+#endif
+
 	struct snd_pcm_substream *playback_substream;
 	struct snd_pcm_substream *capture_substream;
 
@@ -370,6 +377,7 @@ static struct snd_pcm_hardware snd_rme96_playback_spdif_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -400,6 +408,7 @@ static struct snd_pcm_hardware snd_rme96_capture_spdif_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -430,6 +439,7 @@ static struct snd_pcm_hardware snd_rme96_playback_adat_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -456,6 +466,7 @@ static struct snd_pcm_hardware snd_rme96_capture_adat_info =
 	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
 			      SNDRV_PCM_INFO_MMAP_VALID |
 			      SNDRV_PCM_INFO_SYNC_START |
+			      SNDRV_PCM_INFO_RESUME |
 			      SNDRV_PCM_INFO_INTERLEAVED |
 			      SNDRV_PCM_INFO_PAUSE),
 	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
@@ -1386,6 +1397,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 		}
 		break;
 
+	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 		if (RME96_ISPLAYING(rme96)) {
 			if (substream != rme96->playback_substream)
@@ -1401,6 +1413,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
 						 : RME96_STOP_PLAYBACK);
 		break;
 
+	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (!RME96_ISPLAYING(rme96))
 			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
@@ -1441,6 +1454,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 		}
 		break;
 
+	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 		if (RME96_ISRECORDING(rme96)) {
 			if (substream != rme96->capture_substream)
@@ -1456,6 +1470,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
 						 : RME96_STOP_CAPTURE);
 		break;
 
+	case SNDRV_PCM_TRIGGER_RESUME:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (!RME96_ISRECORDING(rme96))
 			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
@@ -1556,6 +1571,10 @@ snd_rme96_free(void *private_data)
 		pci_release_regions(rme96->pci);
 		rme96->port = 0;
 	}
+#ifdef CONFIG_PM
+	vfree(rme96->playback_suspend_buffer);
+	vfree(rme96->capture_suspend_buffer);
+#endif
 	pci_disable_device(rme96->pci);
 }
 
@@ -2354,6 +2373,83 @@ snd_rme96_create_switches(struct snd_card *card,
  * Card initialisation
  */
 
+#ifdef CONFIG_PM
+
+static int
+snd_rme96_suspend(struct pci_dev *pci,
+		  pm_message_t state)
+{
+	struct snd_card *card = pci_get_drvdata(pci);
+	struct rme96 *rme96 = card->private_data;
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
+	snd_pcm_suspend(rme96->playback_substream);
+	snd_pcm_suspend(rme96->capture_substream);
+
+	/* save capture & playback pointers */
+	rme96->playback_pointer = readl(rme96->iobase + RME96_IO_GET_PLAY_POS)
+				  & RME96_RCR_AUDIO_ADDR_MASK;
+	rme96->capture_pointer = readl(rme96->iobase + RME96_IO_GET_REC_POS)
+				 & RME96_RCR_AUDIO_ADDR_MASK;
+
+	/* save playback and capture buffers */
+	memcpy_fromio(rme96->playback_suspend_buffer,
+		      rme96->iobase + RME96_IO_PLAY_BUFFER, RME96_BUFFER_SIZE);
+	memcpy_fromio(rme96->capture_suspend_buffer,
+		      rme96->iobase + RME96_IO_REC_BUFFER, RME96_BUFFER_SIZE);
+
+	/* disable the DAC  */
+	rme96->areg &= ~RME96_AR_DAC_EN;
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+
+	pci_disable_device(pci);
+	pci_save_state(pci);
+
+	return 0;
+}
+
+static int
+snd_rme96_resume(struct pci_dev *pci)
+{
+	struct snd_card *card = pci_get_drvdata(pci);
+	struct rme96 *rme96 = card->private_data;
+
+	pci_restore_state(pci);
+	pci_enable_device(pci);
+
+	/* reset playback and record buffer pointers */
+	writel(0, rme96->iobase + RME96_IO_SET_PLAY_POS
+		  + rme96->playback_pointer);
+	writel(0, rme96->iobase + RME96_IO_SET_REC_POS
+		  + rme96->capture_pointer);
+
+	/* restore playback and capture buffers */
+	memcpy_toio(rme96->iobase + RME96_IO_PLAY_BUFFER,
+		    rme96->playback_suspend_buffer, RME96_BUFFER_SIZE);
+	memcpy_toio(rme96->iobase + RME96_IO_REC_BUFFER,
+		    rme96->capture_suspend_buffer, RME96_BUFFER_SIZE);
+
+	/* reset the ADC */
+	writel(rme96->areg | RME96_AR_PD2,
+	       rme96->iobase + RME96_IO_ADDITIONAL_REG);
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+
+	/* reset and enable DAC, restore analog volume */
+	snd_rme96_reset_dac(rme96);
+	rme96->areg |= RME96_AR_DAC_EN;
+	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
+	if (RME96_HAS_ANALOG_OUT(rme96)) {
+		usleep_range(3000, 10000);
+		snd_rme96_apply_dac_volume(rme96);
+	}
+
+	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
+
+	return 0;
+}
+
+#endif
+
 static void snd_rme96_card_free(struct snd_card *card)
 {
 	snd_rme96_free(card->private_data);
@@ -2390,6 +2486,24 @@ snd_rme96_probe(struct pci_dev *pci,
 		return err;
 	}
 	
+#ifdef CONFIG_PM
+	rme96->playback_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
+	if (!rme96->playback_suspend_buffer) {
+		snd_printk(KERN_ERR
+			   "Failed to allocate playback suspend buffer!\n");
+		snd_card_free(card);
+		return -ENOMEM;
+	}
+	rme96->capture_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
+	if (!rme96->capture_suspend_buffer) {
+		snd_printk(KERN_ERR
+			   "Failed to allocate capture suspend buffer!\n");
+		vfree(rme96->playback_suspend_buffer);
+		snd_card_free(card);
+		return -ENOMEM;
+	}
+#endif
+
 	strcpy(card->driver, "Digi96");
 	switch (rme96->pci->device) {
 	case PCI_DEVICE_ID_RME_DIGI96:
@@ -2433,6 +2547,10 @@ static struct pci_driver rme96_driver = {
 	.id_table = snd_rme96_ids,
 	.probe = snd_rme96_probe,
 	.remove = snd_rme96_remove,
+#ifdef CONFIG_PM
+	.suspend = snd_rme96_suspend,
+	.resume = snd_rme96_resume,
+#endif
 };
 
 module_pci_driver(rme96_driver);
-- 
1.8.1.4


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

Re: [PATCH] rme96 add stream synchronization and PM support

[Takashi Iwai]

At Wed, 21 Aug 2013 09:44:22 +0200,
Knut Petersen wrote:
> 
> [1  <text/plain; ISO-8859-1 (7bit)>]
> On 15.08.2013 10:37, Takashi Iwai wrote:
> > It'd be much simpler to just let the driver aborting the problem in the error of memory allocations.
> 
> Ok, so here is v3:
> 
> - Add PM,
> - abort modprobe if vmalloc() of suspend buffers fail,
> - do not leak memory in that case.

Actually this triggers the double free.  It's freed in
snd_rme96_free() via snd_card_free().

I applied the patch with removal of this double vfree call.


thanks,

Takashi

> 
> cu,
>   Knut
> [2 0001-alsa-rme96-Add-PM-support-v3.patch <text/x-patch (7bit)>]
> >From d26114c1dc66c1396055f5cd44150fa66604a739 Mon Sep 17 00:00:00 2001
> From: Knut Petersen <Knut_Petersen@t-online.de>
> Date: Wed, 21 Aug 2013 09:18:54 +0200
> Subject: [PATCH] alsa/rme96: Add PM support v3
> 
> Without proper power management handling, the first use
> of a Digi96/8 anytime after a suspend / resume cycle will
> start playback with distortions.
> 
> v3: Abort if vmalloc() of suspend buffers fail, but do not
> leak memory in that case.
> 
> Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
> ---
>  sound/pci/rme96.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
> 
> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
> index 3eb0bdd..2acf565 100644
> --- a/sound/pci/rme96.c
> +++ b/sound/pci/rme96.c
> @@ -239,6 +239,13 @@ struct rme96 {
>  
>  	u8 rev; /* card revision number */
>  
> +#ifdef CONFIG_PM
> +	u32 playback_pointer;
> +	u32 capture_pointer;
> +	void *playback_suspend_buffer;
> +	void *capture_suspend_buffer;
> +#endif
> +
>  	struct snd_pcm_substream *playback_substream;
>  	struct snd_pcm_substream *capture_substream;
>  
> @@ -370,6 +377,7 @@ static struct snd_pcm_hardware snd_rme96_playback_spdif_info =
>  	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
>  			      SNDRV_PCM_INFO_MMAP_VALID |
>  			      SNDRV_PCM_INFO_SYNC_START |
> +			      SNDRV_PCM_INFO_RESUME |
>  			      SNDRV_PCM_INFO_INTERLEAVED |
>  			      SNDRV_PCM_INFO_PAUSE),
>  	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
> @@ -400,6 +408,7 @@ static struct snd_pcm_hardware snd_rme96_capture_spdif_info =
>  	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
>  			      SNDRV_PCM_INFO_MMAP_VALID |
>  			      SNDRV_PCM_INFO_SYNC_START |
> +			      SNDRV_PCM_INFO_RESUME |
>  			      SNDRV_PCM_INFO_INTERLEAVED |
>  			      SNDRV_PCM_INFO_PAUSE),
>  	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
> @@ -430,6 +439,7 @@ static struct snd_pcm_hardware snd_rme96_playback_adat_info =
>  	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
>  			      SNDRV_PCM_INFO_MMAP_VALID |
>  			      SNDRV_PCM_INFO_SYNC_START |
> +			      SNDRV_PCM_INFO_RESUME |
>  			      SNDRV_PCM_INFO_INTERLEAVED |
>  			      SNDRV_PCM_INFO_PAUSE),
>  	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
> @@ -456,6 +466,7 @@ static struct snd_pcm_hardware snd_rme96_capture_adat_info =
>  	.info =		     (SNDRV_PCM_INFO_MMAP_IOMEM |
>  			      SNDRV_PCM_INFO_MMAP_VALID |
>  			      SNDRV_PCM_INFO_SYNC_START |
> +			      SNDRV_PCM_INFO_RESUME |
>  			      SNDRV_PCM_INFO_INTERLEAVED |
>  			      SNDRV_PCM_INFO_PAUSE),
>  	.formats =	     (SNDRV_PCM_FMTBIT_S16_LE |
> @@ -1386,6 +1397,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
>  		}
>  		break;
>  
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_STOP:
>  		if (RME96_ISPLAYING(rme96)) {
>  			if (substream != rme96->playback_substream)
> @@ -1401,6 +1413,7 @@ snd_rme96_playback_trigger(struct snd_pcm_substream *substream,
>  						 : RME96_STOP_PLAYBACK);
>  		break;
>  
> +	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		if (!RME96_ISPLAYING(rme96))
>  			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
> @@ -1441,6 +1454,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
>  		}
>  		break;
>  
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>  	case SNDRV_PCM_TRIGGER_STOP:
>  		if (RME96_ISRECORDING(rme96)) {
>  			if (substream != rme96->capture_substream)
> @@ -1456,6 +1470,7 @@ snd_rme96_capture_trigger(struct snd_pcm_substream *substream,
>  						 : RME96_STOP_CAPTURE);
>  		break;
>  
> +	case SNDRV_PCM_TRIGGER_RESUME:
>  	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		if (!RME96_ISRECORDING(rme96))
>  			snd_rme96_trigger(rme96, sync ? RME96_RESUME_BOTH
> @@ -1556,6 +1571,10 @@ snd_rme96_free(void *private_data)
>  		pci_release_regions(rme96->pci);
>  		rme96->port = 0;
>  	}
> +#ifdef CONFIG_PM
> +	vfree(rme96->playback_suspend_buffer);
> +	vfree(rme96->capture_suspend_buffer);
> +#endif
>  	pci_disable_device(rme96->pci);
>  }
>  
> @@ -2354,6 +2373,83 @@ snd_rme96_create_switches(struct snd_card *card,
>   * Card initialisation
>   */
>  
> +#ifdef CONFIG_PM
> +
> +static int
> +snd_rme96_suspend(struct pci_dev *pci,
> +		  pm_message_t state)
> +{
> +	struct snd_card *card = pci_get_drvdata(pci);
> +	struct rme96 *rme96 = card->private_data;
> +
> +	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
> +	snd_pcm_suspend(rme96->playback_substream);
> +	snd_pcm_suspend(rme96->capture_substream);
> +
> +	/* save capture & playback pointers */
> +	rme96->playback_pointer = readl(rme96->iobase + RME96_IO_GET_PLAY_POS)
> +				  & RME96_RCR_AUDIO_ADDR_MASK;
> +	rme96->capture_pointer = readl(rme96->iobase + RME96_IO_GET_REC_POS)
> +				 & RME96_RCR_AUDIO_ADDR_MASK;
> +
> +	/* save playback and capture buffers */
> +	memcpy_fromio(rme96->playback_suspend_buffer,
> +		      rme96->iobase + RME96_IO_PLAY_BUFFER, RME96_BUFFER_SIZE);
> +	memcpy_fromio(rme96->capture_suspend_buffer,
> +		      rme96->iobase + RME96_IO_REC_BUFFER, RME96_BUFFER_SIZE);
> +
> +	/* disable the DAC  */
> +	rme96->areg &= ~RME96_AR_DAC_EN;
> +	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
> +
> +	pci_disable_device(pci);
> +	pci_save_state(pci);
> +
> +	return 0;
> +}
> +
> +static int
> +snd_rme96_resume(struct pci_dev *pci)
> +{
> +	struct snd_card *card = pci_get_drvdata(pci);
> +	struct rme96 *rme96 = card->private_data;
> +
> +	pci_restore_state(pci);
> +	pci_enable_device(pci);
> +
> +	/* reset playback and record buffer pointers */
> +	writel(0, rme96->iobase + RME96_IO_SET_PLAY_POS
> +		  + rme96->playback_pointer);
> +	writel(0, rme96->iobase + RME96_IO_SET_REC_POS
> +		  + rme96->capture_pointer);
> +
> +	/* restore playback and capture buffers */
> +	memcpy_toio(rme96->iobase + RME96_IO_PLAY_BUFFER,
> +		    rme96->playback_suspend_buffer, RME96_BUFFER_SIZE);
> +	memcpy_toio(rme96->iobase + RME96_IO_REC_BUFFER,
> +		    rme96->capture_suspend_buffer, RME96_BUFFER_SIZE);
> +
> +	/* reset the ADC */
> +	writel(rme96->areg | RME96_AR_PD2,
> +	       rme96->iobase + RME96_IO_ADDITIONAL_REG);
> +	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
> +
> +	/* reset and enable DAC, restore analog volume */
> +	snd_rme96_reset_dac(rme96);
> +	rme96->areg |= RME96_AR_DAC_EN;
> +	writel(rme96->areg, rme96->iobase + RME96_IO_ADDITIONAL_REG);
> +	if (RME96_HAS_ANALOG_OUT(rme96)) {
> +		usleep_range(3000, 10000);
> +		snd_rme96_apply_dac_volume(rme96);
> +	}
> +
> +	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
> +
> +	return 0;
> +}
> +
> +#endif
> +
>  static void snd_rme96_card_free(struct snd_card *card)
>  {
>  	snd_rme96_free(card->private_data);
> @@ -2390,6 +2486,24 @@ snd_rme96_probe(struct pci_dev *pci,
>  		return err;
>  	}
>  	
> +#ifdef CONFIG_PM
> +	rme96->playback_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
> +	if (!rme96->playback_suspend_buffer) {
> +		snd_printk(KERN_ERR
> +			   "Failed to allocate playback suspend buffer!\n");
> +		snd_card_free(card);
> +		return -ENOMEM;
> +	}
> +	rme96->capture_suspend_buffer = vmalloc(RME96_BUFFER_SIZE);
> +	if (!rme96->capture_suspend_buffer) {
> +		snd_printk(KERN_ERR
> +			   "Failed to allocate capture suspend buffer!\n");
> +		vfree(rme96->playback_suspend_buffer);
> +		snd_card_free(card);
> +		return -ENOMEM;
> +	}
> +#endif
> +
>  	strcpy(card->driver, "Digi96");
>  	switch (rme96->pci->device) {
>  	case PCI_DEVICE_ID_RME_DIGI96:
> @@ -2433,6 +2547,10 @@ static struct pci_driver rme96_driver = {
>  	.id_table = snd_rme96_ids,
>  	.probe = snd_rme96_probe,
>  	.remove = snd_rme96_remove,
> +#ifdef CONFIG_PM
> +	.suspend = snd_rme96_suspend,
> +	.resume = snd_rme96_resume,
> +#endif
>  };
>  
>  module_pci_driver(rme96_driver);
> -- 
> 1.8.1.4
> 

Re: [PATCH] rme96 Add missing vmalloc.h inclusion

[Takashi Iwai]

At Thu, 22 Aug 2013 15:06:38 +0200,
Knut Petersen wrote:
> 
> [1  <text/plain; ISO-8859-1 (7bit)>]
> On 22.08.2013 10:53, Takashi Iwai wrote:
> > Actually this triggers the double free.  It's freed in
> > snd_rme96_free() via snd_card_free().
> Yes. Now I see that.
> > I applied the patch with removal of this double vfree call.
> Thanks.
> 
> Unfortunately there still is a problem with the code:
> vmalloc() and vfree() were used without inclusion
> of linux/vmalloc.h.

Indeed.  Applied now.  Thanks.


Takashi

> 
> cu,
>   Knut
> 
> 
> [2 0001-alsa-rme96-Add-missing-inclusion-of-linux-vmalloc.h.patch <text/x-patch (7bit)>]
> >From 2a3f98ff69308f3c2ee508ac0887e1f8bf493d64 Mon Sep 17 00:00:00 2001
> From: Knut Petersen <Knut_Petersen@t-online.de>
> Date: Thu, 22 Aug 2013 14:36:16 +0200
> Subject: [PATCH] alsa/rme96: Add missing inclusion of linux/vmalloc.h
> 
> Signed-off-by: Knut Petersen <Knut_Petersen@t-online.de>
> ---
>  sound/pci/rme96.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c
> index 2acf565..cc6deca 100644
> --- a/sound/pci/rme96.c
> +++ b/sound/pci/rme96.c
> @@ -28,6 +28,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/pci.h>
>  #include <linux/module.h>
> +#include <linux/vmalloc.h>
>  
>  #include <sound/core.h>
>  #include <sound/info.h>
> -- 
> 1.8.1.4
>