[PATCH] usb-audio: Input source control - digidesign mbox

[Takashi Iwai]

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