Re: [alsa-devel] [PATCH] ALSA AICA sound on SEGA Dreamcast - fix behaviour in poor resource conditions

[Takashi Iwai <tiwai@suse.de>]

At Fri, 6 Jul 2007 10:46:59 +0100,
Adrian McMenamin wrote:
> 
> On 06/07/07, Takashi Iwai <tiwai@suse.de> wrote:
> > At Thu, 5 Jul 2007 23:07:44 +0100,
> > Adrian McMenamin wrote:
> > > @@ -402,17 +396,10 @@
> > >  static int snd_aicapcm_pcm_trigger(struct snd_pcm_substream
> > >                                  *substream, int cmd)
> > >  {
> > > -     struct snd_card_aica *dreamcastcard;
> > >       switch (cmd) {
> > >       case SNDRV_PCM_TRIGGER_START:
> > >               spu_begin_dma(substream);
> > >               break;
> > > -     case SNDRV_PCM_TRIGGER_STOP:
> > > -             dreamcastcard = substream->pcm->private_data;
> > > -             if (dreamcastcard->timer.data)
> > > -                     del_timer(&dreamcastcard->timer);
> > > -             aica_chn_halt();
> > > -             break;
> > >       default:
> > >               return -EINVAL;
> > >       }
> >
> > Is this a correct change?  Then you'd have no control for stopping the
> > stream.
> >
> 
> The shutdown is all done in the close() now, partly because
> snd_aicapcm_pcm_trigger cannot sleep and close() can. Is there a
> pressing reason to put code in snd_aicapcm_pcm_trigger?

I guess you shoud call del_timer() there at least.  Otherwise the
timer handler (aica_period_elapsed()) might be called even if the
stream is not really running.

The below is some coding style fixes, BTW.  I'm not sure whether
queue_work() is intended in the block if "if (dma_check == 0)" or
not.


Takashi

--- aica.c.old	2007-07-06 11:51:18.000000000 +0200
+++ aica.c	2007-07-06 11:52:10.000000000 +0200
@@ -256,7 +256,8 @@
 		buffer_size = frames_to_bytes(runtime, runtime->buffer_size);
 		if (runtime->channels > 1)
 			dreamcastcard->channel->flags |= 0x01;
-		aica_dma_transfer(runtime->channels, buffer_size, dreamcastcard->substream);
+		aica_dma_transfer(runtime->channels, buffer_size,
+				  dreamcastcard->substream);
 		startup_aica(dreamcastcard);
 		dreamcastcard->clicks =
 		    buffer_size / (AICA_PERIOD_SIZE * runtime->channels);
@@ -268,9 +269,7 @@
 		snd_pcm_period_elapsed(dreamcastcard->substream);
 		dreamcastcard->clicks++;
 		if (unlikely(dreamcastcard->clicks >= AICA_PERIOD_NUMBER))
-		{
 			dreamcastcard->clicks %= AICA_PERIOD_NUMBER;
-		}
 		mod_timer(&dreamcastcard->timer, jiffies + 1);
 	}
 }
@@ -300,8 +299,7 @@
 		dreamcastcard->current_period = play_period;
 	if (unlikely(dreamcastcard->dma_check == 0))
 		dreamcastcard->dma_check = 1;
-		queue_work(aica_queue, &(dreamcastcard->spu_dma_work));
-
+	queue_work(aica_queue, &(dreamcastcard->spu_dma_work));
 }
 
 static void spu_begin_dma(struct snd_pcm_substream *substream)
@@ -311,7 +309,7 @@
 	struct snd_pcm_runtime *runtime;
 	runtime = substream->runtime;
 	dreamcastcard = substream->pcm->private_data;
-	//get the queue to do the work
+	/* get the queue to do the work */
 	queue_work(aica_queue, &(dreamcastcard->spu_dma_work));
 	/* Timer may already be running */
 	if (unlikely(dreamcastcard->timer.data)) {
@@ -597,7 +595,7 @@
 	strcpy(dreamcastcard->card->shortname, SND_AICA_DRIVER);
 	strcpy(dreamcastcard->card->longname,
 	       "Yamaha AICA Super Intelligent Sound Processor for SEGA Dreamcast");
-	//start the worker thread
+	/* start the worker thread */
 	INIT_WORK(&(dreamcastcard->spu_dma_work), run_spu_dma);
 	/* Load the PCM 'chip' */
 	err = snd_aicapcmchip(dreamcastcard, 0);
@@ -631,7 +629,8 @@
 	.probe = snd_aica_probe,
 	.remove = snd_aica_remove,
 	.driver = {
-		   .name = SND_AICA_DRIVER},
+		   .name = SND_AICA_DRIVER
+	},
 };
 
 static int __init aica_init(void)