Re: [PATCH v2 1/1] ALSA: usb-audio: Add custom mixer status quirks for RME CC devices

[Takashi Iwai <tiwai@suse.de>]

On Thu, 04 Oct 2018 23:00:31 +0200,
Jussi Laako wrote:
> 
> On 04.10.2018 23:34, Takashi Iwai wrote:
> > Rather include <linux/math64.h>, and put in the appropriate order.
> 
> Sorry, I had a bit of hard time finding the function in first place,
> but what is the appropriate order?

No strict rule here.  Often people put in the alphabetical order, or
some logic (e.g. header dependencies).  In this case, just append to
linux/*.h files.

> > Since you're calling in the same pattern, it's better to consider to
> > provide a helper to call snd_usb_ctl_msg() with the given type (and
> > shows the error there).  The helper can take snd_usb_lock_shutdown()
> > and unlock in it, so it'll simplify a lot.
> 
> In first case I'm calling GET_STATUS1 and the lock surrounds that. In
> the second case I'm calling both GET_STATUS1 and GET_CURRENT_FREQ and
> the lock surrounds both calls togher. I didn't completely understand
> how to achieve this with the helper, unless you meant through some
> extra argument and control path inside?

This lock/unlock is not about spinlock or such race protection, but
just for a protection for the disconnection check.  So it's fine to
take twice, once for each.  As it's already with the USB ctl msg, it's
no real-time task, after all.

That is, a picture I had in my mind is something like:

static int rme_vendor_verb(struct snd_kcontrol *kcontrol, u32 cmd,
			   u32 *result)
{
	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol);
	struct snd_usb_audio *chip = list->mixer->chip;
	struct usb_device *dev = chip->dev;
	int err;

	err = snd_usb_lock_shutdown(chip);
	if (err < 0)
		return err;

	err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), cmd,
			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
			      0, 0, result, sizeof(*result));
	if (err < 0)
		dev_err(&dev->dev,
			"unable to issue vendor read request (ret = %d)", err);

	snd_usb_unlock_shutdown(chip);
	return err;
}

... and call it like
	err = rme_vendor_verb(kcontrol, RME_GET_STATUS1, &status1);


Takashi