[alsa-devel] [PATCH] Driver for Emagic Audiowerk 2

[Takashi Iwai]

At Tue, 19 Feb 2008 18:11:45 +0100,
Cédric Brégardis wrote:
> 
> At Tuesday 19 February 2008 11:37:09, Takashi Iwai wrote:
> > At Tue, 19 Feb 2008 00:38:54 +0100,
> >
> > Cédric Brégardis wrote:
> > > Signed-off-by: Cedric Bregardis <cedric.bregardis at free.fr>
> > >
> > > Signed-off-by: Jean-Christian Hassler <jhassler at free.fr>
> >
> > Could you avoid HTML mail?  I cannot get the patch properly.
> > If embedding text is difficult, rather use an attachment.
> 
> 
> Oh ! I am really sorry. I don't understand why my mailer has sent this mail in 
> HTML... And as a side effect the mail has been refused by the list (too big).
> 
> So, this time you will find the patch in attachment, as suggested.

Thanks.  I've checked again right now, and found some remaining
issues.

> +	/* check PCI availability (32bit DMA) */
> +	if ((pci_set_dma_mask(pci, DMA_32BIT_MASK) < 0) ||
> +	    (pci_set_consistent_dma_mask(pci, DMA_32BIT_MASK) < 0)) {
> +		printk(KERN_ERR "Impossible to set 32bit mask DMA\n");

The error message should have some prefix to indicate the driver
name.  Otherwise you'll have no idea who wrote it.

> +	if (chip->saa7146.iobase_virt == NULL) {
> +		printk(KERN_ERR "unable to remap memory region");

Ditto.

> +	if (request_irq(pci->irq, snd_aw2_saa7146_interrupt,
> +			IRQF_SHARED, "Audiowerk2", (void *)chip)) {

The cast to void* isn't needed.

> +		printk(KERN_ERR "Cannot grab irq %d\n", pci->irq);

Add prefix.

> +/* constructor */
> +static int __devinit snd_aw2_probe(struct pci_dev *pci,
> +				   const struct pci_device_id *pci_id)
(snip)
> +	/* initialize semaphore */
> +	init_MUTEX(&(chip->saa7146.sem));

Use struct mutex instead of semaphore.  Also, the parenthesis around
'&' isn't needed.

> +	/* init spinlock */
> +	spin_lock_init(&chip->saa7146.reg_lock);

The spinlock is actually local to aw2-alsa.c, isn't it?  Then it
should be outside saa7146.

> +	/* initialize wait queue */
> +	init_waitqueue_head(&chip->saa7146.wait_iic);

... and I'd suggest to the initializations of struct saa7146 to
aw2-saa7146.c.  mutex should be initialized there, too.

> +/* create a pcm device */
> +static int __devinit snd_aw2_new_pcm(struct aw2 *chip)
(snip)
> +	/* pre-allocation of buffers */
> +	/* Preallocate continuous pages. */
> +	err = snd_pcm_lib_preallocate_pages_for_all(pcm_playback_ana,
> +						    SNDRV_DMA_TYPE_DEV,
> +						    snd_dma_pci_data
> +						    (chip->pci),
> +						    64 * 1024, 64 * 1024);
> +	if (err) {
> +		printk(KERN_ERR "snd_pcm_lib_preallocate_pages_for_all error "
> +		       "(0x%X)\n", err);
> +		return err;
> +	}

The error from preallocator isn't fatal.  You can ignore it.

> +void snd_aw2_saa7146_pcm_trigger_start_playback(struct snd_aw2_saa7146 *chip,
> +						int stream_number)
(snip)
> +	} else if (stream_number == 1) {
> +		WRITEREG((TR_E_A1_OUT << 16) | TR_E_A1_OUT, MC1);
> +
> +		/* WS1_CTRL, WS1_SYNC: output TSL1, I2S */
> +		acon1 |= 1 * WS1_CTRL;
> +		WRITEREG(acon1, ACON1);
> +	} else {
> +	}

Remove the empty block.

> +void snd_aw2_saa7146_pcm_trigger_stop_playback(struct snd_aw2_saa7146 *chip,
> +					       int stream_number)
(snip)
> +	} else if (stream_number == 1) {
> +		/* WS1_CTRL, WS1_SYNC: output TSL1, I2S */
> +		acon1 &= ~(3 * WS1_CTRL);
> +		WRITEREG(acon1, ACON1);
> +
> +		WRITEREG((TR_E_A1_OUT << 16), MC1);
> +	} else {
> +	}

Ditto.

> +void snd_aw2_saa7146_pcm_trigger_start_capture(struct snd_aw2_saa7146 *chip,
> +					       int stream_number)
> +{
> +	/* In aw8 driver, dma transfert is always active. It is
> +	   started and stopped in a larger "space" */
> +	if (stream_number == 0) {
> +		WRITEREG((TR_E_A1_IN << 16) | TR_E_A1_IN, MC1);
> +	} else {
> +	}

Ditto.

> +void snd_aw2_saa7146_pcm_trigger_stop_capture(struct snd_aw2_saa7146 *chip,
> +					      int stream_number)
> +{
> +	if (stream_number == 0) {
> +		WRITEREG((TR_E_A1_IN << 16), MC1);
> +	} else {
> +	}

Ditto.

> +irqreturn_t snd_aw2_saa7146_interrupt(int irq, void *dev_id)
> +{
> +	unsigned int isr;
> +	unsigned int iicsta;
> +	struct snd_aw2_saa7146 *chip = dev_id;
> +
> +	isr = READREG(ISR);
> +	if (!isr)
> +		return IRQ_NONE;
> +
> +	WRITEREG(isr, ISR);
> +
> +	if (isr & (IIC_S | IIC_E)) {
> +		iicsta = READREG(IICSTA);
> +		WRITEREG(0x100, IICSTA);
> +		wake_up(&chip->wait_iic);
> +	} else if (isr) {
> +	}

Ditto.

> +unsigned int snd_aw2_saa7146_get_hw_ptr_playback(struct snd_aw2_saa7146 *chip,
> +						 int stream_number,
> +						 unsigned char *start_addr,
> +						 unsigned int buffer_size)
(snip)
> +	if (stream_number == 1) {
> +		pci_adp = READREG(PCI_ADP1);
> +		ptr = pci_adp - (size_t) start_addr;
> +
> +		if (ptr == buffer_size)
> +			ptr = 0;
> +	} else {
> +	}

Ditto.

> +int snd_aw2_saa7146_is_using_digital_input(struct snd_aw2_saa7146 *chip)
> +{
> +	unsigned int reg_val = READREG(GPIO_CTRL);
> +	if ((reg_val & 0xFF) == 0x40) {
> +		return 1;
> +	} else {
> +		return 0;
> +	}

Remove braces around the single if-statement.
Can be found in many other places.
(I thought this should be suggested by checkpatch.pl, but apparently
 not perfectly detected...)


Takashi