Re: Re: + v4l-720-alsa-support-for-saa7134-that-should-work-fix.patch added to -mm tree

[Takashi Iwai <tiwai@suse.de>]

[It seems my previous post wasn't delivered by some reason.
 If you already received this, please disregard.]

At Mon, 07 Nov 2005 04:42:13 -0200,
Mauro Carvalho Chehab wrote:
> 
> Lee,
> 
> Em Dom, 2005-11-06 às 13:33 -0500, Lee Revell escreveu:
> > On Sun, 2005-11-06 at 00:12 -0800, Andrew Morton wrote:
> > > Well that didn't work.  The problem is that
> > > drivers/media/video/saa7134/saa7134-alsa.c doesn't appear to be wired
> > > up into the build system - it simply doesn't get compiled.
> > > 
> > > Please send a fix against next -mm? 
> > 
> > Also please send all ALSA related patches to
> > alsa-devel@lists.sourceforge.net for review.
> 
> 	I'm sending you enclosed saa7134-alsa patch. To make easier to
> understand, I've merged all stuff. This is highly dependent of the other
> saa7134 parts, since PCI stuff are common to both video and audio
> funcion on this device.
> 	This is meant to replace saa7134-oss (after more tests) that,
> currently, is part of saa7134 module.

Thanks!
How about Kconfig?  Do you select SND or it depends on SND?

The below are some nitpicks.


> +#include <sound/rawmidi.h>

The driver doesn't support any rawmidi function, no?


> +unsigned int dsp_nr = 0;
> +module_param(dsp_nr,   int, 0444);
> +MODULE_PARM_DESC(dsp_nr,   "alsa device number");

This is what index[] for.

> +/*
> + * Configuration macros
> + */
> +
> +/* defaults */
> +#define MAX_BUFFER_SIZE		(256*1024)
> +#define USE_FORMATS 		SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE | SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE

Better to fold lines :)

> +static snd_card_t *snd_saa7134_cards[SNDRV_CARDS] = SNDRV_DEFAULT_PTR;

Simply don't set anything to initialize NULLs.


> +void saa7134_irq_alsa_done(struct saa7134_dev *dev, unsigned long status)
> +{
(snip)
> +	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);
> +		saa7134_dma_stop(dev);

This may bring the PCM state inconsistent.
In such a case, you can call snd_pcm_stop(substream,
SNDRV_PCM_STATE_XRUN).  But call it outside the spinlock.  Otherwise
you'll get deadlock.


> +/*
> + * ALSA capture trigger
> + *
> + *   - One of the ALSA capture callbacks.
> + *
> + *   Called whenever a capture is started or stopped. Must be defined,
> + *   but there's nothing we want to do here
> + *
> + */
> +
> +static int snd_card_saa7134_capture_trigger(snd_pcm_substream_t * substream,
> +					  int cmd)
> +{
> +	return 0;
> +}

Hmm, dma_start and dma_stop should be called from trigger callbacks.
Calling it from prepare is a wrong implementation.

> +static int snd_card_saa7134_capture_prepare(snd_pcm_substream_t * substream)
(snip)
> +	/* start dma */
> +	spin_lock_irqsave(&dev->slock,flags);
> +	saa7134_dma_start(dev);
> +	spin_unlock_irqrestore(&dev->slock,flags);

You don't need irqsave in prepare callback, use spin_lock_irq(),
instead.  But, this part should be anyway in trigger callback...

> +static snd_pcm_hardware_t snd_card_saa7134_capture =
> +{
> +	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED |
> +				 SNDRV_PCM_INFO_BLOCK_TRANSFER |
> +				 SNDRV_PCM_INFO_MMAP_VALID),
> +	.formats =		USE_FORMATS,
> +	.rates =		USE_RATE,
> +	.rate_min =		USE_RATE_MIN,
> +	.rate_max =		USE_RATE_MAX,
> +	.channels_min =		USE_CHANNELS_MIN,
> +	.channels_max =		USE_CHANNELS_MAX,
> +	.buffer_bytes_max =	MAX_BUFFER_SIZE,
> +	.period_bytes_min =	64,
> +	.period_bytes_max =	MAX_BUFFER_SIZE,
> +	.periods_min =		USE_PERIODS_MIN,
> +	.periods_max =		USE_PERIODS_MAX,
> +	.fifo_size =		0x08070503,

