| Message ID | 1415059597-18344-2-git-send-email-damien@zamaudio.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Takashi Iwai |
| Headers | show |
At Tue, 4 Nov 2014 11:06:36 +1100, Damien Zammit wrote: > > Signed-off-by: Damien Zammit <damien@zamaudio.com> > --- > sound/usb/mixer_maps.c | 9 +++ > sound/usb/mixer_quirks.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 186 insertions(+) > > diff --git a/sound/usb/mixer_maps.c b/sound/usb/mixer_maps.c > index d1d72ff..9ee3fc2 100644 > --- a/sound/usb/mixer_maps.c > +++ b/sound/usb/mixer_maps.c > @@ -179,6 +179,11 @@ static struct usbmix_name_map audigy2nx_map[] = { > { 0 } /* terminator */ > }; > > +static struct usbmix_name_map mbox1_map[] = { > + { 1, "Control" }, "Control" sounds too ambiguous. > + { 0 } /* terminator */ > +}; > + > static struct usbmix_selector_map c400_selectors[] = { > { > .id = 0x80, > @@ -416,6 +421,10 @@ static struct usbmix_ctl_map usbmix_ctl_maps[] = { > .map = aureon_51_2_map, > }, > { > + .id = USB_ID(0x0dba, 0x1000), > + .map = mbox1_map, > + }, > + { > .id = USB_ID(0x13e5, 0x0001), > .map = scratch_live_map, > .ignore_ctl_error = 1, > diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c > index 3980bf5..716c32c 100644 > --- a/sound/usb/mixer_quirks.c > +++ b/sound/usb/mixer_quirks.c > @@ -565,6 +565,176 @@ static int snd_xonar_u1_controls_create(struct usb_mixer_interface *mixer) > return 0; > } > > +/* Digidesign Mbox 1 clock source switch (internal/spdif) */ > + > +struct snd_mbox1_switch_priv_val { > + struct usb_mixer_interface *mixer; > + int cached_value; > + int is_cached; Use bool instead of int. Also true/false for 1/0. > +}; > + > +static int snd_mbox1_switch_get(struct snd_kcontrol *kctl, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_mbox1_switch_priv_val *pval; > + unsigned char value; > + > + value = 0x00; > + > + pval = (struct snd_mbox1_switch_priv_val *) > + kctl->private_value; > + > + if (pval->is_cached) { > + ucontrol->value.enumerated.item[0] = pval->cached_value; > + return 0; > + } > + > + ucontrol->value.enumerated.item[0] = value; > + pval->cached_value = value; > + pval->is_cached = 1; > + > + return 0; > +} > + > +static int snd_mbox1_switch_put(struct snd_kcontrol *kctl, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_usb_audio *chip; > + struct snd_mbox1_switch_priv_val *pval; > + > + struct usb_mixer_interface *mixer; > + int changed, cur_val, err, new_val; > + unsigned char value[2]; > + unsigned char buff[3]; > + > + changed = 0; > + value[0] = 0x00; > + value[1] = 0x00; > + > + pval = (struct snd_mbox1_switch_priv_val *) > + kctl->private_value; > + cur_val = pval->cached_value; > + new_val = ucontrol->value.enumerated.item[0]; > + > + mixer = (struct usb_mixer_interface *) pval->mixer; Superfluous cast. > + if (snd_BUG_ON(!mixer)) > + return -EINVAL; > + > + chip = (struct snd_usb_audio *) mixer->chip; Ditto. > + if (snd_BUG_ON(!chip)) > + return -EINVAL; > + > + if (!pval->is_cached) { > + cur_val = value[0]; > + pval->cached_value = cur_val; > + pval->is_cached = 1; > + } > + /* update value if needed */ > + if (cur_val != new_val) { > + value[0] = new_val; > + value[1] = 0; > + down_read(&mixer->chip->shutdown_rwsem); You can replace mixer->chip with chip (in below too). > + if (mixer->chip->shutdown) { > + err = -ENODEV; > + } else { > + err = snd_usb_ctl_msg(mixer->chip->dev, > + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, > + USB_DIR_IN | > + USB_TYPE_CLASS | > + USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1); > + if (err < 0) > + return err; > + err = snd_usb_ctl_msg(mixer->chip->dev, > + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, > + USB_DIR_IN | > + USB_TYPE_CLASS | > + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); > + if (err < 0) > + return err; What do these two messages and why needed? > + if (new_val == 0) { > + buff[0] = 0x80; > + buff[1] = 0xbb; > + buff[2] = 0x00; > + } else { > + buff[0] = buff[1] = buff[2] = 0; > + } > + err = snd_usb_ctl_msg(mixer->chip->dev, > + usb_sndctrlpipe(mixer->chip->dev, 0), 0x1, > + USB_TYPE_CLASS | > + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); > + if (err < 0) > + return err; > + err = snd_usb_ctl_msg(mixer->chip->dev, > + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, > + USB_DIR_IN | > + USB_TYPE_CLASS | > + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); > + if (err < 0) > + return err; > + err = snd_usb_ctl_msg(mixer->chip->dev, > + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, > + USB_DIR_IN | > + USB_TYPE_CLASS | > + USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3); > + if (err < 0) > + return err; Ditto. > + } > + up_read(&mixer->chip->shutdown_rwsem); > + if (err < 0) > + return err; > + > + pval->cached_value = new_val; > + pval->is_cached = 1; > + changed = 1; > + } > + > + return changed; > +} > + > +static int snd_mbox1_switch_info(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_info *uinfo) > +{ > + static const char *const texts[2] = { > + "Internal", > + "S/PDIF" > + }; > + > + return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts); > +} > + > +static int snd_mbox1_create_sync_switch(struct usb_mixer_interface *mixer) > +{ > + static struct snd_kcontrol_new template = { > + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, > + .name = "Clock Source", > + .index = 0, > + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE, > + .info = snd_mbox1_switch_info, > + .get = snd_mbox1_switch_get, > + .put = snd_mbox1_switch_put > + }; > + > + struct snd_kcontrol *kctl; > + struct snd_mbox1_switch_priv_val *pval; > + > + pval = kzalloc(sizeof(*pval), GFP_KERNEL); > + if (!pval) > + return -ENOMEM; > + > + pval->cached_value = 0; > + pval->is_cached = 0; > + pval->mixer = mixer; > + > + template.private_value = (unsigned long) pval; This is basically racy (although it wouldn't happen practically much) since static variable isn't local, thus two concurrent accesses overwrite with each other. Set kct->private_value instead of the template below. > + kctl = snd_ctl_new1(&template, mixer->chip); > + if (!kctl) { > + kfree(pval); > + return -ENOMEM; > + } You need to set private_free. Otherwise pval will be leaked. thanks, Takashi > + > + return snd_ctl_add(mixer->chip->card, kctl); > +} > + > /* Native Instruments device quirks */ > > #define _MAKE_NI_CONTROL(bRequest,wIndex) ((bRequest) << 16 | (wIndex)) > @@ -1605,6 +1775,13 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer) > snd_audigy2nx_proc_read); > break; > > + /* Digidesign Mbox 1 */ > + case USB_ID(0x0dba, 0x1000): > + err = snd_mbox1_create_sync_switch(mixer); > + if (err < 0) > + break; > + break; > + > /* EMU0204 */ > case USB_ID(0x041e, 0x3f19): > err = snd_emu0204_controls_create(mixer); > -- > 1.9.1 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
Hi Takashi, On 07/11/14 01:12, Takashi Iwai wrote: > At Tue, 4 Nov 2014 11:06:36 +1100, > Damien Zammit wrote: >> + down_read(&mixer->chip->shutdown_rwsem); I dont know what down_read and up_read does. >> + if (mixer->chip->shutdown) { >> + err = -ENODEV; >> + } else { >> + err = snd_usb_ctl_msg(mixer->chip->dev, >> + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, >> + USB_DIR_IN | >> + USB_TYPE_CLASS | >> + USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1); >> + if (err < 0) >> + return err; >> + err = snd_usb_ctl_msg(mixer->chip->dev, >> + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, >> + USB_DIR_IN | >> + USB_TYPE_CLASS | >> + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); >> + if (err < 0) >> + return err; > > What do these two messages and why needed? These two messages are the first half of the SPDIF mode selection. The device does not respond to standard mixer commands. I snooped the bus in Windows and replayed the packets when selecting SPDIF mode. Basically it appears that this command sets the mode of the device to receive the SPDIF clock source setting. >> + if (new_val == 0) { >> + buff[0] = 0x80; >> + buff[1] = 0xbb; >> + buff[2] = 0x00; >> + } else { >> + buff[0] = buff[1] = buff[2] = 0; >> + } If the mixer control is cleared to internal clock mode, the device expects the next command to have the sample rate fed in. Otherwise if the mixer control is set to spdif mode, the buffer is fed 3 zero bytes instead. It does not work as expected if the sample rate is allowed to be changed by the user, therefore I locked the sample rate to 48kHz. >> + err = snd_usb_ctl_msg(mixer->chip->dev, >> + usb_sndctrlpipe(mixer->chip->dev, 0), 0x1, >> + USB_TYPE_CLASS | >> + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); >> + if (err < 0) >> + return err; >> + err = snd_usb_ctl_msg(mixer->chip->dev, >> + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, >> + USB_DIR_IN | >> + USB_TYPE_CLASS | >> + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); >> + if (err < 0) >> + return err; >> + err = snd_usb_ctl_msg(mixer->chip->dev, >> + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, >> + USB_DIR_IN | >> + USB_TYPE_CLASS | >> + USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3); >> + if (err < 0) >> + return err; This is where the magic happens... >> + } >> + up_read(&mixer->chip->shutdown_rwsem); No idea what up_read is for. Thanks for the review, and I will post a new patch soon. How do I tell git send-email to reply to this thread? Thanks, Damien
At Sun, 09 Nov 2014 00:30:48 +1100, Damien Zammit wrote: > > Hi Takashi, > > On 07/11/14 01:12, Takashi Iwai wrote: > > At Tue, 4 Nov 2014 11:06:36 +1100, > > Damien Zammit wrote: > >> + down_read(&mixer->chip->shutdown_rwsem); > I dont know what down_read and up_read does. This is to protect against the disconnect and autopm. If you touch the USB hardware, do lock with this. > >> + if (mixer->chip->shutdown) { > >> + err = -ENODEV; > >> + } else { > >> + err = snd_usb_ctl_msg(mixer->chip->dev, > >> + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, > >> + USB_DIR_IN | > >> + USB_TYPE_CLASS | > >> + USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1); > >> + if (err < 0) > >> + return err; > >> + err = snd_usb_ctl_msg(mixer->chip->dev, > >> + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, > >> + USB_DIR_IN | > >> + USB_TYPE_CLASS | > >> + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); > >> + if (err < 0) > >> + return err; > > > > What do these two messages and why needed? > > These two messages are the first half of the SPDIF mode selection. > The device does not respond to standard mixer commands. > I snooped the bus in Windows and replayed the packets when selecting > SPDIF mode. Basically it appears that this command sets the mode of the > device to receive the SPDIF clock source setting. Then add the comment explaining that. There is absolutely no reason not to add any comment for such a black magic. > >> + if (new_val == 0) { > >> + buff[0] = 0x80; > >> + buff[1] = 0xbb; > >> + buff[2] = 0x00; > >> + } else { > >> + buff[0] = buff[1] = buff[2] = 0; > >> + } > If the mixer control is cleared to internal clock mode, the device > expects the next command to have the sample rate fed in. Otherwise if > the mixer control is set to spdif mode, the buffer is fed 3 zero bytes > instead. It does not work as expected if the sample rate is allowed to > be changed by the user, therefore I locked the sample rate to 48kHz. Ditto. Give more comments. > >> + err = snd_usb_ctl_msg(mixer->chip->dev, > >> + usb_sndctrlpipe(mixer->chip->dev, 0), 0x1, > >> + USB_TYPE_CLASS | > >> + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); > >> + if (err < 0) > >> + return err; > >> + err = snd_usb_ctl_msg(mixer->chip->dev, > >> + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, > >> + USB_DIR_IN | > >> + USB_TYPE_CLASS | > >> + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); > >> + if (err < 0) > >> + return err; > >> + err = snd_usb_ctl_msg(mixer->chip->dev, > >> + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, > >> + USB_DIR_IN | > >> + USB_TYPE_CLASS | > >> + USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3); > >> + if (err < 0) > >> + return err; > This is where the magic happens... And yet give more comments. > >> + up_read(&mixer->chip->shutdown_rwsem); > No idea what up_read is for. > > Thanks for the review, and I will post a new patch soon. > How do I tell git send-email to reply to this thread? For the revised patch series, you shouldn't connect to the old thread. Hanging too deeply make often rather difficult to read. Takashi
diff --git a/sound/usb/mixer_maps.c b/sound/usb/mixer_maps.c index d1d72ff..9ee3fc2 100644 --- a/sound/usb/mixer_maps.c +++ b/sound/usb/mixer_maps.c @@ -179,6 +179,11 @@ static struct usbmix_name_map audigy2nx_map[] = { { 0 } /* terminator */ }; +static struct usbmix_name_map mbox1_map[] = { + { 1, "Control" }, + { 0 } /* terminator */ +}; + static struct usbmix_selector_map c400_selectors[] = { { .id = 0x80, @@ -416,6 +421,10 @@ static struct usbmix_ctl_map usbmix_ctl_maps[] = { .map = aureon_51_2_map, }, { + .id = USB_ID(0x0dba, 0x1000), + .map = mbox1_map, + }, + { .id = USB_ID(0x13e5, 0x0001), .map = scratch_live_map, .ignore_ctl_error = 1, diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c index 3980bf5..716c32c 100644 --- a/sound/usb/mixer_quirks.c +++ b/sound/usb/mixer_quirks.c @@ -565,6 +565,176 @@ static int snd_xonar_u1_controls_create(struct usb_mixer_interface *mixer) return 0; } +/* Digidesign Mbox 1 clock source switch (internal/spdif) */ + +struct snd_mbox1_switch_priv_val { + struct usb_mixer_interface *mixer; + int cached_value; + int is_cached; +}; + +static int snd_mbox1_switch_get(struct snd_kcontrol *kctl, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_mbox1_switch_priv_val *pval; + unsigned char value; + + value = 0x00; + + pval = (struct snd_mbox1_switch_priv_val *) + kctl->private_value; + + if (pval->is_cached) { + ucontrol->value.enumerated.item[0] = pval->cached_value; + return 0; + } + + ucontrol->value.enumerated.item[0] = value; + pval->cached_value = value; + pval->is_cached = 1; + + return 0; +} + +static int snd_mbox1_switch_put(struct snd_kcontrol *kctl, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_usb_audio *chip; + struct snd_mbox1_switch_priv_val *pval; + + struct usb_mixer_interface *mixer; + int changed, cur_val, err, new_val; + unsigned char value[2]; + unsigned char buff[3]; + + changed = 0; + value[0] = 0x00; + value[1] = 0x00; + + pval = (struct snd_mbox1_switch_priv_val *) + kctl->private_value; + cur_val = pval->cached_value; + new_val = ucontrol->value.enumerated.item[0]; + + mixer = (struct usb_mixer_interface *) pval->mixer; + if (snd_BUG_ON(!mixer)) + return -EINVAL; + + chip = (struct snd_usb_audio *) mixer->chip; + if (snd_BUG_ON(!chip)) + return -EINVAL; + + if (!pval->is_cached) { + cur_val = value[0]; + pval->cached_value = cur_val; + pval->is_cached = 1; + } + /* update value if needed */ + if (cur_val != new_val) { + value[0] = new_val; + value[1] = 0; + down_read(&mixer->chip->shutdown_rwsem); + if (mixer->chip->shutdown) { + err = -ENODEV; + } else { + err = snd_usb_ctl_msg(mixer->chip->dev, + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, + USB_DIR_IN | + USB_TYPE_CLASS | + USB_RECIP_INTERFACE, 0x00, 0x500, buff, 1); + if (err < 0) + return err; + err = snd_usb_ctl_msg(mixer->chip->dev, + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, + USB_DIR_IN | + USB_TYPE_CLASS | + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); + if (err < 0) + return err; + if (new_val == 0) { + buff[0] = 0x80; + buff[1] = 0xbb; + buff[2] = 0x00; + } else { + buff[0] = buff[1] = buff[2] = 0; + } + err = snd_usb_ctl_msg(mixer->chip->dev, + usb_sndctrlpipe(mixer->chip->dev, 0), 0x1, + USB_TYPE_CLASS | + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); + if (err < 0) + return err; + err = snd_usb_ctl_msg(mixer->chip->dev, + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, + USB_DIR_IN | + USB_TYPE_CLASS | + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3); + if (err < 0) + return err; + err = snd_usb_ctl_msg(mixer->chip->dev, + usb_rcvctrlpipe(mixer->chip->dev, 0), 0x81, + USB_DIR_IN | + USB_TYPE_CLASS | + USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3); + if (err < 0) + return err; + } + up_read(&mixer->chip->shutdown_rwsem); + if (err < 0) + return err; + + pval->cached_value = new_val; + pval->is_cached = 1; + changed = 1; + } + + return changed; +} + +static int snd_mbox1_switch_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + static const char *const texts[2] = { + "Internal", + "S/PDIF" + }; + + return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts); +} + +static int snd_mbox1_create_sync_switch(struct usb_mixer_interface *mixer) +{ + static struct snd_kcontrol_new template = { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = "Clock Source", + .index = 0, + .access = SNDRV_CTL_ELEM_ACCESS_READWRITE, + .info = snd_mbox1_switch_info, + .get = snd_mbox1_switch_get, + .put = snd_mbox1_switch_put + }; + + struct snd_kcontrol *kctl; + struct snd_mbox1_switch_priv_val *pval; + + pval = kzalloc(sizeof(*pval), GFP_KERNEL); + if (!pval) + return -ENOMEM; + + pval->cached_value = 0; + pval->is_cached = 0; + pval->mixer = mixer; + + template.private_value = (unsigned long) pval; + kctl = snd_ctl_new1(&template, mixer->chip); + if (!kctl) { + kfree(pval); + return -ENOMEM; + } + + return snd_ctl_add(mixer->chip->card, kctl); +} + /* Native Instruments device quirks */ #define _MAKE_NI_CONTROL(bRequest,wIndex) ((bRequest) << 16 | (wIndex)) @@ -1605,6 +1775,13 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer) snd_audigy2nx_proc_read); break; + /* Digidesign Mbox 1 */ + case USB_ID(0x0dba, 0x1000): + err = snd_mbox1_create_sync_switch(mixer); + if (err < 0) + break; + break; + /* EMU0204 */ case USB_ID(0x041e, 0x3f19): err = snd_emu0204_controls_create(mixer);
Signed-off-by: Damien Zammit <damien@zamaudio.com> --- sound/usb/mixer_maps.c | 9 +++ sound/usb/mixer_quirks.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+)