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

[Takashi Iwai]

At Sat, 16 Feb 2008 00:30:55 +0100,
Cédric Brégardis wrote:
> 
> Hi,
> 
> We managed to write ALSA driver for Emagic Audiowerk 2 PCI sound card (based 
> on Philips SAA7146 chip). You can find more information on  
> https://gna.org/projects/aw2-alsa/.
> 
> We think it's time to integrate the sources into the alsa/kernel tree. So, 
> here is the patch for this driver. We hope this patch is usable. 
> 
> Regards,
> 
> Jean-Christian Hassler and Cedric Bregardis
> 
> 
> Signed-off-by: Cedric Bregardis <cedric.bregardis at free.fr>
> Signed-off-by: Jean-Christian Hassler <jhassler at free.fr>

Thanks for the patch.

First of all, could you fix the issues reported by checkpatch.pl?

Below is a quick review.


> --- /dev/null
> +++ b/sound/pci/aw2/aw2-alsa.c
(snip)
> +#include <sound/driver.h>

sound/driver.h is deprecated now.  Simply remove this line.

> +struct aw2_pcm_device_t {

Usually *_t is used for typedef'ed types.  Use names without _t for
structs, i.e. "struct aw2_pcm_device".

> +/* module parameters (see "Module Parameters") */

The comment (see "Module Parameters") isn't needed for the real
codes.  It's a comment that is valid only in my tutorial.

> +/* pci_driver definition */
> +static struct pci_driver driver = {
> +	.name = "My Own Chip",

Choose a better name :)

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

Ditto about the comment.


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

Ditto.

> +static int __devinit snd_aw2_create(struct snd_card *card,
> +				    struct pci_dev *pci, struct aw2_t **rchip)
> +{
(snip)
> +	chip = kcalloc(1, sizeof(*chip), GFP_KERNEL);

Use kzalloc.

> +	/* (1) PCI resource allocation */
> +	err = pci_request_regions(pci, "Audiowerk2");
> +	if (err < 0) {
> +		kfree(chip);
> +		return err;
> +	}
> +	chip->saa7146.iobase_phys = pci_resource_start(pci, 0);
> +	chip->saa7146.iobase_virt =
> +	    ioremap_nocache(chip->saa7146.iobase_phys,
> +			    pci_resource_len(pci, 0));

Better to check whether ioremap succeeded or not.

> +/* constructor -- see "Constructor" sub-section */

Fix the comment.

> +/* destructor -- see "Destructor" sub-section */

Dtto.

> +/* open callback */
> +static int snd_aw2_pcm_playback_open(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +	printk(KERN_DEBUG "Playback_open \n");

Avoid unconditionally compiled debug messages.  If you really want to
keep such debug messages, use snd_printdd() or whatever that won't be
compiled unless explicitly specified.

> +/* create a pcm device */
> +static int __devinit snd_aw2_new_pcm(struct aw2_t *chip)
> +{
> +	struct snd_pcm *pcm_playback_ana;
> +	struct snd_pcm *pcm_playback_num;
> +	struct snd_pcm *pcm_capture;
> +	int err = 0;
> +
> +	/* Create new Alsa PCM device */
> +
> +	err =
> +	    snd_pcm_new(chip->card, "Audiowerk2 analog playback", 0, 1, 0,
> +			&pcm_playback_ana);
> +	if (err >= 0) {

It's easier to bail out when the error is returned.

> +	err = snd_pcm_new(chip->card, "Audiowerk2 capture", 2, 0, 1,
> +			  &pcm_capture);

Any reason to make all PCM streams non-duplex?
Oh I see in the below...

> +static int snd_aw2_control_switch_capture_info(struct snd_kcontrol *kcontrol,
> +					       struct snd_ctl_elem_info *uinfo)
> +{
> +	static char *texts[2] = {
> +		"Analog", "Digital"
> +	};
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
> +	uinfo->count = 1;
> +	uinfo->value.enumerated.items = 2;
> +	if (uinfo->value.enumerated.item >= uinfo->value.enumerated.items) {
> +		uinfo->value.enumerated.item =
> +		    uinfo->value.enumerated.items - 1;
> +	}
> +	strcpy(uinfo->value.enumerated.name,
> +	       texts[uinfo->value.enumerated.item]);
> +	return 0;
> +}

So, you'd like to switch the input on the fly rather than selecting at
open time?  It's a question of the driver design...


> --- /dev/null
> +++ b/sound/pci/aw2/aw2-saa7146.c
(snip)
> +#define WRITEREG(value, addr) do { writel((value), chip->iobase_virt + (addr));\
> +				   mb(); } while (0)

Do you really need a barrier here?

> +void snd_aw2_saa7146_dump_registers(struct snd_aw2_saa7146_t *chip)
> +{
> +	int i = 0;
> +	for (i = 0; i <= 0x148; i += 4) {
> +		printk(KERN_DEBUG "###DUMP 0x%03x: 0x%08x\n", i, READREG(i));
> +	}
> +}

Better to use proc interface than printk for such a purpose.

> +/* chip-specific destructor
> + * (see "PCI Resource Managements")
> + */
> +int snd_aw2_saa7146_free(struct snd_aw2_saa7146_t *chip)
> +{
> +	/* disable all irqs */
> +	WRITEREG(0, IER);
> +
> +	/* reset saa7146 */
> +	WRITEREG((MRST_N << 16), MC1);
> +
> +	/* release the irq */
> +	if (chip->irq >= 0) {
> +		free_irq(chip->irq, (void *)chip);
> +	}
> +	/* release the i/o ports & memory */
> +	if (chip->iobase_virt) {
> +		iounmap(chip->iobase_virt);
> +	}
> +	pci_release_regions(chip->pci);
> +	/* disable the PCI entry */
> +	pci_disable_device(chip->pci);
> +	/* release the data */
> +	kfree(chip);
> +	return 0;
> +}

Put the destructor in the same place as the constructor.  Don't put in
separate files.


> --- /dev/null
> +++ b/sound/pci/aw2/aw2-saa7146.h
(snip)
> +#ifdef AW2_SAA7146_M
> +#define PUBLIC
> +#else
> +#define PUBLIC extern
> +#endif

Don't do this.  It's too hackish.


thanks,

Takashi