fifo_size doesn't play any role in the current version, so leave it
empty.

> +static int snd_card_saa7134_capture_close(snd_pcm_substream_t * substream)
> +{
> +	snd_card_saa7134_t *chip = snd_pcm_substream_chip(substream);
> +	struct saa7134_dev *dev = chip->saadev;
> +	unsigned long flags;
> +
> +	/* stop dma */
> +	spin_lock_irqsave(&dev->slock,flags);
> +	saa7134_dma_stop(dev);
> +	spin_unlock_irqrestore(&dev->slock,flags);

No need for irqsave, too.


> +static int snd_card_saa7134_capture_open(snd_pcm_substream_t * substream)
(snip)
> +	saapcm = kcalloc(1, sizeof(*saapcm), GFP_KERNEL);

Use kzalloc().

> +	if (saapcm == NULL)
> +		return -ENOMEM;
> +	saapcm->saadev=saa7134->saadev;
> +
> +	spin_lock_init(&saapcm->lock);
> +
> +	saapcm->substream = substream;
> +	runtime->private_data = saapcm;
> +	runtime->private_free = snd_card_saa7134_runtime_free;
> +	runtime->hw = snd_card_saa7134_capture;
> +
> +	if ((err = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS)) < 0)
> +		return err;

You need an error path to release the resource here.


> +
> +static int snd_saa7134_volume_get(snd_kcontrol_t * kcontrol, snd_ctl_elem_value_t * ucontrol)
> +{
> +	snd_card_saa7134_t *chip = snd_kcontrol_chip(kcontrol);
> +	unsigned long flags;
> +	int addr = kcontrol->private_value;
> +
> +	spin_lock_irqsave(&chip->mixer_lock, flags);
> +	ucontrol->value.integer.value[0] = chip->mixer_volume[addr][0];
> +	ucontrol->value.integer.value[1] = chip->mixer_volume[addr][1];
> +	spin_unlock_irqrestore(&chip->mixer_lock, flags);

No need for irqsave in control callbacks.
(Is this spinlock for what?)

> +static int snd_saa7134_free(snd_card_saa7134_t *chip)
> +{
> +	return 0;
> +}

Don't you need release_irq?

> +
> +static int snd_saa7134_dev_free(snd_device_t *device)
> +{
> +	snd_card_saa7134_t *chip = device->device_data;
> +	return snd_saa7134_free(chip);
> +}

> +
> +int alsa_card_saa7134_create (struct saa7134_dev *saadev, unsigned int devicenum)
(snip)
> +	if (devicenum) {
> +		card = snd_card_new(devicenum, id[dev], THIS_MODULE, 0);
> +		dsp_nr++;
> +	} else {
> +		card = snd_card_new(index[dev], id[dev], THIS_MODULE, 0);
> +	}

It's weird.  Any problem to use index[] only?

> +	err = request_irq(chip->pci->irq, saa7134_alsa_irq,
> +				SA_SHIRQ | SA_INTERRUPT, saadev->name, saadev);
> +
> +	if (err < 0) {
> +		printk(KERN_ERR "%s: can't get IRQ %d for ALSA\n",
> +			saadev->name, saadev->pci->irq);

You need snd_card_free() and kfree(chip), here.

> +		return err;
> +	}
> +
> +	if ((err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, chip, &ops)) < 0) {
> +		snd_saa7134_free(chip);

Here, too.

> +	spin_lock_init(&chip->mixer_lock);

You should initialize it beforehand (although it's harmless until
register is called).


> +	snd_card_set_dev(card, &chip->pci->dev);
> +
> +	/* End of "creation" */
> +
> +	strcpy(card->shortname, "SAA7134");
> +	sprintf(card->longname, "%s at 0x%lx irq %d",
> +		chip->saadev->name, chip->iobase, chip->irq);
> +
> +	if ((err = snd_card_register(card)) == 0) {
> +		snd_saa7134_cards[dev] = card;

Incremant dev here.  Otherwise it'll be overridden.

> +		return 0;
> +	}
> +
> +__nodev:
> +	snd_card_free(card);
> +	kfree(card);

Don't kfree.  The object is already released in snd_card_free.


Takashi


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php