Skip to contentSkip to conversation

saa7146: Emagic Audiowerk8 low-level ALSA driver

PATCH In review Matthias Nyffenegger 2 participants 2 messages Oct 22, 2008 – Oct 23, 2008

lore
Share & tools
lore

Matthias Nyffenegger

author
#0 lore raw

PATCH 2/3 saa7146: Emagic Audiowerk8 low-level ALSA driver

From: Matthias Nyffenegger <[email protected]>

Low-level ALSA driver for Emagic Audiowerk8 sound card. Project page: http://sourceforge.net/projects/aw8-alsa
Built and tested with Vanilla 2.6.25.16

Signed-off-by Matthias Nyffenegger <[email protected]>

---
This is a request for submission to ALSA-tree.

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/README.txt ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/README.txt

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/README.txt renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/README.txt +1 -0
@@ -0,0 +1 @@
+See sourceforge.net project "Audiowerk8 ALSA driver" (aw8-alsa) Wiki for more information.

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/Makefile ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/Makefile +8 -0
@@ -0,0 +1,8 @@
+# Makefile for Audiowerk module.
+# run command in source directory:
+# 	make -C /usr/src/linux M=`pwd` modules modules_install
+
+EXTRA_CFLAGS := -DSAA7146_SUBSYS_LOG_TAG=\"AW8\"
+obj-m := snd-aw8.o
+snd-aw8-objs := saa7146i2c.o saa7146i2s.o saa7146audio.o aw8.o aw8alsa.o
+

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +826 -0

826 lines not on this page · raw message

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.c ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.c +185 -0

185 lines not on this page · raw message

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.h ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.h

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.h renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.h +80 -0

80 lines not on this page · raw message

#1 lore raw

At Thu, 23 Oct 2008 00:04:59 +0200,

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/README.txt line 1

Quoted 7 lines · Matthias Nyffenegger

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/README.txt ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/README.txt

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/README.txt renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/README.txt +1 -0
@@ -0,0 +1 @@
+See sourceforge.net project "Audiowerk8 ALSA driver" (aw8-alsa) Wiki for more information.

Hmm, this isn't particularly informative. Such a comment can be put in the corresponding card entry in Documentation/sound/alsa/ALSA-Configuration.txt.

Also, this directory is no right place to put the document. Put any document to Documentation/sound/alsa directory.

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile line 5

Quoted 10 lines

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/Makefile ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/Makefile +5 -0
@@ -0,0 +1,8 @@
+# Makefile for Audiowerk module.
+# run command in source directory:
+# 	make -C /usr/src/linux M=`pwd` modules modules_install
+
+EXTRA_CFLAGS := -DSAA7146_SUBSYS_LOG_TAG=\"AW8\"

Don't use EXTRA_CFLAGS just for this purpose.

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile line 6

Quoted 1 line
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/Makefile +1 -0
+obj-m := snd-aw8.o

The driver can be built in kernel, no?

Quoted 7 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/Makefile +1 -0
+snd-aw8-objs := saa7146i2c.o saa7146i2s.o saa7146audio.o aw8.o aw8alsa.o

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +0 -0

(snip)

Quoted 1 line

+#include <linux/version.h>

You don't need this.

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c line 26

Quoted 5 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +5 -0
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/sched.h>

This, too.

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c line 37

Quoted 11 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +11 -0
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <sound/core.h>
+#include <sound/initval.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include "log.h"
+#include "saa7146audio.h"
+#include "aw8.h"
+
+/* From Takashi Iwai's "Writing an ALSA Driver" developer guide */

Hmm... I suggest you to remove this.

Quoted 16 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +16 -0
+static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
+static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
+static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP;
+
+/*
+ * Module info
+ */
+MODULE_AUTHOR("Matthias Nyffenegger matthias.nyffenegger[AT]bluewin.ch");
+MODULE_DESCRIPTION(AW_MODULE_DESCRIPTION);
+MODULE_LICENSE("GPL");
+MODULE_VERSION("0:0.2");
+
+/**
+ * Module parameter 'input_type': select capture input digital or analog
+ * 				(default)
+ */

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c line 63

