[alsa-devel] [Fwd: Scratch Live amplifier switch]
Hi. Following the patch to add the amplifier switch (changing input levels phono/line) for the Serato Scratch Live device.
Please apply or complain ;)
At Fri, 28 Aug 2009 09:52:45 +0200, LCID Fire wrote:
Hi. Following the patch to add the amplifier switch (changing input levels phono/line) for the Serato Scratch Live device.
Please apply or complain ;)
First off, try $LINUX/scripts/checkpatch.pl and fix complains there. Also, there are too many dead codes. If they have to be there, keep in a bit more readable style please.
+/* Wrapper for setting and submitting the interrupt urb().*/ +static int snd_usb_interrupt_trans(struct usb_device *dev, unsigned int pipe, void *data,
__u16 size, usb_complete_t callback)+{
- int err;
- void *buf = NULL;
- struct urb* pUrb = NULL;
- if (size > 0) {
buf = kmemdup(data, size, GFP_KERNEL);if (!buf)return -ENOMEM;- }
- pUrb = usb_alloc_urb(1/*int iso packets*/, GFP_KERNEL);
- if (!pUrb)
return -ENOMEM;
A memory leak here. Free buf in the error path.
- /*TODO: Remove hardcoded 4*/
- usb_fill_int_urb(pUrb, dev, pipe, buf, size, callback, NULL, 4);
- err = usb_submit_urb(pUrb, GFP_KERNEL);
- return err;
Ditto.
+/* gets called multiple times! */ +static int snd_sl_controls_create(struct usb_mixer_interface *mixer) +{
- int i, err;
- for (i = 0; i < ARRAY_SIZE(snd_sl_controls); ++i) {
struct snd_ctl_elem_value ucontrol;struct snd_kcontrol *kcontrol = snd_ctl_new1(&snd_sl_controls[i], mixer);
Missing NULL check here.
/* Phono input is on by default */ucontrol.value.integer.value[0] = 1;err = snd_sl_phono_put(kcontrol, &ucontrol);
I'd recommend not to use put callback. snd_kcontrol_value is a big struct, so it should be avoided to be used on the stack as much as possible. Better to take the accessor part from the put callback and call it from here.
Could you fix and repost?
Thanks,
Takashi
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Takashi Iwai wrote:
I'd recommend not to use put callback. snd_kcontrol_value is a big struct, so it should be avoided to be used on the stack as much as possible. Better to take the accessor part from the put callback and call it from here.
Could you fix and repost?
I reworked the patch and it should be better now (with regards to multiple issues). As always - apply or complain :)
At Fri, 16 Oct 2009 21:52:48 +0200, LCID Fire wrote:
Signed-off-by: Andreas Bergmeier andreas.bergmeier@gmx.net
Thanks for the renewed patch. Below is a quick review.
diff --git a/sound/usb/usbmixer.c b/sound/usb/usbmixer.c index 00397c8..532d02d 100644 --- a/sound/usb/usbmixer.c +++ b/sound/usb/usbmixer.c @@ -2013,6 +2013,281 @@ static void snd_audigy2nx_proc_read(struct snd_info_entry *entry, } }
+static void snd_usb_cleanup_interrupt_urb(struct urb *pUrb)
Avoid Windows-style variable names as much as possible.
+{
- if (pUrb->transfer_buffer_length > 0)
kfree(pUrb->transfer_buffer);- kfree(pUrb->context);
- usb_free_urb(pUrb);
+}
+/* Wrapper for setting and submitting the interrupt urb().*/ +static int snd_usb_interrupt_trans(struct usb_device *dev,
- unsigned int pipe, void *data, __u16 size, usb_complete_t callback)
No need to use __u16. Here it can be simply unsigned int.
+{
- int err = 0;
- void *buf = NULL;
- struct urb *pUrb = NULL;
No need for initializations of all of these.
(snip)
+/* gets called multiple times! */ +static int snd_sl_controls_create(struct usb_mixer_interface *mixer) +{
- int i, err;
- for (i = 0; i < ARRAY_SIZE(snd_sl_controls); ++i) {
struct snd_kcontrol *kcontrol =snd_ctl_new1(&snd_sl_controls[i], mixer);
Missing NULL check here.
/* Since we only need one control and the routine* is called multiple times we have to ignore all* attempts to attach controls multiple times*/
Hm, why is it called so many times? Can't it be limited with a check of iface or altsetting?
if (snd_ctl_find_id(mixer->chip->card, &kcontrol->id)) {/* we found the control - it is already present* so just continue*/snd_ctl_free_one(kcontrol);continue;}/* add frees the control if on err < 0! */err = snd_ctl_add(mixer->chip->card,kcontrol);if (err < 0)return err;/* Make sure device is set to default */err = snd_sl_phono_set(kcontrol, kcontrol->private_value);if (err < 0) {snd_ctl_free_one(kcontrol);
Don't free this after adding via snd_ctl_add(). Just keep it. The final destructor will clean up everything eventually.
thanks,
Takashi
participants (2)
-
LCID Fire -
Takashi Iwai