Re: [Patch 1/1] V4L (926) Saa7134 alsa can only be autoloaded after saa7134 is active

[Takashi Iwai <tiwai@suse.de>]

At Mon, 07 Nov 2005 18:48:18 -0200,
mchehab@brturbo.com.br wrote:
> 
> From: Ricardo Cerqueira <v4l@cerqueira.org>
> 
> - Saa7134-alsa can only be autoloaded after saa7134 is active
> - Applied pertinent changes proposed by the ALSA team
> - dsp_nr replaced by ALSA's index[]
> 
> Signed-off-by: Ricardo Cerqueira <v4l@cerqueira.org>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@brturbo.com.br>

Thanks for the quick rewrite!


> --- hg.old.orig/drivers/media/video/saa7134/saa7134-alsa.c
> +++ hg.old/drivers/media/video/saa7134/saa7134-alsa.c
> @@ -186,30 +171,31 @@ void saa7134_irq_alsa_done(struct saa713
>  		goto done;
>  	}
>  
> -	if (dev->oss.read_count >= dev->oss.blksize * (dev->oss.blocks-2)) {
> -		dprintk("irq: overrun [full=%d/%d] - Blocks in %d\n",dev->oss.read_count,
> -			dev->oss.bufsize, dev->oss.blocks);
> +	if (dev->dmasound.read_count >= dev->dmasound.blksize * (dev->dmasound.blocks-2)) {
> +		dprintk("irq: overrun [full=%d/%d] - Blocks in %d\n",dev->dmasound.read_count,
> +			dev->dmasound.bufsize, dev->dmasound.blocks);
> +		snd_pcm_stop(dev->dmasound.substream,SNDRV_PCM_STATE_XRUN);
>  		saa7134_dma_stop(dev);
>  		goto done;
>  	}

You need spin_unlock(&dev->slock) before calling snd_pcm_stop() since
this lock is used in trigger callback, too.  Note that "goto done" may
require the lock again.  Also, the succeeding saa7134_dma_stop() is
superfluous.


> @@ -374,13 +376,13 @@ static int snd_card_saa7134_capture_prep
>  		goto fail2;
>  
>  	/* prepare buffer */
> -	if (0 != (err = videobuf_dma_pci_map(dev->pci,&dev->oss.dma)))
> +	if (0 != (err = videobuf_dma_pci_map(dev->pci,&dev->dmasound.dma)))
>  		return err;
> -	if (0 != (err = saa7134_pgtable_alloc(dev->pci,&dev->oss.pt)))
> +	if (0 != (err = saa7134_pgtable_alloc(dev->pci,&dev->dmasound.pt)))
>  		goto fail1;
> -	if (0 != (err = saa7134_pgtable_build(dev->pci,&dev->oss.pt,
> -					      dev->oss.dma.sglist,
> -					      dev->oss.dma.sglen,
> +	if (0 != (err = saa7134_pgtable_build(dev->pci,&dev->dmasound.pt,
> +					      dev->dmasound.dma.sglist,
> +					      dev->dmasound.dma.sglen,
>  					      0)))
>  		goto fail2;


The buffer allocation would be better to be done in hw_params
callback because prepare callback can be frequently called (although 
it works in prepare callback, too).
Or, at least, you shoud cache the value in the same stream.


> @@ -818,7 +811,7 @@ static int snd_saa7134_capsrc_put(snd_kc
>  		 chip->capture_source[addr][1] != right;
>  	chip->capture_source[addr][0] = left;
>  	chip->capture_source[addr][1] = right;
> -	dev->oss.input=addr;
> +	dev->dmasound.input=addr;
>  	spin_unlock_irqrestore(&chip->mixer_lock, flags);

You can use spin_unlock_irq() safely here.


> @@ -973,18 +963,17 @@ int alsa_card_saa7134_create (struct saa
>  	chip->irq = saadev->pci->irq;
>  	chip->iobase = pci_resource_start(saadev->pci, 0);
>  
> -	err = request_irq(chip->pci->irq, saa7134_alsa_irq,
> +	err = request_irq(saadev->pci->irq, saa7134_alsa_irq,
>  				SA_SHIRQ | SA_INTERRUPT, saadev->name, saadev);

Hmm, still I don't see any call of free_irq() in the driver?


> @@ -1010,7 +997,7 @@ int alsa_card_saa7134_create (struct saa
>  
>  __nodev:
>  	snd_card_free(card);
> -	kfree(card);
> +	kfree(chip);

It's not safe to call kfree(chip) here, too.  This can be already
released, once after snd_device_new() is succeeded.
In your case, it's easier to use the following style:

	err = snd_card_new(index[dev], id[dev], THIS_MODULE,
				sizeof(snd_card_saa7134_t));
	chip = card->private_data;
	card->private_free = snd_saa7134_free;

so that no extra kfree() is needed.


Takashi