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