[alsa-devel] [PATCH v2] MIDI driver for Behringer BCD2000 USB device
This patch adds initial support for the Behringer BCD2000 USB DJ controller. At the moment, only the MIDI part of the device is working, i.e. knobs, buttons and LEDs.
I also plan to add support for the audio part, but I assume that this will require more effort than the rather simple MIDI interface. Progress can be tracked at https://github.com/anyc/snd-usb-bcd2000.
Changes since v1: - fixed the various code style issues, thanks to Daniel Mack and checkpatch.pl.
Signed-off-by: Mario Kicherer dev@kicherer.org --- sound/usb/Kconfig | 14 ++ sound/usb/Makefile | 2 +- sound/usb/bcd2000/Makefile | 3 + sound/usb/bcd2000/bcd2000.c | 553 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 571 insertions(+), 1 deletion(-) create mode 100644 sound/usb/bcd2000/Makefile create mode 100644 sound/usb/bcd2000/bcd2000.c
diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig index de9408b..2abafea 100644 --- a/sound/usb/Kconfig +++ b/sound/usb/Kconfig @@ -146,5 +146,19 @@ config SND_USB_HIFACE To compile this driver as a module, choose M here: the module will be called snd-usb-hiface.
+config SND_USB_BCD2000 + tristate "Behringer BCD2000 MIDI driver" + select SND_HWDEP + select SND_RAWMIDI + help + Say Y here to include MIDI support for the Behringer BCD2000 DJ + controller. + + Audio support is still work-in-progress at + https://github.com/anyc/snd-usb-bcd2000 + + To compile this driver as a module, choose M here: the module + will be called snd-usb-bcd2000. + endif # SND_USB
diff --git a/sound/usb/Makefile b/sound/usb/Makefile index abe668f..2b92f0d 100644 --- a/sound/usb/Makefile +++ b/sound/usb/Makefile @@ -23,4 +23,4 @@ obj-$(CONFIG_SND_USB_UA101) += snd-usbmidi-lib.o obj-$(CONFIG_SND_USB_USX2Y) += snd-usbmidi-lib.o obj-$(CONFIG_SND_USB_US122L) += snd-usbmidi-lib.o
-obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/ diff --git a/sound/usb/bcd2000/Makefile b/sound/usb/bcd2000/Makefile new file mode 100644 index 0000000..6540136 --- /dev/null +++ b/sound/usb/bcd2000/Makefile @@ -0,0 +1,3 @@ +snd-usb-bcd2000-y := bcd2000.o + +obj-$(CONFIG_SND_USB_BCD2000) += snd-usb-bcd2000.o \ No newline at end of file diff --git a/sound/usb/bcd2000/bcd2000.c b/sound/usb/bcd2000/bcd2000.c new file mode 100644 index 0000000..d8c5525 --- /dev/null +++ b/sound/usb/bcd2000/bcd2000.c @@ -0,0 +1,553 @@ +/* + * Behringer BCD2000 driver + * + * Copyright (C) 2014 Mario Kicherer (dev@kicherer.org) + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/usb.h> +#include <linux/usb/audio.h> +#include <sound/core.h> +#include <sound/initval.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/rawmidi.h> +#include "../usbaudio.h" +#include "../midi.h" + +#define DEVICE_NAME "Behringer BCD2000" +#define DEVICE_SHORTNAME "bcd2000" + +#define PREFIX DEVICE_SHORTNAME ": " +#define BUFSIZE 64 + +static struct usb_device_id id_table[] = { + { USB_DEVICE(0x1397, 0x00bd) }, + { }, +}; + +static int debug; +module_param(debug, int, S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(debug, "enable debug output (default: 0)"); + +static unsigned char device_cmd_prefix[] = {0x03, 0x00}; + +static unsigned char bcd2000_init_sequence[] = { + 0x07, 0x00, 0x00, 0x00, 0x78, 0x48, 0x1c, 0x81, + 0xc4, 0x00, 0x00, 0x00, 0x5e, 0x53, 0x4a, 0xf7, + 0x18, 0xfa, 0x11, 0xff, 0x6c, 0xf3, 0x90, 0xff, + 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, + 0x18, 0xfa, 0x11, 0xff, 0x14, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0xf2, 0x34, 0x4a, 0xf7, + 0x18, 0xfa, 0x11, 0xff}; + +struct bcd2000 { + struct usb_device *dev; + struct snd_card *card; + struct usb_interface *intf; + int card_index; + struct snd_pcm *pcm; + struct list_head midi_list; + spinlock_t lock; + struct mutex mutex; + + int midi_out_active; + struct snd_rawmidi *rmidi; + struct snd_rawmidi_substream *midi_receive_substream; + struct snd_rawmidi_substream *midi_out_substream; + + unsigned char midi_in_buf[BUFSIZE]; + unsigned char midi_out_buf[BUFSIZE]; + + unsigned char midi_cmd_buf[3]; + unsigned char midi_cmd_offset; + + struct urb *midi_out_urb; + struct urb *midi_in_urb; +}; + +static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX; +static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR; +static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP; + +static DEFINE_MUTEX(devices_mutex); +static unsigned int devices_used; +static struct usb_driver bcd2000_driver; + + + + + + +static int bcd2000_midi_input_open(struct snd_rawmidi_substream *substream) +{ + return 0; +} + +static int bcd2000_midi_input_close(struct snd_rawmidi_substream *substream) +{ + return 0; +} + +static void bcd2000_midi_input_trigger(struct snd_rawmidi_substream *substream, + int up) +{ + struct bcd2000 *bcd2k = substream->rmidi->private_data; + + if (!bcd2k) + return; + + bcd2k->midi_receive_substream = up ? substream : NULL; +} + +static void bcd2000_midi_handle_input(struct bcd2000 *bcd2k, + const unsigned char *buf, int len) +{ + unsigned char length, cmd, buf_offset, tocopy; + + if (!bcd2k->midi_receive_substream) + return; + + if (debug) { + print_hex_dump(KERN_DEBUG, "received from device: ", + DUMP_PREFIX_NONE, 16, 1, buf, len, false); + } + + if (len < 2) + return; + + /* + * The BCD2000 sends MIDI commands with 2 or 3 bytes length. In case of + * 2 bytes, the command is the same as before. + * + * Packet structure: mm nn oo (pp) + * mm: payload length + * nn: MIDI command or note + * oo: note or velocity + * pp: velocity + */ + + length = buf[0]; + + /* ignore packets without payload */ + if (length == 0) + return; + + /* + * The MIDI packets the BCD2000 sends can be arbitrarily truncated or + * concatenated. Therefore, this loop accumulates the bytes from the + * input buffer until a full valid MIDI packet is in the MIDI command + * buffer "midi_cmd_buf". + */ + + buf_offset = 1; + while (buf_offset <= length) { + cmd = buf[buf_offset]; + + if (bcd2k->midi_cmd_offset == 0 && cmd != 0x90 && cmd != 0xb0) { + /* + * this is a 2-byte midi packet -> reuse command byte + * from last midi packet + */ + bcd2k->midi_cmd_offset = 1; + } + + /* determine the number of bytes we want to copy this time */ + tocopy = min(3 - bcd2k->midi_cmd_offset, + length - (buf_offset - 1)); + + /* safety check */ + if (bcd2k->midi_cmd_offset + tocopy < BUFSIZE && + buf_offset + tocopy < len) { + memcpy(&bcd2k->midi_cmd_buf[bcd2k->midi_cmd_offset], + &buf[buf_offset], tocopy); + } else { + snd_printk(KERN_ERR PREFIX "access violation in %s\n", + __func__); + } + + bcd2k->midi_cmd_offset += tocopy; + buf_offset += tocopy; + + /* is our MIDI packet complete? */ + if (bcd2k->midi_cmd_offset == 3) { + if (debug) { + print_hex_dump(KERN_DEBUG, + "sending to userspace: ", + DUMP_PREFIX_NONE, 16, 1, + bcd2k->midi_cmd_buf, + bcd2k->midi_cmd_offset, false); + } + + /* send MIDI packet */ + snd_rawmidi_receive(bcd2k->midi_receive_substream, + bcd2k->midi_cmd_buf, 3); + + bcd2k->midi_cmd_offset = 0; + } + } +} + +static int bcd2000_midi_output_open(struct snd_rawmidi_substream *substream) +{ + return 0; +} + +static int bcd2000_midi_output_close(struct snd_rawmidi_substream *substream) +{ + struct bcd2000 *bcd2k = substream->rmidi->private_data; + if (bcd2k->midi_out_active) { + usb_free_urb(bcd2k->midi_out_urb); + bcd2k->midi_out_active = 0; + } + return 0; +} + +static void bcd2000_midi_send(struct bcd2000 *bcd2k, + struct snd_rawmidi_substream *substream) +{ + int len, ret; + + BUILD_BUG_ON(sizeof(device_cmd_prefix) >= BUFSIZE); + /* copy the "set LED" command bytes */ + memcpy(bcd2k->midi_out_buf, device_cmd_prefix, + sizeof(device_cmd_prefix)); + + /* + * get MIDI packet and leave space for command prefix + * and payload length + */ + len = snd_rawmidi_transmit(substream, + bcd2k->midi_out_buf + 3, BUFSIZE - 3); + + if (len <= 0) + return; + + /* set payload length */ + bcd2k->midi_out_buf[2] = len; + bcd2k->midi_out_urb->transfer_buffer_length = BUFSIZE; + + if (debug) { + print_hex_dump(KERN_DEBUG, "sending to device: ", + DUMP_PREFIX_NONE, 16, 1, + bcd2k->midi_out_buf, len+3, false); + } + + /* send packet to the BCD2000 */ + ret = usb_submit_urb(bcd2k->midi_out_urb, GFP_ATOMIC); + if (ret < 0) { + dev_err(&bcd2k->dev->dev, PREFIX + "%s (%p): usb_submit_urb() failed" + "ret=%d, len=%d\n", __func__, substream, ret, len); + } else { + bcd2k->midi_out_active = 1; + } +} + +static void bcd2000_midi_output_done(struct urb *urb) +{ + struct bcd2000 *bcd2k = urb->context; + + bcd2k->midi_out_active = 0; + if (urb->status != 0) { + dev_err(&urb->dev->dev, PREFIX "urb status: %d\n", urb->status); + return; + } + + if (!bcd2k->midi_out_substream) + return; + + bcd2000_midi_send(bcd2k, bcd2k->midi_out_substream); +} + +static void bcd2000_command_reply_dispatch(struct urb *urb) +{ + int ret; + struct device *dev = &urb->dev->dev; + struct bcd2000 *bcd2k = urb->context; + + if (urb->status || !bcd2k) { + dev_warn(dev, "received urb->status = %i\n", urb->status); + return; + } + + if (urb->actual_length > 0) + bcd2000_midi_handle_input(bcd2k, urb->transfer_buffer, + urb->actual_length); + + bcd2k->midi_in_urb->actual_length = 0; + ret = usb_submit_urb(bcd2k->midi_in_urb, GFP_ATOMIC); + if (ret < 0) + dev_err(dev, "unable to submit urb. OOM!?\n"); +} + +static void bcd2000_midi_output_trigger(struct snd_rawmidi_substream *substream, + int up) +{ + struct bcd2000 *bcd2k = substream->rmidi->private_data; + + if (up) { + bcd2k->midi_out_substream = substream; + if (!bcd2k->midi_out_active) + bcd2000_midi_send(bcd2k, substream); + } else { + bcd2k->midi_out_substream = NULL; + } +} + +static struct snd_rawmidi_ops bcd2000_midi_output = { + .open = bcd2000_midi_output_open, + .close = bcd2000_midi_output_close, + .trigger = bcd2000_midi_output_trigger, +}; + +static struct snd_rawmidi_ops bcd2000_midi_input = { + .open = bcd2000_midi_input_open, + .close = bcd2000_midi_input_close, + .trigger = bcd2000_midi_input_trigger, +}; + +static void bcd2000_init_device(struct bcd2000 *bcd2k) +{ + int ret; + + /* copy init sequence into buffer */ + memcpy(bcd2k->midi_out_buf, bcd2000_init_sequence, 52); + bcd2k->midi_out_urb->transfer_buffer_length = 52; + + /* submit sequence */ + ret = usb_submit_urb(bcd2k->midi_out_urb, GFP_ATOMIC); + if (ret < 0) { + dev_err(&bcd2k->dev->dev, PREFIX + "%s: usb_submit_urb() failed," + "ret=%d: ", __func__, ret); + } else { + bcd2k->midi_out_active = 1; + } + + /* send empty packet to enable button and controller events */ + bcd2k->midi_in_urb->actual_length = 0; + ret = usb_submit_urb(bcd2k->midi_in_urb, GFP_ATOMIC); +} + +static void bcd2000_init_midi(struct bcd2000 *bcd2k) +{ + int ret; + struct snd_rawmidi *rmidi; + + ret = snd_rawmidi_new(bcd2k->card, bcd2k->card->shortname, 0, + 1, /* output */ + 1, /* input */ + &rmidi); + + if (ret < 0) + return; + + strlcpy(rmidi->name, bcd2k->card->shortname, sizeof(rmidi->name)); + + rmidi->info_flags = SNDRV_RAWMIDI_INFO_DUPLEX; + rmidi->private_data = bcd2k; + + rmidi->info_flags |= SNDRV_RAWMIDI_INFO_OUTPUT; + snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_OUTPUT, + &bcd2000_midi_output); + + rmidi->info_flags |= SNDRV_RAWMIDI_INFO_INPUT; + snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_INPUT, + &bcd2000_midi_input); + + bcd2k->rmidi = rmidi; + + bcd2k->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL); + bcd2k->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL); + + usb_fill_int_urb(bcd2k->midi_in_urb, bcd2k->dev, + usb_rcvbulkpipe(bcd2k->dev, 0x81), + bcd2k->midi_in_buf, BUFSIZE, + bcd2000_command_reply_dispatch, bcd2k, 1); + + usb_fill_int_urb(bcd2k->midi_out_urb, bcd2k->dev, + usb_sndbulkpipe(bcd2k->dev, 0x1), + bcd2k->midi_out_buf, BUFSIZE, + bcd2000_midi_output_done, bcd2k, 1); + + bcd2000_init_device(bcd2k); +} + +static void bcd2000_close_midi(struct bcd2000 *bcd2k) +{ + usb_free_urb(bcd2k->midi_out_urb); + usb_free_urb(bcd2k->midi_in_urb); +} + + + + + +/* + * general part + */ + +static void bcd2000_free_usb_related_resources(struct bcd2000 *bcd2k, + struct usb_interface *interface) +{ + struct usb_interface *intf; + + mutex_lock(&bcd2k->mutex); + intf = bcd2k->intf; + bcd2k->intf = NULL; + mutex_unlock(&bcd2k->mutex); + if (intf) { + usb_set_intfdata(intf, NULL); + if (intf != interface) + usb_driver_release_interface(&bcd2000_driver, + intf); + } +} + +static void bcd2000_card_free(struct snd_card *card) +{ + /* empty for now */ +} + +static int bcd2000_probe(struct usb_interface *interface, + const struct usb_device_id *usb_id) +{ + struct snd_card *card; + struct bcd2000 *bcd2k; + unsigned int card_index; + char usb_path[32]; + int err; + + + mutex_lock(&devices_mutex); + + for (card_index = 0; card_index < SNDRV_CARDS; ++card_index) + if (enable[card_index] && !(devices_used & (1 << card_index))) + break; + if (card_index >= SNDRV_CARDS) { + mutex_unlock(&devices_mutex); + return -ENOENT; + } + err = snd_card_create(index[card_index], id[card_index], THIS_MODULE, + sizeof(*bcd2k), &card); + if (err < 0) { + mutex_unlock(&devices_mutex); + return err; + } + card->private_free = bcd2000_card_free; + bcd2k = card->private_data; + bcd2k->dev = interface_to_usbdev(interface); + bcd2k->card = card; + bcd2k->card_index = card_index; + INIT_LIST_HEAD(&bcd2k->midi_list); + spin_lock_init(&bcd2k->lock); + mutex_init(&bcd2k->mutex); + + bcd2k->intf = interface; + + snd_card_set_dev(card, &interface->dev); + + strcpy(card->driver, DEVICE_NAME); + strcpy(card->shortname, DEVICE_SHORTNAME); + usb_make_path(bcd2k->dev, usb_path, sizeof(usb_path)); + snprintf(bcd2k->card->longname, sizeof(bcd2k->card->longname), + DEVICE_NAME ", at %s, %s speed", + usb_path, + bcd2k->dev->speed == USB_SPEED_HIGH ? "high" : "full"); + + dev_info(&bcd2k->dev->dev, PREFIX "%s", bcd2k->card->longname); + + + + bcd2000_init_midi(bcd2k); + + + err = snd_card_register(card); + if (err < 0) + goto probe_error; + + usb_set_intfdata(interface, bcd2k); + devices_used |= 1 << card_index; + + mutex_unlock(&devices_mutex); + return 0; + +probe_error: + bcd2000_free_usb_related_resources(bcd2k, interface); + snd_card_free(card); + mutex_unlock(&devices_mutex); + return err; +} + +static void bcd2000_disconnect(struct usb_interface *interface) +{ + struct bcd2000 *bcd2k = usb_get_intfdata(interface); + struct list_head *midi; + + if (!bcd2k) + return; + + bcd2000_close_midi(bcd2k); + + mutex_lock(&devices_mutex); + + /* make sure that userspace cannot create new requests */ + snd_card_disconnect(bcd2k->card); + + /* make sure that there are no pending USB requests */ + list_for_each(midi, &bcd2k->midi_list) + snd_usbmidi_disconnect(midi); + + bcd2000_free_usb_related_resources(bcd2k, interface); + + devices_used &= ~(1 << bcd2k->card_index); + + snd_card_free_when_closed(bcd2k->card); + + mutex_unlock(&devices_mutex); +} + +static struct usb_driver bcd2000_driver = { + .name = DEVICE_SHORTNAME, + .probe = bcd2000_probe, + .disconnect = bcd2000_disconnect, + .id_table = id_table, +}; + +static int __init bcd2000_init(void) +{ + int retval = 0; + + retval = usb_register(&bcd2000_driver); + if (retval) + snd_printk(KERN_INFO "usb_register failed. Error: %d", retval); + return retval; +} + +static void __exit bcd2000_exit(void) +{ + usb_deregister(&bcd2000_driver); +} + +module_init(bcd2000_init); +module_exit(bcd2000_exit); + +MODULE_DEVICE_TABLE(usb, id_table); +MODULE_AUTHOR("Mario Kicherer, dev@kicherer.org"); +MODULE_DESCRIPTION("Behringer BCD2000 driver"); +MODULE_LICENSE("GPL");
On 02/05/2014 09:10 PM, Mario Kicherer wrote:
This patch adds initial support for the Behringer BCD2000 USB DJ controller. At the moment, only the MIDI part of the device is working, i.e. knobs, buttons and LEDs.
I also plan to add support for the audio part, but I assume that this will require more effort than the rather simple MIDI interface. Progress can be tracked at https://github.com/anyc/snd-usb-bcd2000.
Changes since v1: - fixed the various code style issues, thanks to Daniel Mack and checkpatch.pl.
Hmm, we're getting there :) But I still see:
total: 571 errors, 6 warnings, 580 lines checked
Mostly due to:
ERROR: DOS line endings
Did you send the patch using git send-email?
Also, please have a quick look at Documentation/CodingStyle.
--- /dev/null +++ b/sound/usb/bcd2000/bcd2000.c @@ -0,0 +1,553 @@ +/*
- Behringer BCD2000 driver
- Copyright (C) 2014 Mario Kicherer (dev@kicherer.org)
- 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.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- */
+#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/usb.h> +#include <linux/usb/audio.h> +#include <sound/core.h> +#include <sound/initval.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h>
As long as you don't need them, skip the pcm includes.
+#include <sound/rawmidi.h> +#include "../usbaudio.h" +#include "../midi.h"
+#define DEVICE_NAME "Behringer BCD2000" +#define DEVICE_SHORTNAME "bcd2000"
+#define PREFIX DEVICE_SHORTNAME ": " +#define BUFSIZE 64
+static struct usb_device_id id_table[] = {
- { USB_DEVICE(0x1397, 0x00bd) },
- { },
+};
+static int debug;
Hmm, what about the idea of moving the hex_dump function into an own helper? See below ...
+module_param(debug, int, S_IRUGO | S_IWUSR); +MODULE_PARM_DESC(debug, "enable debug output (default: 0)");
+static unsigned char device_cmd_prefix[] = {0x03, 0x00};
+static unsigned char bcd2000_init_sequence[] = {
- 0x07, 0x00, 0x00, 0x00, 0x78, 0x48, 0x1c, 0x81,
- 0xc4, 0x00, 0x00, 0x00, 0x5e, 0x53, 0x4a, 0xf7,
- 0x18, 0xfa, 0x11, 0xff, 0x6c, 0xf3, 0x90, 0xff,
- 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
- 0x18, 0xfa, 0x11, 0xff, 0x14, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0xf2, 0x34, 0x4a, 0xf7,
- 0x18, 0xfa, 0x11, 0xff};
'};' should be in a new line.
+struct bcd2000 {
- struct usb_device *dev;
- struct snd_card *card;
- struct usb_interface *intf;
- int card_index;
- struct snd_pcm *pcm;
Unused?
- struct list_head midi_list;
- spinlock_t lock;
Unused?
- struct mutex mutex;
That one's also kinda unused (see below)
...
+static DEFINE_MUTEX(devices_mutex); +static unsigned int devices_used; +static struct usb_driver bcd2000_driver;
Kill excessive newlines here.
+static void bcd2000_midi_handle_input(struct bcd2000 *bcd2k,
const unsigned char *buf, int len)
+{
- unsigned char length, cmd, buf_offset, tocopy;
- if (!bcd2k->midi_receive_substream)
return;
- if (debug) {
print_hex_dump(KERN_DEBUG, "received from device: ",
DUMP_PREFIX_NONE, 16, 1, buf, len, false);
- }
You could add something like this:
#ifdef CONFIG_SND_DEBUG static void bcd2000_midi_hex_dump(const char *prefix, const unsigned char *buf, int len) { print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_NONE, 16, 1, buf, len, false); } #else static inline void bcd2000_midi_hex_dump(const char *prefix, const unsigned char *buf, int len) {} #endif
And then just call the function from various places. That would safe you the module parameter, the condition check, and an indentation level.
- if (len < 2)
return;
- /*
* The BCD2000 sends MIDI commands with 2 or 3 bytes length. In case of
* 2 bytes, the command is the same as before.
*
* Packet structure: mm nn oo (pp)
* mm: payload length
* nn: MIDI command or note
* oo: note or velocity
* pp: velocity
*/
- length = buf[0];
- /* ignore packets without payload */
- if (length == 0)
return;
- /*
* The MIDI packets the BCD2000 sends can be arbitrarily truncated or
* concatenated. Therefore, this loop accumulates the bytes from the
* input buffer until a full valid MIDI packet is in the MIDI command
* buffer "midi_cmd_buf".
*/
- buf_offset = 1;
- while (buf_offset <= length) {
cmd = buf[buf_offset];
if (bcd2k->midi_cmd_offset == 0 && cmd != 0x90 && cmd != 0xb0) {
/*
* this is a 2-byte midi packet -> reuse command byte
* from last midi packet
*/
bcd2k->midi_cmd_offset = 1;
}
/* determine the number of bytes we want to copy this time */
tocopy = min(3 - bcd2k->midi_cmd_offset,
length - (buf_offset - 1));
/* safety check */
if (bcd2k->midi_cmd_offset + tocopy < BUFSIZE &&
buf_offset + tocopy < len) {
memcpy(&bcd2k->midi_cmd_buf[bcd2k->midi_cmd_offset],
&buf[buf_offset], tocopy);
} else {
snd_printk(KERN_ERR PREFIX "access violation in %s\n",
__func__);
You want to drop the entire packet here, right?
}
bcd2k->midi_cmd_offset += tocopy;
buf_offset += tocopy;
/* is our MIDI packet complete? */
if (bcd2k->midi_cmd_offset == 3) {
if (debug) {
print_hex_dump(KERN_DEBUG,
"sending to userspace: ",
DUMP_PREFIX_NONE, 16, 1,
bcd2k->midi_cmd_buf,
bcd2k->midi_cmd_offset, false);
}
{} braces are not needed when a block only holds one instruction. See Documentation/CodingStyle.
+static int bcd2000_midi_output_close(struct snd_rawmidi_substream *substream) +{
- struct bcd2000 *bcd2k = substream->rmidi->private_data;
Newline here, and elsewhere in your code, after variable declarations.
- if (bcd2k->midi_out_active) {
usb_free_urb(bcd2k->midi_out_urb);
bcd2k->midi_out_active = 0;
- }
- return 0;
+}
+static void bcd2000_midi_send(struct bcd2000 *bcd2k,
struct snd_rawmidi_substream *substream)
+{
- int len, ret;
- BUILD_BUG_ON(sizeof(device_cmd_prefix) >= BUFSIZE);
- /* copy the "set LED" command bytes */
- memcpy(bcd2k->midi_out_buf, device_cmd_prefix,
sizeof(device_cmd_prefix));
- /*
* get MIDI packet and leave space for command prefix
* and payload length
*/
- len = snd_rawmidi_transmit(substream,
bcd2k->midi_out_buf + 3, BUFSIZE - 3);
- if (len <= 0)
return;
- /* set payload length */
- bcd2k->midi_out_buf[2] = len;
- bcd2k->midi_out_urb->transfer_buffer_length = BUFSIZE;
- if (debug) {
print_hex_dump(KERN_DEBUG, "sending to device: ",
DUMP_PREFIX_NONE, 16, 1,
bcd2k->midi_out_buf, len+3, false);
- }
- /* send packet to the BCD2000 */
- ret = usb_submit_urb(bcd2k->midi_out_urb, GFP_ATOMIC);
This function is not called from atomic context, so GFP_KERNEL is ok.
- if (ret < 0) {
dev_err(&bcd2k->dev->dev, PREFIX
"%s (%p): usb_submit_urb() failed"
"ret=%d, len=%d\n", __func__, substream, ret, len);
- } else {
bcd2k->midi_out_active = 1;
- }
See above.
+}
+static void bcd2000_midi_output_done(struct urb *urb) +{
- struct bcd2000 *bcd2k = urb->context;
- bcd2k->midi_out_active = 0;
- if (urb->status != 0) {
dev_err(&urb->dev->dev, PREFIX "urb status: %d\n", urb->status);
return;
- }
- if (!bcd2k->midi_out_substream)
return;
- bcd2000_midi_send(bcd2k, bcd2k->midi_out_substream);
+}
...
+static void bcd2000_close_midi(struct bcd2000 *bcd2k) +{
- usb_free_urb(bcd2k->midi_out_urb);
- usb_free_urb(bcd2k->midi_in_urb);
+}
Too many newlines :)
+/*
- general part
- */
+static void bcd2000_free_usb_related_resources(struct bcd2000 *bcd2k,
struct usb_interface *interface)
+{
- struct usb_interface *intf;
- mutex_lock(&bcd2k->mutex);
- intf = bcd2k->intf;
- bcd2k->intf = NULL;
- mutex_unlock(&bcd2k->mutex);
What are you trying to lock here, and against what? Without another call site, this mutex is futile.
Thanks, Daniel
At Wed, 05 Feb 2014 23:32:23 +0100, Daniel Mack wrote:
On 02/05/2014 09:10 PM, Mario Kicherer wrote:
This patch adds initial support for the Behringer BCD2000 USB DJ controller. At the moment, only the MIDI part of the device is working, i.e. knobs, buttons and LEDs.
I also plan to add support for the audio part, but I assume that this will require more effort than the rather simple MIDI interface. Progress can be tracked at https://github.com/anyc/snd-usb-bcd2000.
Changes since v1: - fixed the various code style issues, thanks to Daniel Mack and checkpatch.pl.
Hmm, we're getting there :) But I still see:
total: 571 errors, 6 warnings, 580 lines checked
Mostly due to:
ERROR: DOS line endings
This must depend how you extract the mail content. I can save the mail as a Unix plain text, and checkpatch complains only two: ================================================================ WARNING: quoted string split across lines #400: FILE: sound/usb/bcd2000/bcd2000.c:255: + "%s (%p): usb_submit_urb() failed" + "ret=%d, len=%d\n", __func__, substream, ret, len);
WARNING: quoted string split across lines #482: FILE: sound/usb/bcd2000/bcd2000.c:337: + "%s: usb_submit_urb() failed," + "ret=%d: ", __func__, ret);
total: 0 errors, 2 warnings, 580 lines checked ================================================================
Actually, these two should be fixed. Better to keep the string in a single line than breaking 80 chars limit, in general. This would make it easier to work with grep.
Takashi
Thanks, applied most fixes. Further points:
On 05.02.2014 23:32, Daniel Mack wrote:
Mostly due to:
ERROR: DOS line endings
Did you send the patch using git send-email?
Yes, I only get the two warnings as Takashi. I also moved the strings in one line as he suggested.
+struct bcd2000 {
- struct usb_device *dev;
- struct snd_card *card;
- struct usb_interface *intf;
- int card_index;
- struct snd_pcm *pcm;
Unused?
- struct list_head midi_list;
- spinlock_t lock;
Unused?
- struct mutex mutex;
That one's also kinda unused (see below)
You're right. The original module had multiple interfaces. Removed.
+static DEFINE_MUTEX(devices_mutex); +static unsigned int devices_used; +static struct usb_driver bcd2000_driver;
Kill excessive newlines here.
Okay, removed. I find little gaps useful to separate logical parts.
You could add something like this:
#ifdef CONFIG_SND_DEBUG static void bcd2000_midi_hex_dump(const char *prefix, const unsigned char *buf, int len) { print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_NONE, 16, 1, buf, len, false); } #else static inline void bcd2000_midi_hex_dump(const char *prefix, const unsigned char *buf, int len) {} #endif
And then just call the function from various places. That would safe you the module parameter, the condition check, and an indentation level.
Hm, I removed the CONFIG_ part as the module parameter would allow me and possible users to debug things spontaneously. But I can use our approach if you think it would be better nonetheless?
/* safety check */
if (bcd2k->midi_cmd_offset + tocopy < BUFSIZE &&
buf_offset + tocopy < len) {
memcpy(&bcd2k->midi_cmd_buf[bcd2k->midi_cmd_offset],
&buf[buf_offset], tocopy);
} else {
snd_printk(KERN_ERR PREFIX "access violation in %s\n",
__func__);
You want to drop the entire packet here, right?
Hm, I'm not sure. A "return" after the snd_printk would do, right?
Thank you very much! Mario
On 02/06/2014 05:07 PM, Mario Kicherer wrote:
On 05.02.2014 23:32, Daniel Mack wrote:
ERROR: DOS line endings
Did you send the patch using git send-email?
Yes, I only get the two warnings as Takashi. I also moved the strings in one line as he suggested.
As Takashi pointed out, that might be my bad. I'm puzzled though, as I didn't change my procedure.
You could add something like this:
#ifdef CONFIG_SND_DEBUG static void bcd2000_midi_hex_dump(const char *prefix, const unsigned char *buf, int len) { print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_NONE, 16, 1, buf, len, false); } #else static inline void bcd2000_midi_hex_dump(const char *prefix, const unsigned char *buf, int len) {} #endif
And then just call the function from various places. That would safe you the module parameter, the condition check, and an indentation level.
Hm, I removed the CONFIG_ part as the module parameter would allow me and possible users to debug things spontaneously. But I can use our approach if you think it would be better nonetheless?
I think so. Less debug conditionals in the code usually result in better readability. But it's a nit really.
/* safety check */
if (bcd2k->midi_cmd_offset + tocopy < BUFSIZE &&
buf_offset + tocopy < len) {
memcpy(&bcd2k->midi_cmd_buf[bcd2k->midi_cmd_offset],
&buf[buf_offset], tocopy);
} else {
snd_printk(KERN_ERR PREFIX "access violation in %s\n",
__func__);
You want to drop the entire packet here, right?
Hm, I'm not sure. A "return" after the snd_printk would do, right?
Yes, that should do.
Daniel
Mario Kicherer wrote:
This patch adds initial support for the Behringer BCD2000 USB DJ controller. At the moment, only the MIDI part of the device is working, i.e. knobs, buttons and LEDs.
+config SND_USB_BCD2000
- tristate "Behringer BCD2000 MIDI driver"
- select SND_HWDEP
Why HWDEP?
To compile this driver as a module, choose M here: the module
will be called snd-usb-bcd2000.
There is no need to have "usb" in the module name if that is the only bus this device works with.
-obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/
If there is only a single source file, just put it into misc/. If you plan to have multiple source files, then this file should have "_midi" in its name.
+#include "../usbaudio.h" +#include "../midi.h"
Are these headers needed?
+static void bcd2000_midi_input_trigger(struct snd_rawmidi_substream *substream,
int up)
+{
- struct bcd2000 *bcd2k = substream->rmidi->private_data;
- if (!bcd2k)
return;
This cannot happen. And if it happens anyway, silently returning is not necessarily a better option than crashing.
+static void bcd2000_midi_handle_input(struct bcd2000 *bcd2k,
const unsigned char *buf, int len)
- /*
* The MIDI packets the BCD2000 sends can be arbitrarily truncated or
* concatenated. Therefore, this loop accumulates the bytes from the
* input buffer until a full valid MIDI packet is in the MIDI command
* buffer "midi_cmd_buf".
*/
ALSA does not require full MIDI commands, and handles running status.
+static int bcd2000_midi_output_close(struct snd_rawmidi_substream *substream) +{
- struct bcd2000 *bcd2k = substream->rmidi->private_data;
- if (bcd2k->midi_out_active) {
usb_free_urb(bcd2k->midi_out_urb);
Why is this the right place to free the URB?
+static void bcd2000_midi_output_done(struct urb *urb) +{ ...
- if (!bcd2k->midi_out_substream)
return;
- bcd2000_midi_send(bcd2k, bcd2k->midi_out_substream);
The midi_out_substream field gets modified asynchronously by the trigger callback; this might happen between these two statements.
Assuming that setting a pointer is atomic, use ACCESS_ONCE.
+static void bcd2000_init_midi(struct bcd2000 *bcd2k) +{
- bcd2k->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL);
- bcd2k->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL);
Error checking.
+static int bcd2000_probe(struct usb_interface *interface,
const struct usb_device_id *usb_id)
+{
- snprintf(bcd2k->card->longname, sizeof(bcd2k->card->longname),
DEVICE_NAME ", at %s, %s speed",
usb_path,
bcd2k->dev->speed == USB_SPEED_HIGH ? "high" : "full");
The BCD2000 does not support high speed.
+static void bcd2000_disconnect(struct usb_interface *interface) +{
- list_for_each(midi, &bcd2k->midi_list)
snd_usbmidi_disconnect(midi);
midi_list is not actually used.
Regards, Clemens
On 06.02.2014 10:21, Clemens Ladisch wrote:
+config SND_USB_BCD2000
- tristate "Behringer BCD2000 MIDI driver"
- select SND_HWDEP
Why HWDEP?
I admit, I copied this from the other modules (caiaq I believe). After grepping the documentation, I'm not sure if I need this. Should I remove it?
There is no need to have "usb" in the module name if that is the only bus this device works with.
Okay, after looking at the other modules I thought that it is common to use the "snd-usb" prefix. Should I remove it? I find it useful, e.g., to recognize it in a "lsmod".
-obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/
If there is only a single source file, just put it into misc/. If you plan to have multiple source files, then this file should have "_midi" in its name.
Okay, I thought that the code is short enough to put everything in one file but I think with the audio part it would be best to split the file. I can move it to misc/ or do you think an own subdirectory is okay to avoid moving it in later versions?
+#include "../usbaudio.h" +#include "../midi.h"
Are these headers needed?
After cleaning up the code with Daniel, no, it's not needed anymore. Removed.
+static void bcd2000_midi_handle_input(struct bcd2000 *bcd2k,
const unsigned char *buf, int len)
- /*
* The MIDI packets the BCD2000 sends can be arbitrarily truncated or
* concatenated. Therefore, this loop accumulates the bytes from the
* input buffer until a full valid MIDI packet is in the MIDI command
* buffer "midi_cmd_buf".
*/
ALSA does not require full MIDI commands, and handles running status.
Oh, okay. So, should I just pass everything through to snd_rawmidi_receive() ?
+static int bcd2000_midi_output_close(struct snd_rawmidi_substream *substream) +{
- struct bcd2000 *bcd2k = substream->rmidi->private_data;
- if (bcd2k->midi_out_active) {
usb_free_urb(bcd2k->midi_out_urb);
Why is this the right place to free the URB?
Good point, doesn't really make sense as it only allocated once. I'll remove it.
+static void bcd2000_midi_output_done(struct urb *urb) +{ ...
- if (!bcd2k->midi_out_substream)
return;
- bcd2000_midi_send(bcd2k, bcd2k->midi_out_substream);
The midi_out_substream field gets modified asynchronously by the trigger callback; this might happen between these two statements.
Assuming that setting a pointer is atomic, use ACCESS_ONCE.
You mean like this:
struct snd_rawmidi_substream * midi_out_substream; midi_out_substream = ACCESS_ONCE(bcd2k->midi_out_substream); if (!midi_out_substream) return; bcd2000_midi_send(bcd2k, midi_out_substream);
+static void bcd2000_init_midi(struct bcd2000 *bcd2k) +{
- bcd2k->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL);
- bcd2k->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL);
Error checking.
Okay, I guess I should close everything I opened in this module before or would a "return" suffice?
Thank you for your comments! Mario
Mario Kicherer wrote:
On 06.02.2014 10:21, Clemens Ladisch wrote:
+config SND_USB_BCD2000
- tristate "Behringer BCD2000 MIDI driver"
- select SND_HWDEP
Why HWDEP?
I admit, I copied this from the other modules (caiaq I believe). After grepping the documentation, I'm not sure if I need this. Should I remove it?
Yes; you aren't using any snd_hwdep* functions.
There is no need to have "usb" in the module name if that is the only bus this device works with.
Okay, after looking at the other modules I thought that it is common to use the "snd-usb" prefix.
The USB audio class driver uses "snd-usb-audio" because "snd-audio" would be completely useless as a name. Other drivers then blindly copied this.
Should I remove it? I find it useful, e.g., to recognize it in a "lsmod".
"snd-bcd2000" would not be recognizable?
-obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ +obj-$(CONFIG_SND) += misc/ usx2y/ caiaq/ 6fire/ hiface/ bcd2000/
If there is only a single source file, just put it into misc/. If you plan to have multiple source files, then this file should have "_midi" in its name.
Okay, I thought that the code is short enough to put everything in one file but I think with the audio part it would be best to split the file. I can move it to misc/ or do you think an own subdirectory is okay to avoid moving it in later versions?
It doesn't make much sense to put it into a directory that you know you will move it out of later.
ALSA does not require full MIDI commands, and handles running status.
Oh, okay. So, should I just pass everything through to snd_rawmidi_receive() ?
Yes.
+static void bcd2000_midi_output_done(struct urb *urb) +{ ...
- if (!bcd2k->midi_out_substream)
return;
- bcd2000_midi_send(bcd2k, bcd2k->midi_out_substream);
The midi_out_substream field gets modified asynchronously by the trigger callback; this might happen between these two statements.
Assuming that setting a pointer is atomic, use ACCESS_ONCE.
You mean like this:
struct snd_rawmidi_substream * midi_out_substream; midi_out_substream = ACCESS_ONCE(bcd2k->midi_out_substream); if (!midi_out_substream) return; bcd2000_midi_send(bcd2k, midi_out_substream);
Yes.
The code that modifies this field also needs ACCESS_ONCE.
+static void bcd2000_init_midi(struct bcd2000 *bcd2k) +{
- bcd2k->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL);
- bcd2k->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL);
Error checking.
Okay, I guess I should close everything I opened in this module before or would a "return" suffice?
Typically, in case of an error, a function is responsible to revert everything allocated in this function, i.e.:
static int bcd2000_init_midi(struct bcd2000 *bcd2k) { bcd2k->midi_in_urb = usb_alloc_urb(0, GFP_KERNEL); if (!bcd2k->midi_in_urb) return -ENOMEM; bcd2k->midi_out_urb = usb_alloc_urb(0, GFP_KERNEL); if (!bcd2k->midi_out_urb) { usb_free_urb(bcd2k->midi_in_urb); return -ENOMEM; } ...
Regards, Clemens
participants (4)
-
Clemens Ladisch
-
Daniel Mack
-
Mario Kicherer
-
Takashi Iwai