[alsa-devel] [PATCH 1/2] snd-usb-audio: Add mixer control for Digidesign Mbox 1 clock source

[Takashi Iwai]

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