Re: [PATCH v3] MIDI driver for Behringer BCD2000 USB device

[Daniel Mack <daniel@zonque.org>]

Hi Mario,

On 02/10/2014 11:34 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.

This looks mostly nice to me now. I just have some minor comments inline.

>  sound/usb/Kconfig           |  13 ++
>  sound/usb/Makefile          |   2 +-
>  sound/usb/bcd2000/Makefile  |   3 +
>  sound/usb/bcd2000/bcd2000.c | 484 ++++++++++++++++++++++++++++++++++++++++++++

As soon as you add more functionality here such as audio, please rename
the file so it describes what it's doing, and split functionality into
something like card.c, midi.c, pcm.c etc. I think for now, the naming is
acceptable.

> +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};

'};' to a new line, please.

> +static void bcd2000_midi_handle_input(struct bcd2000 *bcd2k,
> +					const unsigned char *buf, int len)

You're passing the 'len' parameter from urb->actual_length, which is
unsigned int. For consistency, make the type match here.

> +{
> +	unsigned char length, tocopy;
> +
> +	if (!bcd2k->midi_receive_substream)
> +		return;
> +
> +	bcd2000_dump_buffer(PREFIX "received from device: ", buf, len);
> +
> +	if (len < 2)
> +		return;
> +
> +	/*
> +	 * 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;
> +
> +	tocopy = min(length, (unsigned char) (len-1));

I'd rather like it the other way around: make 'length' and 'tocopy'
unsigned int, so you can avoid the cast and prevent the statement from
overflowing.

> +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)
> +		snd_printk("%s: snd_rawmidi_transmit error %d\n",
> +				__func__, len);

Takashi just went through all the drivers and replaced the snd_printk()
calls with pr_* and dev_* functions. This driver should also follow that
new fashion ...

> +	if (ret < 0)
> +		dev_err(&bcd2k->dev->dev, PREFIX
> +			"%s (%p): usb_submit_urb() failed, ret=%d, len=%d\n",
> +			__func__, substream, ret, len);

... like you do here.



Thanks!
Daniel