On Sun, 08 Aug 2021 07:09:31 +0200,
Damien Zammit wrote:
>
> This adds a second mixer control to Digidesign Mbox
> to select between Analog/SPDIF capture.
>
> Users will note that selecting the SPDIF input source
> automatically switches the clock mode to sync to SPDIF,
> which is a feature of the hardware.
>
> (You can change the clock source back to internal if you want
> to capture from spdif but not sync to its clock although this mode
> is probably not recommended).
>
> Unfortunately, starting the stream resets both modes
> to Internally clocked and Analog input mode.
Please add your Signed-off-by line. It's mandatory.
About the code change:
> @@ -596,31 +596,53 @@ static int snd_xonar_u1_controls_create(struct usb_mixer_interface *mixer)
>
> /* Digidesign Mbox 1 clock source switch (internal/spdif) */
>
> -static int snd_mbox1_switch_get(struct snd_kcontrol *kctl,
> - struct snd_ctl_elem_value *ucontrol)
> +static int snd_mbox1_clk_switch_get(struct snd_kcontrol *kctl,
> + struct snd_ctl_elem_value *ucontrol)
> {
> + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
> + struct snd_usb_audio *chip = list->mixer->chip;
> + unsigned char buff[3];
> +
You need to wrap the execution with snd_usb_lock_shutdown() /
snd_usb_unlock_shutdown().
> + /* Read clock source */
> + snd_usb_ctl_msg(chip->dev,
> + usb_rcvctrlpipe(chip->dev, 0), 0x81,
> + USB_DIR_IN |
> + USB_TYPE_CLASS |
> + USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
Please try to align the indent level.
And, the execution error check is missing.
Also, I believe it'd be wise to make it a helper function, as this is
used in another place, too. Ditto for other snd_usb_ctl_msg() calls,
too.
> +static int snd_mbox1_src_switch_get(struct snd_kcontrol *kctl,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct usb_mixer_elem_list *list = snd_kcontrol_chip(kctl);
> + struct snd_usb_audio *chip = list->mixer->chip;
> + unsigned char source[1];
Missing snd_usb_lock_shutdown() / *_unlock_*() here, too.
> + /* Read input source */
> + snd_usb_ctl_msg(chip->dev,
> + usb_rcvctrlpipe(chip->dev, 0), 0x81,
> + USB_DIR_IN |
> + USB_TYPE_CLASS |
> + USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);
Check the error.
> + kctl->private_value = source[0] - 1;
The value returned from the hardware isn't reliable. Check whether
it's a valid value.
> + ucontrol->value.enumerated.item[0] = kctl->private_value;
> + return 0;
> +}
> +
> +static int snd_mbox1_src_switch_update(struct usb_mixer_interface *mixer, int val)
> +{
> + struct snd_usb_audio *chip = mixer->chip;
> + int err;
> + unsigned char buff[3];
> + unsigned char source[1];
> +
> + err = snd_usb_lock_shutdown(chip);
> + if (err < 0)
> + return err;
> +
> + /* Read input source */
> + err = snd_usb_ctl_msg(chip->dev,
> + usb_rcvctrlpipe(chip->dev, 0), 0x81,
> + USB_DIR_IN |
> + USB_TYPE_CLASS |
> + USB_RECIP_INTERFACE, 0x00, 0x500, source, 1);
> + if (err < 0)
> + goto err;
> +
> + /* 2 possibilities: ANALOG Source -> 0x01
> + * S/PDIF Source -> 0x02
> + * Setting the input source to S/PDIF resets the clock source to S/PDIF
> + */
> + source[0] = (val + 1) & 0xff;
Here better to trust private_value is a valid value, and you can drop
the superfluous "& 0xff". It's cut down anyway.
thanks,
Takashi
