From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: Re: + v4l-720-alsa-support-for-saa7134-that-should-work-fix.patch added to -mm tree Date: Mon, 07 Nov 2005 16:59:20 +0100 Message-ID: References: <200511060743.jA67hpZa018948@shell0.pdx.osdl.net> <20051106001249.48d3ade0.akpm@osdl.org> <1131301995.13599.5.camel@mindpipe> <1131345733.10094.17.camel@localhost> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Sender: alsa-devel-admin@lists.sourceforge.net Errors-To: alsa-devel-admin@lists.sourceforge.net List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , List-Archive: To: Mauro Carvalho Chehab Cc: alsa-devel@lists.sourceforge.net List-Id: alsa-devel@alsa-project.org [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: >=20 > Lee, >=20 > Em Dom, 2005-11-06 =E0s 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 wir= ed > > > up into the build system - it simply doesn't get compiled. > > >=20 > > > Please send a fix against next -mm?=20 > >=20 > > Also please send all ALSA related patches to > > alsa-devel@lists.sourceforge.net for review. >=20 > I'm sending you enclosed saa7134-alsa patch. To make easier to > understand, I've merged all stuff. This is highly dependent of the othe= r > 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 The driver doesn't support any rawmidi function, no? > +unsigned int dsp_nr =3D 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_B= E | 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] =3D SNDRV_DEFAULT_PT= R; Simply don't set anything to initialize NULLs. > +void saa7134_irq_alsa_done(struct saa7134_dev *dev, unsigned long stat= us) > +{ (snip) > + if (dev->oss.read_count >=3D dev->oss.blksize * (dev->oss.blocks-2)) = { > + dprintk("irq: overrun [full=3D%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 * subs= tream, > + 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 * subs= tream) (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 =3D > +{ > + .info =3D (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTER= LEAVED | > + SNDRV_PCM_INFO_BLOCK_TRANSFER | > + SNDRV_PCM_INFO_MMAP_VALID), > + .formats =3D USE_FORMATS, > + .rates =3D USE_RATE, > + .rate_min =3D USE_RATE_MIN, > + .rate_max =3D USE_RATE_MAX, > + .channels_min =3D USE_CHANNELS_MIN, > + .channels_max =3D USE_CHANNELS_MAX, > + .buffer_bytes_max =3D MAX_BUFFER_SIZE, > + .period_bytes_min =3D 64, > + .period_bytes_max =3D MAX_BUFFER_SIZE, > + .periods_min =3D USE_PERIODS_MIN, > + .periods_max =3D USE_PERIODS_MAX, > + .fifo_size =3D 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 * substr= eam) > +{ > + snd_card_saa7134_t *chip =3D snd_pcm_substream_chip(substream); > + struct saa7134_dev *dev =3D 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 * substre= am) (snip) > + saapcm =3D kcalloc(1, sizeof(*saapcm), GFP_KERNEL); Use kzalloc(). > + if (saapcm =3D=3D NULL) > + return -ENOMEM; > + saapcm->saadev=3Dsaa7134->saadev; > + > + spin_lock_init(&saapcm->lock); > + > + saapcm->substream =3D substream; > + runtime->private_data =3D saapcm; > + runtime->private_free =3D snd_card_saa7134_runtime_free; > + runtime->hw =3D snd_card_saa7134_capture; > + > + if ((err =3D snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARA= M_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_e= lem_value_t * ucontrol) > +{ > + snd_card_saa7134_t *chip =3D snd_kcontrol_chip(kcontrol); > + unsigned long flags; > + int addr =3D kcontrol->private_value; > + > + spin_lock_irqsave(&chip->mixer_lock, flags); > + ucontrol->value.integer.value[0] =3D chip->mixer_volume[addr][0]; > + ucontrol->value.integer.value[1] =3D 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 =3D device->device_data; > + return snd_saa7134_free(chip); > +} > + > +int alsa_card_saa7134_create (struct saa7134_dev *saadev, unsigned int= devicenum) (snip) > + if (devicenum) { > + card =3D snd_card_new(devicenum, id[dev], THIS_MODULE, 0); > + dsp_nr++; > + } else { > + card =3D snd_card_new(index[dev], id[dev], THIS_MODULE, 0); > + } It's weird. Any problem to use index[] only? > + err =3D 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 =3D 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 =3D snd_card_register(card)) =3D=3D 0) { > + snd_saa7134_cards[dev] =3D 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