Quoted 10 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +10 -0
+int input_type;
+module_param(input_type, int, 0444);
+MODULE_PARM_DESC(input_type, "0=analog (default), 1=digital");
+
+#define PCM_0	0
+
+/**
+ * The ALSA 'chip' data structure.
+ */
+struct awalsa_t {

Usually *_t is for typedefed objects. No big problem, though.

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c line 544

Quoted 9 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +9 -0
+/**
+ * ALSA Interrupt Service Routine.
+ */
+static irqreturn_t snd_aw_interrupt(int irq, void *dev_id)
+{
+	struct awalsa_t *awalsa = dev_id;
+	uint32_t isr = 0;
+
+	/* lock isr - block other interrupts */

The spinlock in the interrupt handler isn't for blocking other irqs In your case, apparently no lock is necessary.

Quoted 13 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +13 -0
+	spin_lock(awalsa->isr_lock);
+	isr = saa7146_read(&awalsa->chipaudio.chipi2s.chip, ISR);
+	/* was it one of SAA7146A (remember we might share irq line ..) */
+	if (!isr) {
+		spin_unlock(awalsa->isr_lock);
+		return IRQ_NONE;
+	}
+	/* clear all interrupts */
+	saa7146_write(&awalsa->chipaudio.chipi2s.chip, ISR, isr);
+	if (isr & A1_in && awalsa->pcm_in_substreams[0] != NULL)
+		snd_pcm_period_elapsed(awalsa->pcm_in_substreams[0]);
+	if (isr & A1_out && awalsa->pcm_out_substreams[0] != NULL)
+		snd_pcm_period_elapsed(awalsa->pcm_out_substreams[0]);

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c line 567

Quoted 10 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +10 -0
+	if (isr & A2_out && awalsa->pcm_out_substreams[1] != NULL)
+		snd_pcm_period_elapsed(awalsa->pcm_out_substreams[1]);
+	spin_unlock(awalsa->isr_lock);
+	return IRQ_HANDLED;
+}
+
+/**
+ * ALSA chip-specific destructor.
+ * (see "PCI Resource Managements")
+ */

Better to fix comments...

Quoted 4 lines

+/**
+ * ALSA component-destructor.
+ * (see "Management of Cards and Components")
+ */

Ditto.

Quoted 4 lines

+/**
+ * ALSA chip-specific constructor.
+ * (see "Management of Cards and Components")
+ */

Ditto.

Quoted 4 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +4 -0
+static int __devinit snd_aw_create(struct snd_card *card,
+					struct pci_dev *pci,
+					struct awalsa_t **rchip)
+{

(snip)

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c line 637

Quoted 1 line
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +1 -0
+	/* (1) PCI resource allocation */

The number if comments can be removed. In my document, it just indicates the procedure step.

Quoted 4 lines

+/**
+ * ALSA device constructor -- see "Constructor" sub-section in Takashi Iwai's
+ * "Writing an ALSA Driver" developer guide.
+ */

Hmm...

Quoted 17 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +17 -0
+/**
+ *
+ * PCI specific stuff
+ *
+ */
+
+/**
+ * Organisation of EEPROM of the SAA7146A:
+ * The data of the subsystem ID and the subsystem vendor ID is organized in
+ * the EEPROM in the following order:
+ *
+ * EE-Addr	Value 				Configuration Space Addr.
+ * ---------------------------------------------------------------------
+ * 00 		Subsys ID (high byte) 		2C, bits: 31 - 24
+ * 01 		Subsys ID (low byte)		2C, bits: 23 - 16
+ * 02 		Subsys Vendor ID (high byte) 	2C, bits: 15 -  8
+ * 03 		Subsys Vendor ID (low byte) 	2C, bits:  7 -  0

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c line 786

Quoted 10 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8alsa.c +10 -0
+ * 04 		Max_Lat 			3C, bits: 31 - 24
+ * 05 		Min_Gnt 			3C, bits: 23 - 16
+ *
+ * For AW8 Subsys ID and Subsys Vendor ID are 0xFF on my card.
+ * For AW2 Subsys ID and Subsys Vendor ID are 0x00 on my card.
+ */
+
+static struct pci_device_id snd_aw_ids[] = {
+	{ PCI_VENDOR_ID_PHILIPS, PCI_DEVICE_ID_PHILIPS_SAA7146,
+	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0, },

We have a bad problem here.
As mentioned, there are other drivers with the very same device and the very same PCI ID.
Since your driver doesn't check the availability of the hardware in some way at probe, it loads even for a video device. The same problem is present in the existing snd-aw2 driver, and we need to find out the solution.

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c line 41

Quoted 8 lines

diff -uprN ../alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.c ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.c +4 -0
+/**
+ * TODO: Description
+ */
+enum aw_input_types {analog, digital};

Better to use other words, prefix, whatever.

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c line 50

Quoted 8 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.c +8 -0
+/* forward declarations */
+static int aw8_device_init(struct saa7146i2s *chipi2s);
+static void switch_input(struct saa7146reg *chip, enum aw_input_types type);
+
+/**
+ * see aw8.h
+ */
+int aw_init(struct saa7146i2s *chipi2s, int input_type)

This function name should be more unique, too.

Quoted 1 line

+static void switch_input(struct saa7146reg *chip, enum aw_input_types type)

(snip)

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c line 174

Quoted 5 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.c +5 -0
+	if (type == analog) {
+		saa7146_write(chip, GPIO_0, MASK_GPIO_HI);
+		/* wait until relais is switched and PLL is locked */
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		schedule_timeout(MAX(1, SLEEP_PLL*HZ/1000));

Use msleep() instead.

alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c line 179

Quoted 5 lines
alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c renamed from alsa-driver-1.0.17/alsa-kernel/pci/saa7146/aw8.c +5 -0
+	} else {
+		saa7146_write(chip, GPIO_0, MASK_GPIO_LO);
+		/* wait until relais is switched and digital-in is synced. */
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		schedule_timeout(MAX(1, SLEEP_DIGIN*HZ/1000));

Ditto.

thanks,

Takashi