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

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