[alsa-devel] [PATCH 2/3] saa7146: Emagic Audiowerk8 low-level ALSA driver
Takashi Iwai
tiwai at suse.de
Thu Oct 23 11:48:32 CEST 2008
At Thu, 23 Oct 2008 00:04:59 +0200,
Matthias Nyffenegger wrote:
>
> 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/alsa-kernel/pci/saa7146/README.txt 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/README.txt 2008-10-22 23:34:05.000000000 +0200
> @@ -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.
> 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/alsa-kernel/pci/saa7146/Makefile 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/Makefile 2008-10-22 23:34:05.000000000 +0200
> @@ -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.
> +obj-m := snd-aw8.o
The driver can be built in kernel, no?
> +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/alsa-kernel/pci/saa7146/aw8alsa.c 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8alsa.c 2008-10-22 23:34:05.000000000 +0200
(snip)
> +#include <linux/version.h>
You don't need this.
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/sched.h>
This, too.
> +#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.
> +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)
> + */
> +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 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.
> + 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]);
> + 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...
> +/**
> + * ALSA component-destructor.
> + * (see "Management of Cards and Components")
> + */
Ditto.
> +/**
> + * ALSA chip-specific constructor.
> + * (see "Management of Cards and Components")
> + */
Ditto.
> +static int __devinit snd_aw_create(struct snd_card *card,
> + struct pci_dev *pci,
> + struct awalsa_t **rchip)
> +{
(snip)
> + /* (1) PCI resource allocation */
The number if comments can be removed. In my document, it just
indicates the procedure step.
> +/**
> + * ALSA device constructor -- see "Constructor" sub-section in Takashi Iwai's
> + * "Writing an ALSA Driver" developer guide.
> + */
Hmm...
> +/**
> + *
> + * 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
> + * 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.
> 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/alsa-kernel/pci/saa7146/aw8.c 1970-01-01 01:00:00.000000000 +0100
> +++ ../alsa-driver-1.0.17.mod/alsa-kernel/pci/saa7146/aw8.c 2008-10-22 23:34:05.000000000 +0200
> +/**
> + * TODO: Description
> + */
> +enum aw_input_types {analog, digital};
Better to use other words, prefix, whatever.
> +/* 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.
> +static void switch_input(struct saa7146reg *chip, enum aw_input_types type)
(snip)
> + 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.
> + } 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
More information about the Alsa-devel
mailing list