Driver for TerraTec DMX 6Fire USB

Driver for TerraTec DMX 6Fire USB

[Torsten Schenk]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

Hello,

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/control/module.
- Also the device needs firmware to be able to run. I did not included the firmware (from the windows driver) in the attachment since the 60kB mail size limit were exceeded last time I tried to post this driver.
- Firmware: I read about that .ihex-parsing in drivers is not really approved, 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@24 bits, 6 channels out, 4 channels in (analog)
- MIDI in/out
- firmware loading after cold start
- phono/line switching

If anything else comes to your attention, just let me know.

Greetings, Torsten



[-- Attachment #2: 6fireusb.tar.bz2 --]
[-- Type: application/octet-stream, Size: 13189 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Driver for TerraTec DMX 6Fire USB

[Torsten Schenk]

>Thanks. I committed your patch yesterday to sound git tree. 
> 
>For further works, please base on the latest sound git tree. 
Ok. Thanks very much.

Greetings, Torsten

Re: Driver for TerraTec DMX 6Fire USB

[Takashi Iwai]

At Mon, 24 Jan 2011 13:10:58 +0100,
Torsten Schenk wrote:
> 
> Hello,
> 
> I had to review the code since an error came up: when sound was playing and I disconnected the device, the whole system crashed. This bug is now solved (pcm.c: check if a isoc-packet has invalid status on retire, if so, abort streaming).
> 
> >> >> 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-20 23:07:24.000000000 +0100 
> >> >... 
> >> >> +/* keep next two synced with 
> >> >> + * FW_EP_W_MAX_PACKET_SIZE[] and RATES_MAX_PACKET_SIZE */ 
> >> >> +static const int RATES_IN_PACKET_SIZE[] = { 228, 228, 420, 420, 404, 404 }; 
> >> >> +static const int RATES_OUT_PACKET_SIZE[] = { 228, 228, 420, 420, 604, 604 }; 
> >> >> +static const int RATES[] = { 44100, 48000, 88200, 96000, 176400, 192000 }; 
> >> > 
> >> >Any reason to use capital letters for these? 
> >> >Because they are const? 
> >> That was exactly the idea. 
> > 
> >Hm, but it's not so common. Not too annoying as well, though. 
> Ok, I thought it would be that way... No matter, I changed it to small letters.
> 
> >> >Why x86-dependent? 
> >> I thought so because of the firmware stuff. If bit- or byte-order are changed, the firmware uploading might not work. If I figured out the #ifdef stuff for bit and byte order, I will remove this dependency. 
> > 
> >But you are decoding in bytes, not in words or so. Thus the CPU 
> >byte-order doesn't matter. 
> I reviewed the code and found exactly one endian-dependend thing: a const array I defined with type u16. I changed that to u8 and removed the x86 dependency.

Thanks.  I committed your patch yesterday to sound git tree.

For further works, please base on the latest sound git tree.


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Driver for TerraTec DMX 6Fire USB

[Torsten Schenk]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

Hello,

I had to review the code since an error came up: when sound was playing and I disconnected the device, the whole system crashed. This bug is now solved (pcm.c: check if a isoc-packet has invalid status on retire, if so, abort streaming).

>> >> 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-20 23:07:24.000000000 +0100 
>> >... 
>> >> +/* keep next two synced with 
>> >> + * FW_EP_W_MAX_PACKET_SIZE[] and RATES_MAX_PACKET_SIZE */ 
>> >> +static const int RATES_IN_PACKET_SIZE[] = { 228, 228, 420, 420, 404, 404 }; 
>> >> +static const int RATES_OUT_PACKET_SIZE[] = { 228, 228, 420, 420, 604, 604 }; 
>> >> +static const int RATES[] = { 44100, 48000, 88200, 96000, 176400, 192000 }; 
>> > 
>> >Any reason to use capital letters for these? 
>> >Because they are const? 
>> That was exactly the idea. 
> 
>Hm, but it's not so common. Not too annoying as well, though. 
Ok, I thought it would be that way... No matter, I changed it to small letters.

>> >Why x86-dependent? 
>> I thought so because of the firmware stuff. If bit- or byte-order are changed, the firmware uploading might not work. If I figured out the #ifdef stuff for bit and byte order, I will remove this dependency. 
> 
>But you are decoding in bytes, not in words or so. Thus the CPU 
>byte-order doesn't matter. 
I reviewed the code and found exactly one endian-dependend thing: a const array I defined with type u16. I changed that to u8 and removed the x86 dependency.

Greetings,
Torsten

[-- Attachment #2: usb6fire.tar.bz2 --]
[-- Type: application/octet-stream, Size: 13105 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Driver for TerraTec DMX 6Fire USB

[Takashi Iwai]

At Fri, 21 Jan 2011 12:48:52 +0100,
Torsten Schenk wrote:
> 
> >I found that the recent kernel has linux/ihex.h helper functions. 
> >They might help for simplifying the code. But, this can be done later 
> >as a a clean-up. 
> Yes, I also discovered this helper function, but it supports only some kind of binary ihex format. It did not accept
> the text .ihx files.

OK.

> >> 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-20 23:07:24.000000000 +0100 
> >... 
> >> +/* keep next two synced with 
> >> + * FW_EP_W_MAX_PACKET_SIZE[] and RATES_MAX_PACKET_SIZE */ 
> >> +static const int RATES_IN_PACKET_SIZE[] = { 228, 228, 420, 420, 404, 404 }; 
> >> +static const int RATES_OUT_PACKET_SIZE[] = { 228, 228, 420, 420, 604, 604 }; 
> >> +static const int RATES[] = { 44100, 48000, 88200, 96000, 176400, 192000 }; 
> > 
> >Any reason to use capital letters for these? 
> >Because they are const? 
> That was exactly the idea.

Hm, but it's not so common.  Not too annoying as well, though.

> >Why x86-dependent? 
> I thought so because of the firmware stuff. If bit- or byte-order are changed, the firmware uploading might not work. If I figured out the #ifdef stuff for bit and byte order, I will remove this dependency.

But you are decoding in bytes, not in words or so.  Thus the CPU
byte-order doesn't matter.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Driver for TerraTec DMX 6Fire USB

[Torsten Schenk]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

>It means that your driver code tree isn't sync'ed with the latest 
>tree. Just rebase your tree to the latest one. Or... 
The trick is, I have a kernel tree that only contains this Kconfig. So these two should be the same, because I copied them.
No matter, I edited the patch manually and hope it will perform as it should.

>I found that the recent kernel has linux/ihex.h helper functions. 
>They might help for simplifying the code. But, this can be done later 
>as a a clean-up. 
Yes, I also discovered this helper function, but it supports only some kind of binary ihex format. It did not accept
the text .ihx files.

> 
>> 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-20 23:07:24.000000000 +0100 
>... 
>> +/* keep next two synced with 
>> + * FW_EP_W_MAX_PACKET_SIZE[] and RATES_MAX_PACKET_SIZE */ 
>> +static const int RATES_IN_PACKET_SIZE[] = { 228, 228, 420, 420, 404, 404 }; 
>> +static const int RATES_OUT_PACKET_SIZE[] = { 228, 228, 420, 420, 604, 604 }; 
>> +static const int RATES[] = { 44100, 48000, 88200, 96000, 176400, 192000 }; 
> 
>Any reason to use capital letters for these? 
>Because they are const? 
That was exactly the idea.

>Why x86-dependent? 
I thought so because of the firmware stuff. If bit- or byte-order are changed, the firmware uploading might not work. If I figured out the #ifdef stuff for bit and byte order, I will remove this dependency.

Thanks,
Torsten

[-- Attachment #2: usb6fire.tar.bz2 --]
[-- Type: application/octet-stream, Size: 13280 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Driver for TerraTec DMX 6Fire USB

[Takashi Iwai]

At Thu, 20 Jan 2011 23:38:50 +0100,
Torsten Schenk wrote:
> 
> Hello,
> 
> Thanks very much for the review. I corrected the issues you wrote about (except the last one, see below). checkpatch.pl reports now 0 errors and warnings.
> 
> Another thing about the driver's features:
> I'm currently in contact with a guy that also owns a TT DMX6FireUSB. He says, firmware is not loading for him. I'm trying to figure out how to make
> it work, but I already know, that my driver cannot provide fw uploading for every device out there by now. I think it's just a matter of removing some checks, but I want to make as sure as I can before I just do so.
> 
> If the device was previously used in windows and not unplugged from power, it will work also with no firmware upload.

Could you put such information in either git changelog, some
document or in a comment in the source code?


> >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.
> I though so. checkpatch.pl is a useful script, good to know it exists.
> 
> >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. 
> How is it supposed to be? I reviewed my code and added a prefix to every function. Hopefully
> it didn't get too long now... Some, especially functions with not much content, seem to have a too long name by now for my flavour. 
> 
> >NULLs are automatically filled, so these can be skipped. 
> Which NULLs are automatically filled? Just the area create by snd_card_create or
> every area allocated via kmalloc? If last is true, I will have to remove some more NULLs.

Not with kmalloc.  There is kzalloc or kcalloc for such purpose.
The space allocated via snd_card_new() is zero-cleared.


> >Don't use own bool type. Use the generic "bool" type, and use 
> >true/false instead of TRUE/FALSE (unless the size really matters). 
> >Use standard min() / max() macros. 
> >Use u8 and u16. 
> Didn't know these exists...
> When I used C once, 'bool' and also 'min'/'max' weren't available.
> 
> >Remove all, clean, install things. 
> Just forgot ;)
> 
> >Do you really need hwdep? 
> >Rather select FW_LOADER, if any. 
> > 
> >> +    select SND_PCM 
> Thanks, just copied this from another definition without exactly knowing what it does...
> > 
> >Missing SND_RAWMIDI dependency. 
> Realized that after posting.
> 
> > 
> > 
> >> @@ -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 ;) 
> I'd love not to ;) But how can I remove it? It's automatically generated by diff. Perhaps wrong arguments? Mine are -Nur

It means that your driver code tree isn't sync'ed with the latest
tree.  Just rebase your tree to the latest one.  Or...

> Or should I simply remove these lines with a text editor?

This is an easy alternative.

Below is a few more review comments I found in your new patch.
Could you fix and resend?


thanks,

Takashi

> diff -Nur a/sound/usb/6fire/chip.c b/sound/usb/6fire/chip.c
> --- a/sound/usb/6fire/chip.c	1970-01-01 01:00:00.000000000 +0100
> +++ b/sound/usb/6fire/chip.c	2011-01-20 22:56:37.000000000 +0100
> +static int __devinit usb6fire_chip_probe(struct usb_interface *intf,
> +		const struct usb_device_id *usb_id)
..
> +	/* look if we already serve this card and return if so */
> +	mutex_lock(&register_mutex);
> +	for (i = 0; i < SNDRV_CARDS; i++)
> +		if (devices[i] == device) {
> +			if (chips[i])
> +				chips[i]->intf_count++;
> +			usb_set_intfdata(intf, chips[i]);
> +			mutex_unlock(&register_mutex);
> +			return 0;
> +		} else if (regidx < 0)
> +			regidx = i;
> +		if (regidx < 0) {

The for block should be a block with {...}, as it's no one-line if
statement.

> +static void usb6fire_chip_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 */
> +		card = chip->card;
> +		chip->intf_count--;
> +		if (!chip->intf_count) {
> +			chip->shutdown = true;
> +			snd_card_disconnect(card);
> +			usb6fire_chip_destroy(chip);
> +			usb_set_intfdata(intf, NULL);
> +		}
> +	}

chips[] and devices[] aren't cleared at disconnect?

> 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-20 23:03:32.000000000 +0100
> @@ -0,0 +1,279 @@
> +/*
> + * Linux driver for TerraTec DMX 6Fire USB
> + *
> + * Mixer control
> + *
> + * 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.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <sound/control.h>
> +
> +#include "control.h"
> +#include "comm.h"
> +#include "chip.h"
> +
> +static char *opt_coax_texts[2] = { "Optical", "Coax" };
> +static char *line_phono_texts[2] = { "Line", "Phono" };
> +
> +/*
> + * calculated with $value\[i\] = 128 \cdot sqrt[3]{\frac{i}{128}}$
> + * this is done because the linear values cause rapid degredation
> + * of volume in the uppermost region.
> + */
> +static const u8 LOG_VOLUME_TABLE[128] = { 0x00, 0x19, 0x20, 0x24, 0x28, 0x2b,

Begin the data in the next line.  And use one-tab indent.

> +		0x2e, 0x30, 0x32, 0x34, 0x36, 0x38, 0x3a, 0x3b, 0x3d, 0x3e,
> +		0x40, 0x41, 0x42, 0x43, 0x44, 0x46, 0x47, 0x48, 0x49, 0x4a,
> +		0x4b, 0x4c, 0x4d, 0x4e, 0x4e, 0x4f, 0x50, 0x51, 0x52, 0x53,
> +		0x53, 0x54, 0x55, 0x56, 0x56, 0x57, 0x58, 0x58, 0x59, 0x5a,
> +		0x5b, 0x5b, 0x5c, 0x5c, 0x5d, 0x5e, 0x5e, 0x5f, 0x60, 0x60,
> +		0x61, 0x61, 0x62, 0x62, 0x63, 0x63, 0x64, 0x65, 0x65, 0x66,
> +		0x66, 0x67, 0x67, 0x68, 0x68, 0x69, 0x69, 0x6a, 0x6a, 0x6b,
> +		0x6b, 0x6c, 0x6c, 0x6c, 0x6d, 0x6d, 0x6e, 0x6e, 0x6f, 0x6f,
> +		0x70, 0x70, 0x70, 0x71, 0x71, 0x72, 0x72, 0x73, 0x73, 0x73,
> +		0x74, 0x74, 0x75, 0x75, 0x75, 0x76, 0x76, 0x77, 0x77, 0x77,
> +		0x78, 0x78, 0x78, 0x79, 0x79, 0x7a, 0x7a, 0x7a, 0x7b, 0x7b,
> +		0x7b, 0x7c, 0x7c, 0x7c, 0x7d, 0x7d, 0x7d, 0x7e, 0x7e, 0x7e,
> +		0x7f, 0x7f };
> +
> +/*
> + * data that needs to be sent to device. sets up card internal stuff.
> + * values dumped from windows driver and filtered by trial'n'error.
> + */
> +static const struct {
> +	u8 type;
> +	u8 reg;
> +	u8 value;
> +} init_data[] = {

Ditto.

> +			{ 0x22, 0x00, 0x00 }, { 0x20, 0x00, 0x08 },
> +			{ 0x22, 0x01, 0x01 }, { 0x20, 0x01, 0x08 },
> +			{ 0x22, 0x02, 0x00 }, { 0x20, 0x02, 0x08 },
> +			{ 0x22, 0x03, 0x00 }, { 0x20, 0x03, 0x08 },
> +			{ 0x22, 0x04, 0x00 }, { 0x20, 0x04, 0x08 },
> +			{ 0x22, 0x05, 0x01 }, { 0x20, 0x05, 0x08 },
> +			{ 0x22, 0x04, 0x01 }, { 0x12, 0x04, 0x00 },
> +			{ 0x12, 0x05, 0x00 }, { 0x12, 0x0d, 0x78 },
> +			{ 0x12, 0x21, 0x82 }, { 0x12, 0x22, 0x80 },
> +			{ 0x12, 0x23, 0x00 }, { 0x12, 0x06, 0x02 },
> +			{ 0x12, 0x03, 0x00 }, { 0x12, 0x02, 0x00 },
> +			{ 0x22, 0x03, 0x01 },
> +			{ 0 } /* TERMINATING ENTRY */
> +};
> +
> 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-20 23:09:59.000000000 +0100
> +static const u8 BIT_REVERSE_TABLE[256] = { 0x00, 0x80, 0x40, 0xc0, 0x20, 0xa0,
> +		0x60, 0xe0, 0x10, 0x90, 0x50, 0xd0, 0x30, 0xb0, 0x70, 0xf0,

Ditto.

> +		0x08, 0x88, 0x48, 0xc8, 0x28, 0xa8, 0x68, 0xe8, 0x18, 0x98,
> +		0x58, 0xd8, 0x38, 0xb8, 0x78, 0xf8, 0x04, 0x84, 0x44, 0xc4,
> +		0x24, 0xa4, 0x64, 0xe4, 0x14, 0x94, 0x54, 0xd4, 0x34, 0xb4,
> +		0x74, 0xf4, 0x0c, 0x8c, 0x4c, 0xcc, 0x2c, 0xac, 0x6c, 0xec,
> +		0x1c, 0x9c, 0x5c, 0xdc, 0x3c, 0xbc, 0x7c, 0xfc, 0x02, 0x82,
> +		0x42, 0xc2, 0x22, 0xa2, 0x62, 0xe2, 0x12, 0x92, 0x52, 0xd2,
> +		0x32, 0xb2, 0x72, 0xf2, 0x0a, 0x8a, 0x4a, 0xca, 0x2a, 0xaa,
> +		0x6a, 0xea, 0x1a, 0x9a, 0x5a, 0xda, 0x3a, 0xba, 0x7a, 0xfa,
> +		0x06, 0x86, 0x46, 0xc6, 0x26, 0xa6, 0x66, 0xe6, 0x16, 0x96,
> +		0x56, 0xd6, 0x36, 0xb6, 0x76, 0xf6, 0x0e, 0x8e, 0x4e, 0xce,
> +		0x2e, 0xae, 0x6e, 0xee, 0x1e, 0x9e, 0x5e, 0xde, 0x3e, 0xbe,
> +		0x7e, 0xfe, 0x01, 0x81, 0x41, 0xc1, 0x21, 0xa1, 0x61, 0xe1,
> +		0x11, 0x91, 0x51, 0xd1, 0x31, 0xb1, 0x71, 0xf1, 0x09, 0x89,
> +		0x49, 0xc9, 0x29, 0xa9, 0x69, 0xe9, 0x19, 0x99, 0x59, 0xd9,
> +		0x39, 0xb9, 0x79, 0xf9, 0x05, 0x85, 0x45, 0xc5, 0x25, 0xa5,
> +		0x65, 0xe5, 0x15, 0x95, 0x55, 0xd5, 0x35, 0xb5, 0x75, 0xf5,
> +		0x0d, 0x8d, 0x4d, 0xcd, 0x2d, 0xad, 0x6d, 0xed, 0x1d, 0x9d,
> +		0x5d, 0xdd, 0x3d, 0xbd, 0x7d, 0xfd, 0x03, 0x83, 0x43, 0xc3,
> +		0x23, 0xa3, 0x63, 0xe3, 0x13, 0x93, 0x53, 0xd3, 0x33, 0xb3,
> +		0x73, 0xf3, 0x0b, 0x8b, 0x4b, 0xcb, 0x2b, 0xab, 0x6b, 0xeb,
> +		0x1b, 0x9b, 0x5b, 0xdb, 0x3b, 0xbb, 0x7b, 0xfb, 0x07, 0x87,
> +		0x47, 0xc7, 0x27, 0xa7, 0x67, 0xe7, 0x17, 0x97, 0x57, 0xd7,
> +		0x37, 0xb7, 0x77, 0xf7, 0x0f, 0x8f, 0x4f, 0xcf, 0x2f, 0xaf,
> +		0x6f, 0xef, 0x1f, 0x9f, 0x5f, 0xdf, 0x3f, 0xbf, 0x7f, 0xff };
> +
> +/*
> + * wMaxPacketSize of pcm endpoints.
> + * Order: ALT1EP2 ALT1EP6 ALT2EP2 ALT2EP6 ALT3EP2 ALT3EP6
> + * keep synced with RATES_IN_PACKET_SIZE and RATES_OUT_PACKET_SIZE in pcm.c
> + *
> + * CAUTION: keep sizeof <= buffer[] in usb6fire_fw_init
> + */
> +static const u16 EP_W_MAX_PACKET_SIZE[] = { 228, 228, 420, 420, 404, 604 };
> +
> +struct ihex_record {
> +	u16 address;
> +	u8 len;
> +	u8 data[256];
> +	char error; /* true if an error occured parsing this record */
> +
> +	u8 max_len; /* maximum record length in whole ihex */
> +
> +	/* private */
> +	const char *txt_data;
> +	unsigned int txt_length;
> +	unsigned int txt_offset; /* current position in txt_data */
> +};
> +
> +static u8 usb6fire_fw_ihex_nibble(const u8 n)
> +{
> +	if (n >= '0' && n <= '9')
> +		return n - '0';
> +	else if (n >= 'A' && n <= 'F')
> +		return n - ('A' - 10);
> +	else if (n >= 'a' && n <= 'f')
> +		return n - ('a' - 10);
> +	return 0;
> +}
> +static u8 usb6fire_fw_ihex_hex(const u8 *data, u8 *crc)
> +{
> +	u8 val = (usb6fire_fw_ihex_nibble(data[0]) << 4) |
> +			usb6fire_fw_ihex_nibble(data[1]);
> +	*crc += val;
> +	return val;
> +}
> +
> +/*
> + * returns true if record is available, false otherwise.
> + * iff an error occured, false will be returned and record->error will be true.
> + */
> +static bool usb6fire_fw_ihex_next_record(struct ihex_record *record)
> +{
> +	u8 crc = 0;
> +	u8 type;
> +	int i;
> +
> +	record->error = false;
> +
> +	/* find begin of record (marked by a colon) */
> +	while (record->txt_offset < record->txt_length
> +			&& record->txt_data[record->txt_offset] != ':')
> +		record->txt_offset++;
> +	if (record->txt_offset == record->txt_length)
> +		return false;
> +
> +	/* number of characters needed for len, addr and type entries */
> +	record->txt_offset++;
> +	if (record->txt_offset + 8 > record->txt_length) {
> +		record->error = true;
> +		return false;
> +	}
> +
> +	record->len = usb6fire_fw_ihex_hex(record->txt_data +
> +			record->txt_offset, &crc);
> +	record->txt_offset += 2;
> +	record->address = usb6fire_fw_ihex_hex(record->txt_data +
> +			record->txt_offset, &crc) << 8;
> +	record->txt_offset += 2;
> +	record->address |= usb6fire_fw_ihex_hex(record->txt_data +
> +			record->txt_offset, &crc);
> +	record->txt_offset += 2;
> +	type = usb6fire_fw_ihex_hex(record->txt_data +
> +			record->txt_offset, &crc);
> +	record->txt_offset += 2;
> +
> +	/* number of characters needed for data and crc entries */
> +	if (record->txt_offset + 2 * (record->len + 1) > record->txt_length) {
> +		record->error = true;
> +		return false;
> +	}
> +	for (i = 0; i < record->len; i++) {
> +		record->data[i] = usb6fire_fw_ihex_hex(record->txt_data
> +				+ record->txt_offset, &crc);
> +		record->txt_offset += 2;
> +	}
> +	usb6fire_fw_ihex_hex(record->txt_data + record->txt_offset, &crc);
> +	if (crc) {
> +		record->error = true;
> +		return false;
> +	}
> +
> +	if (type == 1 || !record->len) /* eof */
> +		return false;
> +	else if (type == 0)
> +		return true;
> +	else {
> +		record->error = true;
> +		return false;
> +	}
> +}
> +
> +static int usb6fire_fw_ihex_init(const struct firmware *fw,
> +		struct ihex_record *record)
> +{
> +	record->txt_data = fw->data;
> +	record->txt_length = fw->size;
> +	record->txt_offset = 0;
> +	record->max_len = 0;
> +	/* read all records, if loop ends, record->error indicates,
> +	 * whether ihex is valid. */
> +	while (usb6fire_fw_ihex_next_record(record))
> +		record->max_len = max(record->len, record->max_len);
> +	if (record->error)
> +		return -EINVAL;
> +	record->txt_offset = 0;
> +	return 0;
> +}

I found that the recent kernel has linux/ihex.h helper functions.
They might help for simplifying the code.  But, this can be done later
as a a clean-up.

> 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-20 23:07:24.000000000 +0100
...
> +/* keep next two synced with
> + * FW_EP_W_MAX_PACKET_SIZE[] and RATES_MAX_PACKET_SIZE */
> +static const int RATES_IN_PACKET_SIZE[] = { 228, 228, 420, 420, 404, 404 };
> +static const int RATES_OUT_PACKET_SIZE[] = { 228, 228, 420, 420, 604, 604 };
> +static const int RATES[] = { 44100, 48000, 88200, 96000, 176400, 192000 };

Any reason to use capital letters for these?
Because they are const?

> +static const int RATES_ALSAID[] = { SNDRV_PCM_RATE_44100, SNDRV_PCM_RATE_48000,

Begin the data in the open line, and use a proper indent.

> +		SNDRV_PCM_RATE_88200, SNDRV_PCM_RATE_96000,
> +		SNDRV_PCM_RATE_176400, SNDRV_PCM_RATE_192000 };
> +static const int RATES_ALTSETTING[] = { 1, 1, 2, 2, 3, 3 };
> +
> +/* values to write to soundcard register for all samplerates */
> +static const __u16 RATES_6FIRE_VL[] = {0x00, 0x01, 0x00, 0x01, 0x00, 0x01};
> +static const __u16 RATES_6FIRE_VH[] = {0x11, 0x11, 0x10, 0x10, 0x00, 0x00};

Use u16.

> 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-20 23:25:34.000000000 +0100
> @@ -32,6 +32,19 @@
>  	  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

Why x86-dependent?


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Driver for TerraTec DMX 6Fire USB

[Torsten Schenk]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

Hello,

Thanks very much for the review. I corrected the issues you wrote about (except the last one, see below). checkpatch.pl reports now 0 errors and warnings.

Another thing about the driver's features:
I'm currently in contact with a guy that also owns a TT DMX6FireUSB. He says, firmware is not loading for him. I'm trying to figure out how to make
it work, but I already know, that my driver cannot provide fw uploading for every device out there by now. I think it's just a matter of removing some checks, but I want to make as sure as I can before I just do so.

If the device was previously used in windows and not unplugged from power, it will work also with no firmware upload.

>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.
I though so. checkpatch.pl is a useful script, good to know it exists.

>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. 
How is it supposed to be? I reviewed my code and added a prefix to every function. Hopefully
it didn't get too long now... Some, especially functions with not much content, seem to have a too long name by now for my flavour. 

>NULLs are automatically filled, so these can be skipped. 
Which NULLs are automatically filled? Just the area create by snd_card_create or
every area allocated via kmalloc? If last is true, I will have to remove some more NULLs.

>Don't use own bool type. Use the generic "bool" type, and use 
>true/false instead of TRUE/FALSE (unless the size really matters). 
>Use standard min() / max() macros. 
>Use u8 and u16. 
Didn't know these exists...
When I used C once, 'bool' and also 'min'/'max' weren't available.

>Remove all, clean, install things. 
Just forgot ;)

>Do you really need hwdep? 
>Rather select FW_LOADER, if any. 
> 
>> +    select SND_PCM 
Thanks, just copied this from another definition without exactly knowing what it does...
> 
>Missing SND_RAWMIDI dependency. 
Realized that after posting.

> 
> 
>> @@ -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 ;) 
I'd love not to ;) But how can I remove it? It's automatically generated by diff. Perhaps wrong arguments? Mine are -Nur
Or should I simply remove these lines with a text editor?

Greetings,
Torsten

[-- Attachment #2: usb6fire.tar.bz2 --]
[-- Type: application/octet-stream, Size: 13210 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: 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@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