On Wed, Sep 28, 2011 at 2:21 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> On Tue, Sep 20, 2011 at 06:59:26PM +0200, Daniel Mack wrote:
>> This patch adds f_midi.c to implement a USB gadget function that works
>> with the composite framework, so it can be combined with other USB
>> functions.
>>
>> The code for the ALSA/MIDI logic was taken from the midi device gadget,
>> other parts have been rewritten to benefit from the dynamic descriptor
>> allocation features.
>>
>> This was successfully tested on an OMAP3 board.
>
> pass this through checkpatch.pl --strict.
>
> few extra comments below.
>
>> diff --git a/drivers/usb/gadget/f_midi.c b/drivers/usb/gadget/f_midi.c
>> new file mode 100644
>> index 0000000..a4ff2e9
>> --- /dev/null
>> +++ b/drivers/usb/gadget/f_midi.c
>> @@ -0,0 +1,918 @@
>> +/*
>> + * f_midi.c -- USB MIDI class function driver
>> + *
>> + * Copyright (C) 2006 Thumtronics Pty Ltd.
>> + * Developed for Thumtronics by Grey Innovation
>> + * Ben Williamson <ben.williamson@xxxxxxxxxxxxxxxxxx>
>> + *
>> + * Rewritten for the composite framework
>> + * Copyright (C) 2011 Daniel Mack <zonque@xxxxxxxxx>
>> + *
>> + * Based on drivers/usb/gadget/f_audio.c,
>> + * Copyright (C) 2008 Bryan Wu <cooloney@xxxxxxxxxx>
>> + * Copyright (C) 2008 Analog Devices, Inc
>> + *
>> + * and drivers/usb/gadget/midi.c,
>> + * Copyright (C) 2006 Thumtronics Pty Ltd.
>> + * Ben Williamson <ben.williamson@xxxxxxxxxxxxxxxxxx>
>> + *
>> + * Licensed under the GPL-2 or later.
>
> sure about this "or later" ???
I copied this code from someone else (Ben in this case), and this was
his license. Should I change that?
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/utsname.h>
>> +#include <linux/device.h>
>> +
>> +#include <sound/core.h>
>> +#include <sound/initval.h>
>> +#include <sound/rawmidi.h>
>> +
>> +#include <linux/usb/ch9.h>
>> +#include <linux/usb/gadget.h>
>> +#include <linux/usb/audio.h>
>> +#include <linux/usb/midi.h>
>> +
>> +MODULE_AUTHOR("Ben Williamson, Daniel Mack");
>
> instead, add two entries. One for each author.
Ok.
>
>> +MODULE_LICENSE("GPL v2");
>> +
>> +static const char f_midi_shortname[] = "f_midi";
>> +static const char f_midi_longname[] = "MIDI Gadget";
>> +
>> +/* This is a gadget, and the IN/OUT naming is from the host's perspective.
>> + USB -> OUT endpoint -> rawmidi
>> + USB <- IN endpoint <- rawmidi */
>
> comment style is wrong.
This was also taken from the original sources. But I can fix that.
>
>> +struct gmidi_in_port {
>> + struct f_midi *midi;
>> + int active;
>> + uint8_t cable; /* cable number << 4 */
>> + uint8_t state;
>> +#define STATE_UNKNOWN 0
>> +#define STATE_1PARAM 1
>> +#define STATE_2PARAM_1 2
>> +#define STATE_2PARAM_2 3
>> +#define STATE_SYSEX_0 4
>> +#define STATE_SYSEX_1 5
>> +#define STATE_SYSEX_2 6
>> + uint8_t data[2];
>> +};
>> +
>> +struct f_midi {
>> + struct usb_function func;
>> + spinlock_t lock;
>> + struct usb_gadget *gadget;
>> + struct usb_ep *in_ep, *out_ep;
>> + struct snd_card *card;
>> + struct snd_rawmidi *rmidi;
>> + struct snd_rawmidi_substream *in_substream;
>> + struct snd_rawmidi_substream *out_substream;
>> +
>> + /* For the moment we only support one port in
>> + each direction, but in_port is kept as a
>> + separate struct so we can have more later. */
>
> comment style.
>
>> +/* B.4.2 Class-Specific MS Interface Descriptor */
>> +static struct usb_ms_header_descriptor ms_header_desc __initdata = {
>> + .bLength = USB_DT_MS_HEADER_SIZE,
>> + .bDescriptorType = USB_DT_CS_INTERFACE,
>> + .bDescriptorSubtype = USB_MS_HEADER,
>> + .bcdMSC = cpu_to_le16(0x0100),
>> + .wTotalLength = cpu_to_le16(USB_DT_MS_HEADER_SIZE
>> + + 2*USB_DT_MIDI_IN_SIZE
>> + + 2*USB_DT_MIDI_OUT_SIZE(1)),
>
> you can calculate this on the fly, please do so in midi.c
Not sure whether I follow you here, and this part is also just taken
from gmidi.c originally. However, I changed this whole logic anyway in
my last patch, as these fields are now dynamically allocated. Can we
agree to leave things as they are in this patch then?
Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
