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@free.fr

Signed-off-by: Jean-Christian Hassler jhassler@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

At Tue, 19 Feb 2008 22:34:36 +0100, Cédric Brégardis wrote:

At Tuesday 19 February 2008 18:28:28, Takashi Iwai wrote:

...

At Tue, 19 Feb 2008 18:11:45 +0100,

Cédric Brégardis wrote: [...]

...

So, this time you will find the patch in attachment, as suggested.

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

Thanks. I have corrected the driver according to your review. By the same time I have removed some dead code. The new patch is in attachment.

Thanks, now applied to HG tree.

.. and I found one more missing thing, the declaration of module parameters, after commitment. Fixed on HG tree now together with the addition of a very breif description of aw2 driver to ALSA-Configuration.txt.

Takashi