[alsa-devel] Driver for TerraTec DMX 6Fire USB

[Takashi Iwai]

At Tue, 18 Jan 2011 14:41:47 +0100,
torsten.schenk wrote:
> 
> Hello,
> 
> (to all those who already got this mail: sorry, I sent the attachment accidentally in html format so it was ignored by alsa-devel ML distributor)
> 
> i recently got a TerraTec DMX 6Fire USB. Since it's not supported by alsa, I decided to look into communications between the windows driver and the card. Here is the result.
> 
> What should be revised:
> - I was unsure about naming conventions, so please check the name of card/midi/pcm/module.
> - Also the device needs firmware to be able to run, so I included the firmware from the windows driver in the attachment. I read about that Linus Torvalds doesn't like .ihex-parsing in drivers, but I cannot convert the files easily into a binary format because the offsets are not contiguous. Of course there could be alternate ways to be thought about, which would imply that original files from TT are not supported since they are in ihex.
> - I didn't manage to create a configuration in /usr/share/alsa/cards.
> 
> What is working: Everything except SPDIF
> - Hardware Master volume
> - PCM 44-192kHz at 24 bits, 6 channels out, 4 channels in (analog)
> - MIDI in/out
> - firmware loading for coldplugged devices
> - phono/line switching
> 
> If anything else comes to your attention, just let me know.

Sorry for the late reply.  It took some time until I have free slot.

First off, there are lots of coding-style issues to be fixed.
Try to run scripts/checkpatch.pl once with your patch.  Not all warnings
have to be fixed (e.g. 80-chars) but would be nice if fixed.

Here are some review comments through a very quirk glance:

> +static int __devinit probe(struct usb_interface *intf, const struct usb_device_id *usb_id)

Adding a short prefix (e.g. usb6fire_probe()) is better in general.
Otherwise the stack trace would be difficult to read.  Of course,
too long names are also annoying, so it's a matter of taste.

...
> +	chip = (struct sfire_chip*)card->private_data;

You can avoid cast here.

> +	chips[regidx] = chip;
> +	chip->dev = device;
> +	chip->regidx = regidx;
> +	chip->intf_count = 1;
> +	chip->card = card;
> +	chip->shutdown = FALSE;
> +	chip->midi = NULL;
> +	chip->comm = NULL;
> +	chip->control = NULL;
> +	chip->pcm = NULL;

NULLs are automatically filled, so these can be skipped.

