Re: [alsa-devel] [PATCH 1/2 v5] ALSA: usb-audio: Add mixer control for Digidesign mbox 1 clock source

At Mon, 10 Nov 2014 21:17:54 +1100, Damien Zammit wrote:

+static int snd_mbox1_switch_get(struct snd_kcontrol *kctl,

• 		struct snd_ctl_elem_value *ucontrol)
  

+{

• if (kctl->private_value) {

• ucontrol->value.enumerated.item[0] = true;
  

• return 0;
  

• }
• ucontrol->value.enumerated.item[0] = false;

A simpler code would be just like:

ucontrol->value.enumerated.item[0] = kctl->private_value; return 0;

• return 0;

+}

+static int snd_mbox1_switch_put(struct snd_kcontrol *kctl,

• 		struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_usb_audio *chip;
• struct usb_mixer_interface *mixer;
• int err;
• bool changed, cur_val, new_val;
• bool value;

I guess this "value" variable can be dropped completely?

• unsigned char buff[3];
• changed = false;
• value = false;
• cur_val = kctl->private_value;
• new_val = ucontrol->value.enumerated.item[0];
• mixer = snd_kcontrol_chip(kctl);
• if (snd_BUG_ON(!mixer))

• return -EINVAL;
  

• chip = mixer->chip;
• if (snd_BUG_ON(!chip))

• return -EINVAL;
  

• /* update value if needed */
• if (cur_val != new_val) {

• value = new_val;
  

• down_read(&chip->shutdown_rwsem);
  

• if (chip->shutdown) {
  

• 	err = -ENODEV;
  

• 	goto err;
  

• } else {
  

• 	/* Prepare for magic command to toggle clock 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, buff, 1);
  

• 	if (err < 0)
  

• 		goto err;
  

• 	err = 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);
  

• 	if (err < 0)
  

• 		goto err;
  

• 	/* 2 possibilities:	Internal    -> send sample rate
  

• 	 *			S/PDIF sync -> send zeroes
  

• 	 * NB: Sample rate locked to 48kHz on purpose to
  

• 	 *     prevent user from resetting the sample rate
  

• 	 *     while S/PDIF sync is enabled and confusing
  

• 	 *     this configuration.
  

• 	 */
  

• 	if (new_val == 0) {
  

• 		buff[0] = 0x80;
  

• 		buff[1] = 0xbb;
  

• 		buff[2] = 0x00;
  

• 	} else {
  

• 		buff[0] = buff[1] = buff[2] = 0x00;
  

• 	}
  

• 	/* Send the magic command to toggle the clock source */
  

• 	err = snd_usb_ctl_msg(chip->dev,
  

• 		usb_sndctrlpipe(chip->dev, 0), 0x1,
  

• 		USB_TYPE_CLASS |
  

• 		USB_RECIP_ENDPOINT, 0x100, 0x81, buff, 3);
  

• 	if (err < 0)
  

• 		goto err;
  

• 	err = 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);
  

• 	if (err < 0)
  

• 		goto err;
  

• 	err = snd_usb_ctl_msg(chip->dev,
  

• 		usb_rcvctrlpipe(chip->dev, 0), 0x81,
  

• 		USB_DIR_IN |
  

• 		USB_TYPE_CLASS |
  

• 		USB_RECIP_ENDPOINT, 0x100, 0x2, buff, 3);
  

• 	if (err < 0)
  

• 		goto err;
  

• }
  

• up_read(&chip->shutdown_rwsem);
  

• kctl->private_value = new_val;
  

• changed = true;
  

• }
• return changed;

+err:

• up_read(&chip->shutdown_rwsem);
• return err;

A bit better way would be something like:

if (cur_val == new_val) return 0;

down_read(&chip->shutdown_rwsem); if (chip->shutdown) { err = -ENODEV; goto err; }

/* Prepare for magic command to toggle clock 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, buff, 1); if (err < 0) goto err; ....

kctl->private_value = new_val;

err: up_read(&chip->shutdown_rwsem); return err < 0 ? err : 1; }

This flattens the flow and combines the unlock in a single place. Also, "changed" variable can be dropped in this way.

Takashi