[alsa-devel] [RFC] Cleanup/refactor the usbaudio driver
I've spent some time cleaning up the usbaudio driver. I felt that the existing implementation was quite difficult to follow because it contained code for all sorts of things mixed up in one big file.
So I split the driver into pieces and tried to be as less intrusive as I can, meaning that I didn't touch much real code but rather copied the functions as they were and factored them out. Of course, non-static functions were properly prefixed to avoid name space pollution.
I also moved almost all quirks to a seperate file and introduced two more generic hooks to handle them, so the excecption add less to the actual code of the driver.
Unfortunately it wasn't easily possible to put that big change into smaller pieces, and so 3/3 became a huge chunk.
After all, I feel that it is easier to read now, but I don't know whether you agree. So take this as RFC for now :)
Daniel
As part of the USB audio code cleanup, move the non-standard ua101 driver out of the way.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Clemens Ladisch clemens@ladisch.de Cc: Takashi Iwai tiwai@suse.de --- sound/usb/Makefile | 5 ++--- sound/usb/ua101/Makefile | 2 ++ sound/usb/{ => ua101}/ua101.c | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) create mode 100644 sound/usb/ua101/Makefile rename sound/usb/{ => ua101}/ua101.c (99%)
diff --git a/sound/usb/Makefile b/sound/usb/Makefile index 5bf64ae..8bc005b 100644 --- a/sound/usb/Makefile +++ b/sound/usb/Makefile @@ -4,12 +4,11 @@
snd-usb-audio-objs := usbaudio.o usbmixer.o snd-usb-lib-objs := usbmidi.o -snd-ua101-objs := ua101.o
# Toplevel Module Dependency obj-$(CONFIG_SND_USB_AUDIO) += snd-usb-audio.o snd-usb-lib.o -obj-$(CONFIG_SND_USB_UA101) += snd-ua101.o snd-usb-lib.o +obj-$(CONFIG_SND_USB_UA101) += snd-usb-lib.o obj-$(CONFIG_SND_USB_USX2Y) += snd-usb-lib.o obj-$(CONFIG_SND_USB_US122L) += snd-usb-lib.o
-obj-$(CONFIG_SND) += usx2y/ caiaq/ +obj-$(CONFIG_SND) += ua101/ usx2y/ caiaq/ diff --git a/sound/usb/ua101/Makefile b/sound/usb/ua101/Makefile new file mode 100644 index 0000000..ccefd81 --- /dev/null +++ b/sound/usb/ua101/Makefile @@ -0,0 +1,2 @@ +snd-ua101-objs := ua101.o +obj-$(CONFIG_SND_USB_UA101) += snd-ua101.o diff --git a/sound/usb/ua101.c b/sound/usb/ua101/ua101.c similarity index 99% rename from sound/usb/ua101.c rename to sound/usb/ua101/ua101.c index 4f4ccdf..c7dcfe2 100644 --- a/sound/usb/ua101.c +++ b/sound/usb/ua101/ua101.c @@ -23,7 +23,7 @@ #include <sound/initval.h> #include <sound/pcm.h> #include <sound/pcm_params.h> -#include "usbaudio.h" +#include "../usbaudio.h"
MODULE_DESCRIPTION("Edirol UA-101 driver"); MODULE_AUTHOR("Clemens Ladisch clemens@ladisch.de");
Rename snd-usb-lib to snd-usbmidi-lib as MIDI functions are the only thing it actually contains. Introduce a new header file to only declare these functions.
Introduced usbmixer.h for all functions exported by usbmixer.c.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Clemens Ladisch clemens@ladisch.de Cc: Takashi Iwai tiwai@suse.de --- sound/usb/Makefile | 11 +++++---- sound/usb/ua101/ua101.c | 1 + sound/usb/usbaudio.c | 3 +- sound/usb/usbaudio.h | 51 -------------------------------------------- sound/usb/usbmidi.c | 1 + sound/usb/usbmidi.h | 48 +++++++++++++++++++++++++++++++++++++++++ sound/usb/usbmixer.c | 1 + sound/usb/usbmixer.h | 11 +++++++++ sound/usb/usx2y/us122l.c | 1 + sound/usb/usx2y/usbusx2y.h | 1 + 10 files changed, 72 insertions(+), 57 deletions(-) create mode 100644 sound/usb/usbmidi.h create mode 100644 sound/usb/usbmixer.h
diff --git a/sound/usb/Makefile b/sound/usb/Makefile index 8bc005b..dc31306 100644 --- a/sound/usb/Makefile +++ b/sound/usb/Makefile @@ -3,12 +3,13 @@ #
snd-usb-audio-objs := usbaudio.o usbmixer.o -snd-usb-lib-objs := usbmidi.o +snd-usbmidi-lib-objs := usbmidi.o
# Toplevel Module Dependency -obj-$(CONFIG_SND_USB_AUDIO) += snd-usb-audio.o snd-usb-lib.o -obj-$(CONFIG_SND_USB_UA101) += snd-usb-lib.o -obj-$(CONFIG_SND_USB_USX2Y) += snd-usb-lib.o -obj-$(CONFIG_SND_USB_US122L) += snd-usb-lib.o +obj-$(CONFIG_SND_USB_AUDIO) += snd-usb-audio.o snd-usbmidi-lib.o + +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) += ua101/ usx2y/ caiaq/ diff --git a/sound/usb/ua101/ua101.c b/sound/usb/ua101/ua101.c index c7dcfe2..34a55c3 100644 --- a/sound/usb/ua101/ua101.c +++ b/sound/usb/ua101/ua101.c @@ -24,6 +24,7 @@ #include <sound/pcm.h> #include <sound/pcm_params.h> #include "../usbaudio.h" +#include "../usbmidi.h"
MODULE_DESCRIPTION("Edirol UA-101 driver"); MODULE_AUTHOR("Clemens Ladisch clemens@ladisch.de"); diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c index c539f7f..02870d5 100644 --- a/sound/usb/usbaudio.c +++ b/sound/usb/usbaudio.c @@ -56,7 +56,8 @@ #include <sound/initval.h>
#include "usbaudio.h" - +#include "usbmidi.h" +#include "usbmixer.h"
MODULE_AUTHOR("Takashi Iwai tiwai@suse.de"); MODULE_DESCRIPTION("USB Audio"); diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h index 6b016d4..4fde472 100644 --- a/sound/usb/usbaudio.h +++ b/sound/usb/usbaudio.h @@ -21,9 +21,6 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
-/* maximum number of endpoints per interface */ -#define MIDI_MAX_ENDPOINTS 2 - /* handling of USB vendor/product ID pairs as 32-bit numbers */ #define USB_ID(vendor, product) (((vendor) << 16) | (product)) #define USB_ID_VENDOR(id) ((id) >> 16) @@ -90,39 +87,6 @@ struct snd_usb_audio_quirk { const void *data; };
-/* data for QUIRK_MIDI_FIXED_ENDPOINT */ -struct snd_usb_midi_endpoint_info { - int8_t out_ep; /* ep number, 0 autodetect */ - uint8_t out_interval; /* interval for interrupt endpoints */ - int8_t in_ep; - uint8_t in_interval; - uint16_t out_cables; /* bitmask */ - uint16_t in_cables; /* bitmask */ -}; - -/* for QUIRK_MIDI_YAMAHA, data is NULL */ - -/* for QUIRK_MIDI_MIDIMAN, data points to a snd_usb_midi_endpoint_info - * structure (out_cables and in_cables only) */ - -/* for QUIRK_COMPOSITE, data points to an array of snd_usb_audio_quirk - * structures, terminated with .ifnum = -1 */ - -/* for QUIRK_AUDIO_FIXED_ENDPOINT, data points to an audioformat structure */ - -/* for QUIRK_AUDIO/MIDI_STANDARD_INTERFACE, data is NULL */ - -/* for QUIRK_AUDIO_EDIROL_UA700_UA25/UA1000, data is NULL */ - -/* for QUIRK_IGNORE_INTERFACE, data is NULL */ - -/* for QUIRK_MIDI_NOVATION and _RAW, data is NULL */ - -/* for QUIRK_MIDI_EMAGIC, data points to a snd_usb_midi_endpoint_info - * structure (out_cables and in_cables only) */ - -/* for QUIRK_MIDI_CME, data is NULL */ - /* */
@@ -149,21 +113,6 @@ int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe, __u8 request, __u8 requesttype, __u16 value, __u16 index, void *data, __u16 size, int timeout);
-int snd_usb_create_mixer(struct snd_usb_audio *chip, int ctrlif, - int ignore_error); -void snd_usb_mixer_disconnect(struct list_head *p); - -int snd_usbmidi_create(struct snd_card *card, - struct usb_interface *iface, - struct list_head *midi_list, - const struct snd_usb_audio_quirk *quirk); -void snd_usbmidi_input_stop(struct list_head* p); -void snd_usbmidi_input_start(struct list_head* p); -void snd_usbmidi_disconnect(struct list_head *p); - -void snd_emuusb_set_samplerate(struct snd_usb_audio *chip, - unsigned char samplerate_id); - /* * retrieve usb_interface descriptor from the host interface * (conditional for compatibility with the older API) diff --git a/sound/usb/usbmidi.c b/sound/usb/usbmidi.c index 2c59afd..5915a04 100644 --- a/sound/usb/usbmidi.c +++ b/sound/usb/usbmidi.c @@ -53,6 +53,7 @@ #include <sound/rawmidi.h> #include <sound/asequencer.h> #include "usbaudio.h" +#include "usbmidi.h"
/* diff --git a/sound/usb/usbmidi.h b/sound/usb/usbmidi.h new file mode 100644 index 0000000..2089ec9 --- /dev/null +++ b/sound/usb/usbmidi.h @@ -0,0 +1,48 @@ +#ifndef __USBMIDI_H +#define __USBMIDI_H + +/* maximum number of endpoints per interface */ +#define MIDI_MAX_ENDPOINTS 2 + +/* data for QUIRK_MIDI_FIXED_ENDPOINT */ +struct snd_usb_midi_endpoint_info { + int8_t out_ep; /* ep number, 0 autodetect */ + uint8_t out_interval; /* interval for interrupt endpoints */ + int8_t in_ep; + uint8_t in_interval; + uint16_t out_cables; /* bitmask */ + uint16_t in_cables; /* bitmask */ +}; + +/* for QUIRK_MIDI_YAMAHA, data is NULL */ + +/* for QUIRK_MIDI_MIDIMAN, data points to a snd_usb_midi_endpoint_info + * structure (out_cables and in_cables only) */ + +/* for QUIRK_COMPOSITE, data points to an array of snd_usb_audio_quirk + * structures, terminated with .ifnum = -1 */ + +/* for QUIRK_AUDIO_FIXED_ENDPOINT, data points to an audioformat structure */ + +/* for QUIRK_AUDIO/MIDI_STANDARD_INTERFACE, data is NULL */ + +/* for QUIRK_AUDIO_EDIROL_UA700_UA25/UA1000, data is NULL */ + +/* for QUIRK_IGNORE_INTERFACE, data is NULL */ + +/* for QUIRK_MIDI_NOVATION and _RAW, data is NULL */ + +/* for QUIRK_MIDI_EMAGIC, data points to a snd_usb_midi_endpoint_info + * structure (out_cables and in_cables only) */ + +/* for QUIRK_MIDI_CME, data is NULL */ + +int snd_usbmidi_create(struct snd_card *card, + struct usb_interface *iface, + struct list_head *midi_list, + const struct snd_usb_audio_quirk *quirk); +void snd_usbmidi_input_stop(struct list_head* p); +void snd_usbmidi_input_start(struct list_head* p); +void snd_usbmidi_disconnect(struct list_head *p); + +#endif /* __USBMIDI_H */ diff --git a/sound/usb/usbmixer.c b/sound/usb/usbmixer.c index 8e8f871..43d53a3 100644 --- a/sound/usb/usbmixer.c +++ b/sound/usb/usbmixer.c @@ -41,6 +41,7 @@ #include <sound/tlv.h>
#include "usbaudio.h" +#include "usbmixer.h"
/* */ diff --git a/sound/usb/usbmixer.h b/sound/usb/usbmixer.h new file mode 100644 index 0000000..e199e4b --- /dev/null +++ b/sound/usb/usbmixer.h @@ -0,0 +1,11 @@ +#ifndef __USBMIXER_H +#define __USBMIXER_H + +int snd_usb_create_mixer(struct snd_usb_audio *chip, int ctrlif, + int ignore_error); +void snd_usb_mixer_disconnect(struct list_head *p); + +void snd_emuusb_set_samplerate(struct snd_usb_audio *chip, + unsigned char samplerate_id); + +#endif /* __USBMIXER_H */ diff --git a/sound/usb/usx2y/us122l.c b/sound/usb/usx2y/us122l.c index 44deb21..4f6518c 100644 --- a/sound/usb/usx2y/us122l.c +++ b/sound/usb/usx2y/us122l.c @@ -25,6 +25,7 @@ #define MODNAME "US122L" #include "usb_stream.c" #include "../usbaudio.h" +#include "../usbmidi.h" #include "us122l.h"
MODULE_AUTHOR("Karsten Wiese fzu@wemgehoertderstaat.de"); diff --git a/sound/usb/usx2y/usbusx2y.h b/sound/usb/usx2y/usbusx2y.h index 1d174ce..9ab97b4 100644 --- a/sound/usb/usx2y/usbusx2y.h +++ b/sound/usb/usx2y/usbusx2y.h @@ -1,6 +1,7 @@ #ifndef USBUSX2Y_H #define USBUSX2Y_H #include "../usbaudio.h" +#include "../usbmidi.h" #include "usbus428ctldefs.h"
#define NRURBS 2
Daniel Mack wrote:
As part of the USB audio code cleanup, move the non-standard ua101 driver out of the way.
A directory with a single source file in it isn't quite an improvement. Please name it "misc" so that other specialized drivers could be put in there.
Regards, Clemens
On Mon, Mar 01, 2010 at 12:27:31PM +0100, Clemens Ladisch wrote:
Daniel Mack wrote:
As part of the USB audio code cleanup, move the non-standard ua101 driver out of the way.
A directory with a single source file in it isn't quite an improvement. Please name it "misc" so that other specialized drivers could be put in there.
Ok, will do. However, I wonder which way is best to do that. Given that you posted patches on top of mine, can I take this as a general agreement to my changeset? If so, I will just provide a patch on top to do the ua101 -> misc rename, because otherwise, I would have to rebase all three patches for that.
Daniel
At Mon, 1 Mar 2010 15:01:29 +0100, Daniel Mack wrote:
On Mon, Mar 01, 2010 at 12:27:31PM +0100, Clemens Ladisch wrote:
Daniel Mack wrote:
As part of the USB audio code cleanup, move the non-standard ua101 driver out of the way.
A directory with a single source file in it isn't quite an improvement. Please name it "misc" so that other specialized drivers could be put in there.
Ok, will do. However, I wonder which way is best to do that. Given that you posted patches on top of mine, can I take this as a general agreement to my changeset? If so, I will just provide a patch on top to do the ua101 -> misc rename, because otherwise, I would have to rebase all three patches for that.
Well, honestly, I prefer seeing the fixes by Clemens based on the current code, and merge Daniel's refactoring patches after 2.6.34 merge window.
Refactoring is a good thing but it can easily introduce any careless bugs (like copy&paste error), and this split action makes really hard to track the changes. Thus I'd like to keep this well ripened before reaching to the Linus tree, not only for a few days.
thanks,
Takashi
On Mon, Mar 01, 2010 at 03:06:57PM +0100, Takashi Iwai wrote:
At Mon, 1 Mar 2010 15:01:29 +0100, Daniel Mack wrote:
Ok, will do. However, I wonder which way is best to do that. Given that you posted patches on top of mine, can I take this as a general agreement to my changeset? If so, I will just provide a patch on top to do the ua101 -> misc rename, because otherwise, I would have to rebase all three patches for that.
Well, honestly, I prefer seeing the fixes by Clemens based on the current code, and merge Daniel's refactoring patches after 2.6.34 merge window.
Clemens fixes only make sense for v2 of the USB audio spec, which isn't fully supported yet. IOW, the code as it will go into 2.6.34 is not able to support that, with or without my refactoring. So rebasing them doesn't make sense IMO.
Refactoring is a good thing but it can easily introduce any careless bugs (like copy&paste error), and this split action makes really hard to track the changes. Thus I'd like to keep this well ripened before reaching to the Linus tree, not only for a few days.
Yes, certainly. I didn't expect that to be taken for .34.
I would be fine with having it in a branch on your side, and getting it merged to your for-next after any .34 issues are sorted out.
There will be quite some more patches to fully support the most common features, but they won't be as intrusive and big of course.
Daniel
At Mon, 1 Mar 2010 15:18:48 +0100, Daniel Mack wrote:
On Mon, Mar 01, 2010 at 03:06:57PM +0100, Takashi Iwai wrote:
At Mon, 1 Mar 2010 15:01:29 +0100, Daniel Mack wrote:
Ok, will do. However, I wonder which way is best to do that. Given that you posted patches on top of mine, can I take this as a general agreement to my changeset? If so, I will just provide a patch on top to do the ua101 -> misc rename, because otherwise, I would have to rebase all three patches for that.
Well, honestly, I prefer seeing the fixes by Clemens based on the current code, and merge Daniel's refactoring patches after 2.6.34 merge window.
Clemens fixes only make sense for v2 of the USB audio spec, which isn't fully supported yet. IOW, the code as it will go into 2.6.34 is not able to support that, with or without my refactoring. So rebasing them doesn't make sense IMO.
OK, then we can merge them together later.
Refactoring is a good thing but it can easily introduce any careless bugs (like copy&paste error), and this split action makes really hard to track the changes. Thus I'd like to keep this well ripened before reaching to the Linus tree, not only for a few days.
Yes, certainly. I didn't expect that to be taken for .34.
Good that we agree :)
I would be fine with having it in a branch on your side, and getting it merged to your for-next after any .34 issues are sorted out.
There will be quite some more patches to fully support the most common features, but they won't be as intrusive and big of course.
Fair enough.
thanks,
Takashi
On Mon, Mar 01, 2010 at 03:23:37PM +0100, Takashi Iwai wrote:
At Mon, 1 Mar 2010 15:18:48 +0100, Daniel Mack wrote:
Clemens fixes only make sense for v2 of the USB audio spec, which isn't fully supported yet. IOW, the code as it will go into 2.6.34 is not able to support that, with or without my refactoring. So rebasing them doesn't make sense IMO.
OK, then we can merge them together later.
Any plan on when to do that? I'm not in a hurry really, but I see the usbaudio driver moving on, so merging would need some manual backporting already. I can do that if you want and prepare a new patchset.
Btw - I'm curious how the commits e61e642c and 864c11080 made it to your tree. Wasn't the alsa-devel list copied for the patch or am I losing email?
Thanks, Daniel
At Thu, 4 Mar 2010 16:16:27 +0100, Daniel Mack wrote:
On Mon, Mar 01, 2010 at 03:23:37PM +0100, Takashi Iwai wrote:
At Mon, 1 Mar 2010 15:18:48 +0100, Daniel Mack wrote:
Clemens fixes only make sense for v2 of the USB audio spec, which isn't fully supported yet. IOW, the code as it will go into 2.6.34 is not able to support that, with or without my refactoring. So rebasing them doesn't make sense IMO.
OK, then we can merge them together later.
Any plan on when to do that? I'm not in a hurry really, but I see the usbaudio driver moving on, so merging would need some manual backporting already. I can do that if you want and prepare a new patchset.
You can prepare patches based on topic/misc branch of sound git tree, git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git topic/misc
or base on the latest Linus tree, which already contains USB audio v2.0 patches.
Then I'll create a separate branch (maybe again topic/usb-v2.0) and apply your patches, keep separate from linux-next branch.
Btw - I'm curious how the commits e61e642c and 864c11080 made it to your tree. Wasn't the alsa-devel list copied for the patch or am I losing email?
The former was a fix by Jaroslav directly done on git, and the latter came from LKML.
Takashi
On Thu, Mar 04, 2010 at 04:31:19PM +0100, Takashi Iwai wrote:
Daniel Mack wrote:
Any plan on when to do that? I'm not in a hurry really, but I see the usbaudio driver moving on, so merging would need some manual backporting already. I can do that if you want and prepare a new patchset.
You can prepare patches based on topic/misc branch of sound git tree, git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git topic/misc
or base on the latest Linus tree, which already contains USB audio v2.0 patches.
Then I'll create a separate branch (maybe again topic/usb-v2.0) and apply your patches, keep separate from linux-next branch.
Ok, will do. However, I wonder how new patches to the usbaudio driver will be handled then until the branch is merged?
Thanks, Daniel
At Thu, 4 Mar 2010 16:38:34 +0100, Daniel Mack wrote:
On Thu, Mar 04, 2010 at 04:31:19PM +0100, Takashi Iwai wrote:
Daniel Mack wrote:
Any plan on when to do that? I'm not in a hurry really, but I see the usbaudio driver moving on, so merging would need some manual backporting already. I can do that if you want and prepare a new patchset.
You can prepare patches based on topic/misc branch of sound git tree, git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git topic/misc
or base on the latest Linus tree, which already contains USB audio v2.0 patches.
Then I'll create a separate branch (maybe again topic/usb-v2.0) and apply your patches, keep separate from linux-next branch.
Ok, will do. However, I wonder how new patches to the usbaudio driver will be handled then until the branch is merged?
They are merged back to topic/usb-v2.0 branch. Conflicts are solved manually at merges.
Takashi
participants (3)
-
Clemens Ladisch -
Daniel Mack -
Takashi Iwai