> +	ret = snd_usb_6fire_comm_init(chip);
> +	if(ret < 0)
> +	{
> +		snd_card_free(card);
> +		return ret;
> +	}
> +
> +	ret = snd_usb_6fire_midi_init(chip);
> +	if(ret < 0)
> +	{
> +		snd_usb_6fire_comm_destroy(chip);
> +		snd_card_free(card);
> +		return ret;
> +	}
> +
> +	ret = snd_usb_6fire_pcm_init(chip);
> +	if(ret < 0)
> +	{
> +		snd_usb_6fire_midi_destroy(chip);
> +		snd_usb_6fire_comm_destroy(chip);
> +		snd_card_free(card);
> +		return ret;
> +	}
> +
> +	ret = snd_usb_6fire_control_init(chip);
> +	if(ret < 0)
> +	{
> +		snd_usb_6fire_pcm_destroy(chip);
> +		snd_usb_6fire_midi_destroy(chip);
> +		snd_usb_6fire_comm_destroy(chip);
> +		snd_card_free(card);
> +		return ret;
> +	}
> +
> +	ret = snd_card_register(card);
> +	if(ret < 0)
> +	{
> +		snd_usb_6fire_pcm_destroy(chip);
> +		snd_usb_6fire_midi_destroy(chip);
> +		snd_usb_6fire_comm_destroy(chip);
> +		snd_usb_6fire_control_destroy(chip);
> +		snd_card_free(card);

A general way for error-handling is either using goto or using a
common destructor.  In either way, you can call the *_free or *_destroy
with the non-NULL check of each instance.  It's a cleaner in general.

> +static void disconnect(struct usb_interface *intf)
> +{
> +	struct sfire_chip *chip;
> +	struct snd_card *card;
> +	
> +	chip = usb_get_intfdata(intf);
> +	if(chip) //if !chip, fw upload has been performed so that we don't have a chip.
> +	{
> +		card = chip->card;
> +		chip->intf_count--;
> +		if (chip->intf_count == 0)
> +		{
> +			chip->shutdown = TRUE;
> +			snd_card_disconnect(card);
> +			snd_usb_6fire_comm_destroy(chip);
> +			snd_usb_6fire_control_destroy(chip);
> +			snd_usb_6fire_pcm_destroy(chip);
> +			snd_usb_6fire_midi_destroy(chip);
> +			snd_card_free(card);
> +			usb_set_intfdata(intf, NULL);

The same destructor as in probe can be used here.

> +static struct usb_device_id device_table[] =
> +{
> +	{
> +		.match_flags = USB_DEVICE_ID_MATCH_DEVICE,
> +		.idVendor = 0x0ccd,
> +		.idProduct = 0x0080
> +	},
> +	{ }
> +};

Usually MODULE_DEVICE_TABLE() follows just after the definition.

> diff -Nur a/sound/usb/6fire/chip.h b/sound/usb/6fire/chip.h
> --- a/sound/usb/6fire/chip.h	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/chip.h	2011-01-18 13:58:06.000000000 +0100
> @@ -0,0 +1,30 @@
> +/*
> + * Linux driver for TerraTec DMX 6Fire USB
> + *
> + * Author: Torsten Schenk
> + * Created: Jan 01, 2011
> + * Copyright:	(C) Torsten Schenk
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#pragma once

Don't use prgram.
Also, add #ifdef xxx for protecting the double inclusion.

> +#include "common.h"
> +
> +struct sfire_chip
> +{
> +	struct usb_device *dev;
> +	struct snd_card *card;
> +	int intf_count; //number of registered interfaces
> +	int regidx; //index in module parameter arrays
> +	BOOL shutdown;

Don't use own bool type.  Use the generic "bool" type, and use
true/false instead of TRUE/FALSE (unless the size really matters).

> diff -Nur a/sound/usb/6fire/comm.c b/sound/usb/6fire/comm.c
> --- a/sound/usb/6fire/comm.c	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/comm.c	2011-01-18 13:58:21.000000000 +0100
...
> +static int write_02(struct comm_runtime *rt, __u8 reg, __u16 value)

Use u8 and u16.

> diff -Nur a/sound/usb/6fire/comm.h b/sound/usb/6fire/comm.h
> --- a/sound/usb/6fire/comm.h	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/comm.h	2011-01-18 13:58:29.000000000 +0100
> @@ -0,0 +1,42 @@
> +/*
> + * Linux driver for TerraTec DMX 6Fire USB
> + *
> + * Author: Torsten Schenk
> + * Created: Jan 01, 2011
> + * Copyright:	(C) Torsten Schenk
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#pragma once

Ditto.  No pragma and add ifdef.

> diff -Nur a/sound/usb/6fire/common.h b/sound/usb/6fire/common.h
> --- a/sound/usb/6fire/common.h	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/common.h	2011-01-18 13:58:38.000000000 +0100
> +#pragma once
> +
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <sound/core.h>
> +
> +#define BOOL char
> +#define TRUE 1
> +#define FALSE 0

Don't use these.

> +#define MIN(X, Y) ((X) < (Y) ? (X) : (Y))
> +#define MAX(X, Y) ((X) > (Y) ? (X) : (Y))

Use standard min() / max() macros.

> +#define ERR KERN_ERR "6fire: "
> +#define WARNING KERN_WARNING "6fire: "

Hmm... These are also better to be avoided.  At most, define
"6fire: " as PREFIX or PFX or whatever.  But KERN_ERR should be put
plainly.

> diff -Nur a/sound/usb/6fire/control.c b/sound/usb/6fire/control.c
> --- a/sound/usb/6fire/control.c	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/control.c	2011-01-18 13:58:57.000000000 +0100
> diff -Nur a/sound/usb/6fire/firmware.c b/sound/usb/6fire/firmware.c
> --- a/sound/usb/6fire/firmware.c	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/firmware.c	2011-01-18 13:57:15.000000000 +0100
...
> +static inline int fpga_write(struct usb_device *device, char *data, int len)

Avoid inline unless you really need it.
Also add MODULE_FIRMWARE() definitions in this file.

> diff -Nur a/sound/usb/6fire/Makefile b/sound/usb/6fire/Makefile
> --- a/sound/usb/6fire/Makefile	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/Makefile	2011-01-16 23:14:22.000000000 +0100
> @@ -0,0 +1,13 @@
> +obj-m += snd-usb-6fire.o
> +snd-usb-6fire-objs += chip.o comm.o midi.o control.o firmware.o pcm.o
> +
> +all:
> +	make CONFIG_DEBUG_SECTION_MISMATCH=y -C /lib/modules/$(shell uname -r)/build M=$(PWD) modules
> +
> +clean:
> +	make -C /lib/modules/$(shell uname -r)/build M=$(PWD) clean
> +
> +install:
> +	mkdir -p /lib/modules/$(shell uname -r)/extras
> +	cp snd-usb-6fire.ko /lib/modules/$(shell uname -r)/extras

Remove all, clean, install things.

> diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
> --- a/sound/usb/6fire/pcm.c	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/pcm.c	2011-01-18 13:59:57.000000000 +0100
> diff -Nur a/sound/usb/Kconfig b/sound/usb/Kconfig
> --- a/sound/usb/Kconfig	2011-01-16 23:20:41.000000000 +0100
> +++ b/sound/usb/Kconfig	2011-01-16 23:14:22.000000000 +0100
> @@ -32,6 +32,17 @@
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called snd-ua101.
 
> +config SND_USB_6FIRE
> +	tristate "TerraTec DMX 6Fire USB"
> +	depends on X86
> +	select SND_HWDEP

Do you really need hwdep?
Rather select FW_LOADER, if any.

> +	select SND_PCM

Missing SND_RAWMIDI dependency.


> @@ -65,7 +76,6 @@
>  	    * Native Instruments Guitar Rig Session I/O
>  	    * Native Instruments Guitar Rig mobile
>  	    * Native Instruments Traktor Kontrol X1
> -	    * Native Instruments Traktor Kontrol S4
 
>  	   To compile this driver as a module, choose M here: the module
>  	   will be called snd-usb-caiaq.
> @@ -83,7 +93,6 @@
>  	   * Native Instruments Kore Controller
>  	   * Native Instruments Kore Controller 2
>  	   * Native Instruments Audio Kontrol 1
> -	   * Native Instruments Traktor Kontrol S4
 
Don't bother others ;)


thanks,

Takashi