Re: [alsa-devel] Driver for TerraTec DMX 6Fire USB
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
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
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(®ister_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(®ister_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
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
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
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
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
Hello everyone,
The driver I wrote for the 6fire usb is now in use by some people so that I got feedback. This enabled me to improve and bugfix some things. Here the patch.
Greets, Torsten
At Thu, 31 Mar 2011 21:25:23 +0200, Torsten Schenk wrote:
Hello everyone,
The driver I wrote for the 6fire usb is now in use by some people so that I got feedback. This enabled me to improve and bugfix some things. Here the patch.
Could you give a bit more information, what does your patch fix exactly? This must be written in the changelog.
thanks,
Takashi
+static int usb6fire_control_digital_thru_info(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_info *uinfo)+{
- uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
- uinfo->count = 1;
- uinfo->value.integer.min = 0;
- uinfo->value.integer.max = 1;
- return 0;
+}
Use the common snd_ctl_boolean_mono_info() instead.
+/* check, if the firmware version the devices has currently loaded
- is known by this driver. 'version' needs to have 4 bytes version
- info data. */
+static int usb6fire_fw_check(u8 *version) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(known_fw_versions); i++)
if (!memcmp(version, known_fw_versions, 4))
No increment or indexing (although there is only one element for now)?
thanks,
Takashi
Ok, fixed your two issues. Here the changelog and the patch again:
- control: digital thru added (for conversion optical<->tos) - restructuring: samplerate, channel and streaming control moved to control.c - firmware loader: loader now accepts all hardware versions of the device - firmware loader: if firmware is already present it will be accepted if it is known to be working - pcm: added support for s32_le sampleformat - makefile: "experimental" dependency removed (tested and confirmed by users on http://sixfireusb.sourceforge.net) - makefile: description updated - bugfix: unsigned pcm_runtime.rate now never uses negative values (completion of signedness bug correction submitted by Dan Carpenter) - bugfix: firmware leak fixed if error occurs (already submitted by Jesper Juhl but not in kernel yet)
Thanks Torsten
At Sun, 03 Apr 2011 14:30:43 +0200, Torsten Schenk wrote:
Ok, fixed your two issues. Here the changelog and the patch again:
- control: digital thru added (for conversion optical<->tos)
- restructuring: samplerate, channel and streaming control moved to control.c
- firmware loader: loader now accepts all hardware versions of the device
- firmware loader: if firmware is already present it will be accepted if it is known to be working
- pcm: added support for s32_le sampleformat
- makefile: "experimental" dependency removed (tested and confirmed by users on http://sixfireusb.sourceforge.net)
- makefile: description updated
- bugfix: unsigned pcm_runtime.rate now never uses negative values (completion of signedness bug correction submitted by Dan Carpenter)
- bugfix: firmware leak fixed if error occurs (already submitted by Jesper Juhl but not in kernel yet)
Thanks. But, as you see, there are too many changes done in a single patch. Could you split the changes into several incremental patches? In principle, one patch should contain one change. Of course, some trivial changes can be merged, but the above is way too much.
Takashi
Thanks. But, as you see, there are too many changes done in a single patch. Could you split the changes into several incremental patches? In principle, one patch should contain one change. Of course, some trivial changes can be merged, but the above is way too much.
Ok, got how it's done.
For the following patches be working correctly, please apply the patch previously supplied by Jesper Juhl ("Don't leak firmware when usb6fire_fw_ihex_init() fails.") before if missing... I downloaded the current kernel and this patch didn't seem to be applied.
Thanks, Torsten
Kernel configuration updated: - experimental dependency removed - description updated
Signed-off-by: Torsten Schenk torsten.schenk@zoho.com
diff -Nur a/sound/usb/Kconfig b/sound/usb/Kconfig --- a/sound/usb/Kconfig 2011-03-31 21:20:22.000000000 +0200 +++ b/sound/usb/Kconfig 2011-03-31 21:07:57.000000000 +0200 @@ -100,7 +100,6 @@
config SND_USB_6FIRE tristate "TerraTec DMX 6Fire USB" - depends on EXPERIMENTAL select FW_LOADER select SND_RAWMIDI select SND_PCM @@ -111,5 +111,3 @@ - after it has been coldstarted. This driver currently does not support - firmware loading for all devices. If you own such a device, - you could start windows and let the windows driver upload - the firmware. As long as you do not unplug your device from power, - it should be usable. + after it has been coldstarted. An install script for the firmware + and further help can be found at + http://sixfireusb.sourceforge.net
At Mon, 04 Apr 2011 11:45:28 +0200, Torsten Schenk wrote:
Kernel configuration updated:
- experimental dependency removed
- description updated
Signed-off-by: Torsten Schenk torsten.schenk@zoho.com
Thanks, now applied all 5 patches to sound git tree (in topic/misc branch).
At the next time, please give more appropriate summary to each patch corresponding to the content.
Takashi
diff -Nur a/sound/usb/Kconfig b/sound/usb/Kconfig --- a/sound/usb/Kconfig 2011-03-31 21:20:22.000000000 +0200 +++ b/sound/usb/Kconfig 2011-03-31 21:07:57.000000000 +0200 @@ -100,7 +100,6 @@
config SND_USB_6FIRE tristate "TerraTec DMX 6Fire USB"
depends on EXPERIMENTAL select FW_LOADER select SND_RAWMIDI select SND_PCM@@ -111,5 +111,3 @@
after it has been coldstarted. This driver currently does not supportfirmware loading for all devices. If you own such a device,you could start windows and let the windows driver uploadthe firmware. As long as you do not unplug your device from power,it should be usable.
after it has been coldstarted. An install script for the firmwareand further help can be found athttp://sixfireusb.sourceforge.net
Completion of signedness bug for pcm_runtime.rate: variable will never get assigned a negative value now.
Signed-off-by: Torsten Schenk torsten.schenk@zoho.com
diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c --- a/sound/usb/6fire/pcm.c 2011-04-04 11:17:56.469832999 +0200 +++ b/sound/usb/6fire/pcm.c 2011-04-04 11:23:12.993833002 +0200 @@ -456,7 +456,7 @@ /* all substreams closed? if so, stop streaming */ if (!rt->playback.instance && !rt->capture.instance) { usb6fire_pcm_stream_stop(rt); - rt->rate = -1; + rt->rate = ARRAY_SIZE(rates); } } mutex_unlock(&rt->stream_mutex); @@ -480,7 +480,6 @@ struct pcm_runtime *rt = snd_pcm_substream_chip(alsa_sub); struct pcm_substream *sub = usb6fire_pcm_get_substream(alsa_sub); struct snd_pcm_runtime *alsa_rt = alsa_sub->runtime; - int i; int ret;
if (rt->panic) @@ -493,12 +492,10 @@ sub->period_off = 0;
if (rt->stream_state == STREAM_DISABLED) { - for (i = 0; i < ARRAY_SIZE(rates); i++) - if (alsa_rt->rate == rates[i]) { - rt->rate = i; + for (rt->rate = 0; rt->rate < ARRAY_SIZE(rates); rt->rate++) + if (alsa_rt->rate == rates[rt->rate]) break; - } - if (i == ARRAY_SIZE(rates)) { + if (rt->rate == ARRAY_SIZE(rates)) { mutex_unlock(&rt->stream_mutex); snd_printk("invalid rate %d in prepare.\n", alsa_rt->rate); @@ -613,7 +610,7 @@
rt->chip = chip; rt->stream_state = STREAM_DISABLED; - rt->rate = -1; + rt->rate = ARRAY_SIZE(rates); init_waitqueue_head(&rt->stream_wait_queue); mutex_init(&rt->stream_mutex);
Added support for sample format s32_le.
Signed-off-by: Torsten Schenk torsten.schenk@zoho.com
diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c --- a/sound/usb/6fire/pcm.c 2011-04-04 11:23:42.102833002 +0200 +++ b/sound/usb/6fire/pcm.c 2011-04-04 11:23:10.342833002 +0200 @@ -64,7 +64,7 @@ SNDRV_PCM_INFO_MMAP_VALID | SNDRV_PCM_INFO_BATCH,
- .formats = SNDRV_PCM_FMTBIT_S24_LE, + .formats = SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE,
.rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 | @@ -228,7 +228,7 @@ unsigned int total_length = 0; struct pcm_runtime *rt = snd_pcm_substream_chip(sub->instance); struct snd_pcm_runtime *alsa_rt = sub->instance->runtime; - u32 *src = (u32 *) urb->buffer; + u32 *src = NULL; u32 *dest = (u32 *) (alsa_rt->dma_area + sub->dma_off * (alsa_rt->frame_bits >> 3)); u32 *dest_end = (u32 *) (alsa_rt->dma_area + alsa_rt->buffer_size @@ -244,7 +244,12 @@ else frame_count = 0;
- src = (u32 *) (urb->buffer + total_length); + if (alsa_rt->format == SNDRV_PCM_FORMAT_S24_LE) + src = (u32 *) (urb->buffer + total_length); + else if (alsa_rt->format == SNDRV_PCM_FORMAT_S32_LE) + src = (u32 *) (urb->buffer - 1 + total_length); + else + return; src++; /* skip leading 4 bytes of every packet */ total_length += urb->packets[i].length; for (frame = 0; frame < frame_count; frame++) { @@ -274,9 +279,18 @@ * (alsa_rt->frame_bits >> 3)); u32 *src_end = (u32 *) (alsa_rt->dma_area + alsa_rt->buffer_size * (alsa_rt->frame_bits >> 3)); - u32 *dest = (u32 *) urb->buffer; + u32 *dest; int bytes_per_frame = alsa_rt->channels << 2;
+ if (alsa_rt->format == SNDRV_PCM_FORMAT_S32_LE) + dest = (u32 *) (urb->buffer - 1); + else if (alsa_rt->format == SNDRV_PCM_FORMAT_S24_LE) + dest = (u32 *) (urb->buffer); + else { + snd_printk(KERN_ERR PREFIX "Unknown sample format."); + return; + } + for (i = 0; i < PCM_N_PACKETS_PER_URB; i++) { /* at least 4 header bytes for valid packet. * after that: 32 bits per sample for analog channels */
Firmware loader: magical device bytes check updated (accepts all device versions now and accepts possibly loaded firmware, if it is knowing to be working)
Signed-off-by: Torsten Schenk torsten.schenk@zoho.com
diff -Nur a/sound/usb/6fire/firmware.c b/sound/usb/6fire/firmware.c --- a/sound/usb/6fire/firmware.c 2011-04-04 11:26:54.528833002 +0200 +++ b/sound/usb/6fire/firmware.c 2011-04-04 11:25:27.014833002 +0200 @@ -3,12 +3,6 @@ * * Firmware loader * - * Currently not working for all devices. To be able to use the device - * in linux, it is also possible to let the windows driver upload the firmware. - * For that, start the computer in windows and reboot. - * As long as the device is connected to the power supply, no firmware reload - * needs to be performed. - * * Author: Torsten Schenk torsten.schenk@zoho.com * Created: Jan 01, 2011 * Version: 0.3.0 @@ -72,6 +66,10 @@ 0x94, 0x01, 0x5c, 0x02 /* alt 3: 404 EP2 and 604 EP6 (25 fpp) */ };
+static const u8 known_fw_versions[][4] = { + { 0x03, 0x01, 0x0b, 0x00 } +}; + struct ihex_record { u16 address; u8 len; @@ -364,6 +362,25 @@ return 0; }
+/* check, if the firmware version the devices has currently loaded + * is known by this driver. 'version' needs to have 4 bytes version + * info data. */ +static int usb6fire_fw_check(u8 *version) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(known_fw_versions); i++) + if (!memcmp(version, known_fw_versions + i, 4)) + return 0; + + snd_printk(KERN_ERR PREFIX "invalid fimware version in device: " + "%02x %02x %02x %02x. " + "please reconnect to power. if this failure " + "still happens, check your firmware installation.", + version[0], version[1], version[2], version[3]); + return -EINVAL; +} + int usb6fire_fw_init(struct usb_interface *intf) { int i; @@ -379,9 +396,7 @@ "firmware state.\n"); return ret; } - if (buffer[0] != 0xeb || buffer[1] != 0xaa || buffer[2] != 0x55 - || buffer[4] != 0x03 || buffer[5] != 0x01 || buffer[7] - != 0x00) { + if (buffer[0] != 0xeb || buffer[1] != 0xaa || buffer[2] != 0x55) { snd_printk(KERN_ERR PREFIX "unknown device firmware state " "received from device: "); for (i = 0; i < 8; i++) @@ -390,7 +405,7 @@ return -EIO; } /* do we need fpga loader ezusb firmware? */ - if (buffer[3] == 0x01 && buffer[6] == 0x19) { + if (buffer[3] == 0x01) { ret = usb6fire_fw_ezusb_upload(intf, "6fire/dmx6firel2.ihx", 0, NULL, 0); if (ret < 0) @@ -398,7 +413,10 @@ return FW_NOT_READY; } /* do we need fpga firmware and application ezusb firmware? */ - else if (buffer[3] == 0x02 && buffer[6] == 0x0b) { + else if (buffer[3] == 0x02) { + ret = usb6fire_fw_check(buffer + 4); + if (ret < 0) + return ret; ret = usb6fire_fw_fpga_upload(intf, "6fire/dmx6firecf.bin"); if (ret < 0) return ret; @@ -411,8 +429,8 @@ return FW_NOT_READY; } /* all fw loaded? */ - else if (buffer[3] == 0x03 && buffer[6] == 0x0b) - return 0; + else if (buffer[3] == 0x03) + return usb6fire_fw_check(buffer + 4); /* unknown data? */ else { snd_printk(KERN_ERR PREFIX "unknown device firmware state "
Digital Thru mixer element added (device can act as converter optical<->coax)
Signed-off-by: Torsten Schenk torsten.schenk@zoho.com
diff -Nur a/sound/usb/6fire/control.c b/sound/usb/6fire/control.c --- a/sound/usb/6fire/control.c 2011-03-31 21:20:22.000000000 +0200 +++ b/sound/usb/6fire/control.c 2011-04-04 11:30:05.664833001 +0200 @@ -65,6 +65,15 @@ { 0 } /* TERMINATING ENTRY */ };
+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}; + +enum { + DIGITAL_THRU_ONLY_SAMPLERATE = 3 +}; + static void usb6fire_control_master_vol_update(struct control_runtime *rt) { struct comm_runtime *comm_rt = rt->chip->comm; @@ -95,6 +104,67 @@ } }
+static int usb6fire_control_set_rate(struct control_runtime *rt, int rate) +{ + int ret; + struct usb_device *device = rt->chip->dev; + struct comm_runtime *comm_rt = rt->chip->comm; + + if (rate < 0 || rate >= CONTROL_N_RATES) + return -EINVAL; + + ret = usb_set_interface(device, 1, rates_altsetting[rate]); + if (ret < 0) + return ret; + + /* set soundcard clock */ + ret = comm_rt->write16(comm_rt, 0x02, 0x01, rates_6fire_vl[rate], + rates_6fire_vh[rate]); + if (ret < 0) + return ret; + + return 0; +} + +static int usb6fire_control_set_channels( + struct control_runtime *rt, int n_analog_out, + int n_analog_in, bool spdif_out, bool spdif_in) +{ + int ret; + struct comm_runtime *comm_rt = rt->chip->comm; + + /* enable analog inputs and outputs + * (one bit per stereo-channel) */ + ret = comm_rt->write16(comm_rt, 0x02, 0x02, + (1 << (n_analog_out / 2)) - 1, + (1 << (n_analog_in / 2)) - 1); + if (ret < 0) + return ret; + + /* disable digital inputs and outputs */ + /* TODO: use spdif_x to enable/disable digital channels */ + ret = comm_rt->write16(comm_rt, 0x02, 0x03, 0x00, 0x00); + if (ret < 0) + return ret; + + return 0; +} + +static int usb6fire_control_streaming_update(struct control_runtime *rt) +{ + struct comm_runtime *comm_rt = rt->chip->comm; + + if (comm_rt) { + if (!rt->usb_streaming && rt->digital_thru_switch) + usb6fire_control_set_rate(rt, + DIGITAL_THRU_ONLY_SAMPLERATE); + return comm_rt->write16(comm_rt, 0x02, 0x00, 0x00, + (rt->usb_streaming ? 0x01 : 0x00) | + (rt->digital_thru_switch ? 0x08 : 0x00)); + } + return -EINVAL; +} + static int usb6fire_control_master_vol_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) { @@ -195,6 +265,28 @@ return 0; }
+static int usb6fire_control_digital_thru_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct control_runtime *rt = snd_kcontrol_chip(kcontrol); + int changed = 0; + + if (rt->digital_thru_switch != ucontrol->value.integer.value[0]) { + rt->digital_thru_switch = ucontrol->value.integer.value[0]; + usb6fire_control_streaming_update(rt); + changed = 1; + } + return changed; +} + +static int usb6fire_control_digital_thru_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct control_runtime *rt = snd_kcontrol_chip(kcontrol); + ucontrol->value.integer.value[0] = rt->digital_thru_switch; + return 0; +} + static struct __devinitdata snd_kcontrol_new elements[] = { { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, @@ -223,6 +315,15 @@ .get = usb6fire_control_opt_coax_get, .put = usb6fire_control_opt_coax_put }, + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Digital Thru Playback Route", + .index = 0, + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE, + .info = snd_ctl_boolean_mono_info, + .get = usb6fire_control_digital_thru_get, + .put = usb6fire_control_digital_thru_put + }, {} };
@@ -238,6 +339,9 @@ return -ENOMEM;
rt->chip = chip; + rt->update_streaming = usb6fire_control_streaming_update; + rt->set_rate = usb6fire_control_set_rate; + rt->set_channels = usb6fire_control_set_channels;
i = 0; while (init_data[i].type) { @@ -249,6 +353,7 @@ usb6fire_control_opt_coax_update(rt); usb6fire_control_line_phono_update(rt); usb6fire_control_master_vol_update(rt); + usb6fire_control_streaming_update(rt);
i = 0; while (elements[i].name) { diff -Nur a/sound/usb/6fire/control.h b/sound/usb/6fire/control.h --- a/sound/usb/6fire/control.h 2011-03-31 21:20:22.000000000 +0200 +++ b/sound/usb/6fire/control.h 2011-04-04 11:30:10.821833002 +0200 @@ -21,12 +21,29 @@ CONTROL_MAX_ELEMENTS = 32 };
+enum { + CONTROL_RATE_44KHZ, + CONTROL_RATE_48KHZ, + CONTROL_RATE_88KHZ, + CONTROL_RATE_96KHZ, + CONTROL_RATE_176KHZ, + CONTROL_RATE_192KHZ, + CONTROL_N_RATES +}; + struct control_runtime { + int (*update_streaming)(struct control_runtime *rt); + int (*set_rate)(struct control_runtime *rt, int rate); + int (*set_channels)(struct control_runtime *rt, int n_analog_out, + int n_analog_in, bool spdif_out, bool spdif_in); + struct sfire_chip *chip;
struct snd_kcontrol *element[CONTROL_MAX_ELEMENTS]; bool opt_coax_switch; bool line_phono_switch; + bool digital_thru_switch; + bool usb_streaming; u8 master_vol; };
diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c --- a/sound/usb/6fire/pcm.c 2011-04-04 11:23:10.000000000 +0200 +++ b/sound/usb/6fire/pcm.c 2011-04-04 11:30:15.623833002 +0200 @@ -17,26 +17,23 @@ #include "pcm.h" #include "chip.h" #include "comm.h" +#include "control.h"
enum { OUT_N_CHANNELS = 6, IN_N_CHANNELS = 4 };
/* keep next two synced with - * FW_EP_W_MAX_PACKET_SIZE[] and RATES_MAX_PACKET_SIZE */ + * FW_EP_W_MAX_PACKET_SIZE[] and RATES_MAX_PACKET_SIZE + * and CONTROL_RATE_XXX in control.h */ 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 }; -static const int rates_altsetting[] = { 1, 1, 2, 2, 3, 3 }; static const int rates_alsaid[] = { SNDRV_PCM_RATE_44100, SNDRV_PCM_RATE_48000, SNDRV_PCM_RATE_88200, SNDRV_PCM_RATE_96000, SNDRV_PCM_RATE_176400, SNDRV_PCM_RATE_192000 };
-/* 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}; - enum { /* settings for pcm */ OUT_EP = 6, IN_EP = 2, MAX_BUFSIZE = 128 * 1024 }; @@ -48,15 +45,6 @@ STREAM_STOPPING };
-enum { /* pcm sample rates (also index into RATES_XXX[]) */ - RATE_44KHZ, - RATE_48KHZ, - RATE_88KHZ, - RATE_96KHZ, - RATE_176KHZ, - RATE_192KHZ -}; - static const struct snd_pcm_hardware pcm_hw = { .info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED | @@ -87,57 +75,34 @@ static int usb6fire_pcm_set_rate(struct pcm_runtime *rt) { int ret; - struct usb_device *device = rt->chip->dev; - struct comm_runtime *comm_rt = rt->chip->comm; + struct control_runtime *ctrl_rt = rt->chip->control;
- if (rt->rate >= ARRAY_SIZE(rates)) - return -EINVAL; - /* disable streaming */ - ret = comm_rt->write16(comm_rt, 0x02, 0x00, 0x00, 0x00); + ctrl_rt->usb_streaming = false; + ret = ctrl_rt->update_streaming(ctrl_rt); if (ret < 0) { snd_printk(KERN_ERR PREFIX "error stopping streaming while " "setting samplerate %d.\n", rates[rt->rate]); return ret; }
- ret = usb_set_interface(device, 1, rates_altsetting[rt->rate]); - if (ret < 0) { - snd_printk(KERN_ERR PREFIX "error setting interface " - "altsetting %d for samplerate %d.\n", - rates_altsetting[rt->rate], rates[rt->rate]); - return ret; - } - - /* set soundcard clock */ - ret = comm_rt->write16(comm_rt, 0x02, 0x01, rates_6fire_vl[rt->rate], - rates_6fire_vh[rt->rate]); + ret = ctrl_rt->set_rate(ctrl_rt, rt->rate); if (ret < 0) { snd_printk(KERN_ERR PREFIX "error setting samplerate %d.\n", rates[rt->rate]); return ret; }
- /* enable analog inputs and outputs - * (one bit per stereo-channel) */ - ret = comm_rt->write16(comm_rt, 0x02, 0x02, - (1 << (OUT_N_CHANNELS / 2)) - 1, - (1 << (IN_N_CHANNELS / 2)) - 1); + ret = ctrl_rt->set_channels(ctrl_rt, OUT_N_CHANNELS, IN_N_CHANNELS, + false, false); if (ret < 0) { - snd_printk(KERN_ERR PREFIX "error initializing analog channels " + snd_printk(KERN_ERR PREFIX "error initializing channels " "while setting samplerate %d.\n", rates[rt->rate]); return ret; } - /* disable digital inputs and outputs */ - ret = comm_rt->write16(comm_rt, 0x02, 0x03, 0x00, 0x00); - if (ret < 0) { - snd_printk(KERN_ERR PREFIX "error initializing digital " - "channels while setting samplerate %d.\n", - rates[rt->rate]); - return ret; - }
- ret = comm_rt->write16(comm_rt, 0x02, 0x00, 0x00, 0x01); + ctrl_rt->usb_streaming = true; + ret = ctrl_rt->update_streaming(ctrl_rt); if (ret < 0) { snd_printk(KERN_ERR PREFIX "error starting streaming while " "setting samplerate %d.\n", rates[rt->rate]); @@ -168,12 +133,15 @@ static void usb6fire_pcm_stream_stop(struct pcm_runtime *rt) { int i; + struct control_runtime *ctrl_rt = rt->chip->control;
if (rt->stream_state != STREAM_DISABLED) { for (i = 0; i < PCM_N_URBS; i++) { usb_kill_urb(&rt->in_urbs[i].instance); usb_kill_urb(&rt->out_urbs[i].instance); } + ctrl_rt->usb_streaming = false; + ctrl_rt->update_streaming(ctrl_rt); rt->stream_state = STREAM_DISABLED; } }
Fixed remaining issues of the signedness bug discovered by Dan Carpenter. A check was remaining that tests if unsigned rt->rate is >= 0. Changed that so that rt->rate now consistently uses ARRAY_SIZE(rates) as invalid rate value and not -1.
Signed-off-by: Torsten Schenk torsten.schenk@zoho.com
diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c --- a/sound/usb/6fire/pcm.c 2011-06-16 20:49:55.859600014 +0200 +++ b/sound/usb/6fire/pcm.c 2011-06-16 20:48:07.000000000 +0200 @@ -395,12 +395,12 @@ alsa_rt->hw = pcm_hw;
if (alsa_sub->stream == SNDRV_PCM_STREAM_PLAYBACK) { - if (rt->rate >= 0) + if (rt->rate < ARRAY_SIZE(rates)) alsa_rt->hw.rates = rates_alsaid[rt->rate]; alsa_rt->hw.channels_max = OUT_N_CHANNELS; sub = &rt->playback; } else if (alsa_sub->stream == SNDRV_PCM_STREAM_CAPTURE) { - if (rt->rate >= 0) + if (rt->rate < ARRAY_SIZE(rates)) alsa_rt->hw.rates = rates_alsaid[rt->rate]; alsa_rt->hw.channels_max = IN_N_CHANNELS; sub = &rt->capture;
At Thu, 16 Jun 2011 21:06:27 +0200, Torsten Schenk wrote:
Fixed remaining issues of the signedness bug discovered by Dan Carpenter. A check was remaining that tests if unsigned rt->rate is >= 0. Changed that so that rt->rate now consistently uses ARRAY_SIZE(rates) as invalid rate value and not -1.
Signed-off-by: Torsten Schenk torsten.schenk@zoho.com
Thanks, applied now. (But at the next, please give a bit better patch subject :)
Takashi
diff -Nur a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c --- a/sound/usb/6fire/pcm.c 2011-06-16 20:49:55.859600014 +0200 +++ b/sound/usb/6fire/pcm.c 2011-06-16 20:48:07.000000000 +0200 @@ -395,12 +395,12 @@ alsa_rt->hw = pcm_hw;
if (alsa_sub->stream == SNDRV_PCM_STREAM_PLAYBACK) {
if (rt->rate >= 0)
if (rt->rate < ARRAY_SIZE(rates)) alsa_rt->hw.rates = rates_alsaid[rt->rate];
if (rt->rate >= 0)
if (rt->rate < ARRAY_SIZE(rates)) alsa_rt->hw.rates = rates_alsaid[rt->rate];
participants (2)
-
Takashi Iwai -
Torsten Schenk