[alsa-devel] [PATCH] snd-usb-us122l v0.4 for US-122L

[Takashi Iwai]

At Mon, 14 Apr 2008 21:29:54 +0200,
Karsten Wiese wrote:
> 
> Hi,
> 
> there are 2 fixes since v0.3 here:
>  - compiles & works for x86 32bit again.
>  - hardware configuration is done via ioctl() instead of write().
>    Fixing a bug that led to jackd not starting again after a stop.

diff --git a/sound/usb/usbmidi.c b/sound/usb/usbmidi.c
index 6676a17..3c55dea 100644
--- a/sound/usb/usbmidi.c
+++ b/sound/usb/usbmidi.c
(snip)
+static void snd_usbmidi_us122l_output(struct snd_usb_midi_out_endpoint *ep)
(snip)
+	while (count < 9)
+		((char *)ep->urb->transfer_buffer)[count++] = 0xFD;

Use memset().

diff --git a/sound/usb/usx2y/Makefile b/sound/usb/usx2y/Makefile
index 9ac22bc..7489330 100644
--- a/sound/usb/usx2y/Makefile
+++ b/sound/usb/usx2y/Makefile
@@ -1,3 +1,5 @@
 snd-usb-usx2y-objs := usbusx2y.o usX2Yhwdep.o usx2yhwdeppcm.o
+snd-usb-us122l-objs := us122l.o
 
 obj-$(CONFIG_SND_USB_USX2Y) += snd-usb-usx2y.o
+obj-$(CONFIG_SND_USB_US122L) += snd-usb-us122l.o

Missing dependency to snd-usb-audio?
Maybe we need to split a helper module...

+static void pt_version(struct usb_device *dev)
+{
+	int ret;
+	u8 *buf = kmalloc(200, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
+			      'V',
+			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      0, 0, buf, 200, 1000);
+	if (ret < 0) {
+		P_DBG("%i", ret);
+		goto out;
+	}
+	P_VDBG("%u %u %u", buf[0], buf[1], buf[2]);
+	buf[ret] = 0;
+	P_VDBG(".%s.", buf + 3);
+
+out:
+	kfree(buf);
+}

This function is only for debugging, no?

+static int pt_info(struct usb_device *dev)
+{
+	int ret;
+	u8 *buf = kmalloc(200, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
+			      'I',
+			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      0, 0, buf, 200, 1000);
+	if (ret < 0) {
+		P_DBG("%i", ret);
+		goto out;
+	}
+	P_VDBG("0x%X", buf[0]);
+	ret = buf[0];
+
+out:
+	kfree(buf);
+	return ret;
+}

Ditto.

+static void pt_info_set(struct usb_device *dev, u8 v)
+{
+	int ret;
+
+	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+			      'I',
+			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      v, 0, NULL, 0, 1000);
+	P_VDBG("%i", ret);
+}

Ditto.

--- /dev/null
+++ b/sound/usb/usx2y/usb_stream.h
(snip)
+
+#define INTERFACE_VERSION 1
+#define KERNEL_64 0x80000000

Should be more unique name as this seems imported by user-space
apps...

+#define USB_STREAM_INTERFACE_VERSION \
+	((BITS_PER_LONG == 64 ? KERNEL_64 : 0) | INTERFACE_VERSION)
+
+#define SNDRV_USB_STREAM_IOCTL_SET_PARAMS \
+	_IOW('H', 0x90, struct usb_stream_config)
+
+#if !__KERNEL__

Usually #ifndef __KERNEL__

+struct usb_iso_packet_descriptor {
+	unsigned int offset;
+	unsigned int length;		/* expected length */
+	unsigned int actual_length;
+	int status;
+};

Why this is needed for !__KERNEL__ ?
I see only struct usb_stream_config is required.

+
+#endif
+
+#define USB_STREAM_NURBS 4
+struct usb_stream_config {
+	unsigned version;
+	unsigned sample_rate;
+	unsigned period_frames;
+	unsigned frame_size;
+};

The codes below here should be hidden to user-space.
If it's exported, then prepare more explicit struct.  For example,
pointers may have different sizes on user-space.


+#define P_WARN(fmt, ...) \
+	printk(KERN_WARNING"%s %s:%i "fmt"\n", \
+	       MODNAME, __func__, __LINE__, ## __VA_ARGS__);
+
+#define P_DBG(fmt, ...) \
+	printk(KERN_DEBUG"%s %s:%i "fmt"\n", \
+	       MODNAME, __func__, __LINE__, ## __VA_ARGS__);
+
+#define P_VDBG(fmt, ...)

Try to avoid own-printk macros as much as possible...


thanks,

Takashi