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

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

[Jussi Laako]

This patch has been updated with following changes and should be now
good for merging:
- Replace 64-bit division with suitable function to make it available
  on all architectures
- Use specific VID:PID pairs instead of just VID to avoid confusions

Last item was changed after discussion with RME.

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

[Jussi Laako]

Adds several vendor specific mixer quirks for RME's Class Compliant
USB devices. These provide extra status information from the device
otherwise not available.

These include AES/SPDIF rate and status information, current system
sampling rate and measured frequency. This information is especially
useful in cases where device's clock is slaved to external clock
source.

Signed-off-by: Jussi Laako <jussi@sonarnerd.net>
---
 sound/usb/mixer_quirks.c | 349 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 349 insertions(+)

diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index cbfb48bdea51..d75e1f025b3b 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -27,6 +27,8 @@
  *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
  */
 
+#include <asm/div64.h>
+
 #include <linux/hid.h>
 #include <linux/init.h>
 #include <linux/slab.h>
@@ -1817,6 +1819,347 @@ static int dell_dock_mixer_init(struct usb_mixer_interface *mixer)
 	return 0;
 }
 
+/* RME Class Compliant device quirks */
+
+static const u32 snd_rme_rate_table[] = {
+	32000, 44100, 48000, 50000,
+	64000, 88200, 96000, 100000,
+	128000, 176400, 192000, 200000,
+	256000,	352800, 384000, 400000,
+	512000, 705600, 768000, 800000
+};
+
+static int snd_rme_get_status1(struct snd_kcontrol *kcontrol,
+			       u32 *status1)
+{
+	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),
+			      23,  /* GET_STATUS1 */
+			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      0, 0,
+			      status1, sizeof(*status1));
+	if (err < 0) {
+		dev_err(&dev->dev,
+			"unable to issue vendor read request (ret = %d)", err);
+		goto end;
+	}
+
+end:
+	snd_usb_unlock_shutdown(chip);
+	return err;
+}
+
+static int snd_rme_rate_get(struct snd_kcontrol *kcontrol,
+			    struct snd_ctl_elem_value *ucontrol)
+{
+	u32 status1;
+	u32 rate = 0;
+	int idx;
+	int err;
+
+	err = snd_rme_get_status1(kcontrol, &status1);
+	if (err < 0)
+		return err;
+
+	switch (kcontrol->private_value)
+	{
+	case 0:  /* System */
+		idx = (status1 >> 16) & 0x1f;
+		if (idx < ARRAY_SIZE(snd_rme_rate_table))
+			rate = snd_rme_rate_table[idx];
+		break;
+	case 1:  /* AES */
+		idx = (status1 >> 8) & 0xf;
+		if (idx < 12)
+			rate = snd_rme_rate_table[idx];
+		break;
+	case 2:  /* SPDIF */
+		idx = (status1 >> 12) & 0xf;
+		if (idx < 12)
+			rate = snd_rme_rate_table[idx];
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ucontrol->value.integer.value[0] = rate;
+	return 0;
+}
+
+static int snd_rme_sync_state_get(struct snd_kcontrol *kcontrol,
+				  struct snd_ctl_elem_value *ucontrol)
+{
+	u32 status1;
+	int idx = 0;
+	int err;
+
+	err = snd_rme_get_status1(kcontrol, &status1);
+	if (err < 0)
+		return err;
+
+	switch (kcontrol->private_value)
+	{
+	case 1:  /* AES */
+		if (status1 & 0x4)
+			idx = 2;
+		else if (status1 & 0x1)
+			idx = 1;
+		break;
+	case 2:  /* SPDIF */
+		if (status1 & 0x8)
+			idx = 2;
+		else if (status1 & 0x2)
+			idx = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ucontrol->value.enumerated.item[0] = idx;
+	return 0;
+}
+
+static int snd_rme_spdif_if_get(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	u32 status1;
+	int err;
+
+	err = snd_rme_get_status1(kcontrol, &status1);
+	if (err < 0)
+		return err;
+
+	ucontrol->value.enumerated.item[0] = (status1 >> 4) & 0x1;
+	return 0;
+}
+
+static int snd_rme_spdif_format_get(struct snd_kcontrol *kcontrol,
+				    struct snd_ctl_elem_value *ucontrol)
+{
+	u32 status1;
+	int err;
+
+	err = snd_rme_get_status1(kcontrol, &status1);
+	if (err < 0)
+		return err;
+
+	ucontrol->value.enumerated.item[0] = (status1 >> 5) & 0x1;
+	return 0;
+}
+
+static int snd_rme_sync_source_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	u32 status1;
+	int err;
+
+	err = snd_rme_get_status1(kcontrol, &status1);
+	if (err < 0)
+		return err;
+
+	ucontrol->value.enumerated.item[0] = (status1 >> 6) & 0x3;
+	return 0;
+}
+
+static int snd_rme_current_freq_get(struct snd_kcontrol *kcontrol,
+				    struct snd_ctl_elem_value *ucontrol)
+{
+	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol);
+	struct snd_usb_audio *chip = list->mixer->chip;
+	struct usb_device *dev = chip->dev;
+	u32 status1;
+	const u64 num = 104857600000000;
+	u32 den;
+	unsigned int freq;
+	int err;
+
+	err = snd_usb_lock_shutdown(chip);
+	if (err < 0)
+		return err;
+
+	err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0),
+			      23,  /* GET_STATUS1 */
+			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      0, 0,
+			      &status1, sizeof(status1));
+	if (err < 0) {
+		dev_err(&dev->dev,
+			"unable to issue vendor read request (ret = %d)", err);
+		goto end;
+	}
+
+	err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0),
+			      17,  /* GET_CURRENT_FREQ */
+			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+			      0, 0,
+			      &den, sizeof(den));
+	if (err < 0) {
+		dev_err(&dev->dev,
+			"unable to issue vendor read request (ret = %d)", err);
+		goto end;
+	}
+	freq = (den == 0) ? 0 : div64_u64(num, den);
+
+	freq <<= ((status1 >> 18) & 0x7);
+	ucontrol->value.integer.value[0] = freq;
+
+end:
+	snd_usb_unlock_shutdown(chip);
+	return err;
+}
+
+static int snd_rme_rate_info(struct snd_kcontrol *kcontrol,
+			     struct snd_ctl_elem_info *uinfo)
+{
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min =
+		(kcontrol->private_value > 0) ? 0 : 32000;
+	uinfo->value.integer.max =
+		(kcontrol->private_value > 0) ? 200000 : 800000;
+	uinfo->value.integer.step = 0;
+	return 0;
+}
+
+static int snd_rme_sync_state_info(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_info *uinfo)
+{
+	static const char *const sync_states[] = {
+		"No Lock", "Lock", "Sync"
+	};
+
+	return snd_ctl_enum_info(uinfo, 1,
+				 ARRAY_SIZE(sync_states), sync_states);
+}
+
+static int snd_rme_spdif_if_info(struct snd_kcontrol *kcontrol,
+				 struct snd_ctl_elem_info *uinfo)
+{
+	static const char *const spdif_if[] = {
+		"Coaxial", "Optical"
+	};
+
+	return snd_ctl_enum_info(uinfo, 1,
+				 ARRAY_SIZE(spdif_if), spdif_if);
+}
+
+static int snd_rme_spdif_format_info(struct snd_kcontrol *kcontrol,
+				     struct snd_ctl_elem_info *uinfo)
+{
+	static const char *const optical_type[] = {
+		"Consumer", "Professional"
+	};
+
+	return snd_ctl_enum_info(uinfo, 1,
+				 ARRAY_SIZE(optical_type), optical_type);
+}
+
+static int snd_rme_sync_source_info(struct snd_kcontrol *kcontrol,
+				    struct snd_ctl_elem_info *uinfo)
+{
+	static const char *const sync_sources[] = {
+		"Internal", "AES", "SPDIF", "Internal"
+	};
+
+	return snd_ctl_enum_info(uinfo, 1,
+				 ARRAY_SIZE(sync_sources), sync_sources);
+}
+
+static struct snd_kcontrol_new snd_rme_controls[] = {
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "AES Rate",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_rate_info,
+		.get = snd_rme_rate_get,
+		.private_value = 1
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "AES Sync",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_sync_state_info,
+		.get = snd_rme_sync_state_get,
+		.private_value = 1
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "SPDIF Rate",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_rate_info,
+		.get = snd_rme_rate_get,
+		.private_value = 2
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "SPDIF Sync",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_sync_state_info,
+		.get = snd_rme_sync_state_get,
+		.private_value = 2
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "SPDIF Interface",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_spdif_if_info,
+		.get = snd_rme_spdif_if_get,
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "SPDIF Format",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_spdif_format_info,
+		.get = snd_rme_spdif_format_get,
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "Sync Source",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_sync_source_info,
+		.get = snd_rme_sync_source_get
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "System Rate",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_rate_info,
+		.get = snd_rme_rate_get,
+		.private_value = 0
+	},
+	{
+		.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+		.name = "Current Frequency",
+		.access = SNDRV_CTL_ELEM_ACCESS_READ | SNDRV_CTL_ELEM_ACCESS_VOLATILE,
+		.info = snd_rme_rate_info,
+		.get = snd_rme_current_freq_get
+	}
+};
+
+static int snd_rme_controls_create(struct usb_mixer_interface *mixer)
+{
+	int err, i;
+
+	for (i = 0; i < ARRAY_SIZE(snd_rme_controls); ++i) {
+		err = add_single_ctl_with_resume(mixer, 0,
+						 NULL,
+						 &snd_rme_controls[i],
+						 NULL);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
 int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 {
 	int err = 0;
@@ -1904,6 +2247,12 @@ int snd_usb_mixer_apply_create_quirk(struct usb_mixer_interface *mixer)
 	case USB_ID(0x0bda, 0x4014): /* Dell WD15 dock */
 		err = dell_dock_mixer_init(mixer);
 		break;
+
+	case USB_ID(0x2a39, 0x3fd2): /* RME ADI-2 Pro */
+	case USB_ID(0x2a39, 0x3fd3): /* RME ADI-2 DAC */
+	case USB_ID(0x2a39, 0x3fd4): /* RME */
+		err = snd_rme_controls_create(mixer);
+		break;
 	}
 
 	return err;
-- 
2.17.1

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

[Takashi Iwai]

On Thu, 04 Oct 2018 21:17:43 +0200,
Jussi Laako wrote:
> 
> Adds several vendor specific mixer quirks for RME's Class Compliant
> USB devices. These provide extra status information from the device
> otherwise not available.
> 
> These include AES/SPDIF rate and status information, current system
> sampling rate and measured frequency. This information is especially
> useful in cases where device's clock is slaved to external clock
> source.
> 
> Signed-off-by: Jussi Laako <jussi@sonarnerd.net>

The implementations look mostly OK, but just some nitpicking:

> ---
>  sound/usb/mixer_quirks.c | 349 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 349 insertions(+)
> 
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index cbfb48bdea51..d75e1f025b3b 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -27,6 +27,8 @@
>   *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
>   */
>  
> +#include <asm/div64.h>
> +

Rather include <linux/math64.h>, and put in the appropriate order.

>  #include <linux/hid.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> @@ -1817,6 +1819,347 @@ static int dell_dock_mixer_init(struct usb_mixer_interface *mixer)
>  	return 0;
>  }
>  
> +/* RME Class Compliant device quirks */
> +
> +static const u32 snd_rme_rate_table[] = {
> +	32000, 44100, 48000, 50000,
> +	64000, 88200, 96000, 100000,
> +	128000, 176400, 192000, 200000,
> +	256000,	352800, 384000, 400000,
> +	512000, 705600, 768000, 800000
> +};
> +
> +static int snd_rme_get_status1(struct snd_kcontrol *kcontrol,
> +			       u32 *status1)
> +{
> +	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),
> +			      23,  /* GET_STATUS1 */

This number (and the other later ones) should be defined instead of
putting in each commit.

> +			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +			      0, 0,
> +			      status1, sizeof(*status1));
> +	if (err < 0) {
> +		dev_err(&dev->dev,
> +			"unable to issue vendor read request (ret = %d)", err);
> +		goto end;
> +	}

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.

> +
> +end:
> +	snd_usb_unlock_shutdown(chip);
> +	return err;
> +}
> +
> +static int snd_rme_rate_get(struct snd_kcontrol *kcontrol,
> +			    struct snd_ctl_elem_value *ucontrol)
> +{
> +	u32 status1;
> +	u32 rate = 0;
> +	int idx;
> +	int err;
> +
> +	err = snd_rme_get_status1(kcontrol, &status1);
> +	if (err < 0)
> +		return err;
> +
> +	switch (kcontrol->private_value)
> +	{
> +	case 0:  /* System */

Let's use enum.

> +		idx = (status1 >> 16) & 0x1f;

And, define this magic number 16.

> +		if (idx < ARRAY_SIZE(snd_rme_rate_table))
> +			rate = snd_rme_rate_table[idx];
> +		break;
> +	case 1:  /* AES */
> +		idx = (status1 >> 8) & 0xf;

Ditto (also for the rest).

> +static int snd_rme_sync_state_get(struct snd_kcontrol *kcontrol,
> +				  struct snd_ctl_elem_value *ucontrol)
> +{
> +	u32 status1;
> +	int idx = 0;
> +	int err;
> +
> +	err = snd_rme_get_status1(kcontrol, &status1);
> +	if (err < 0)
> +		return err;
> +
> +	switch (kcontrol->private_value)
> +	{
> +	case 1:  /* AES */
> +		if (status1 & 0x4)
> +			idx = 2;
> +		else if (status1 & 0x1)
> +			idx = 1;

All these magic numbers should be defined as well.

> +static int snd_rme_current_freq_get(struct snd_kcontrol *kcontrol,
> +				    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol);
> +	struct snd_usb_audio *chip = list->mixer->chip;
> +	struct usb_device *dev = chip->dev;
> +	u32 status1;
> +	const u64 num = 104857600000000;

The 64bit const should be with ULL suffix.

> +static int snd_rme_rate_info(struct snd_kcontrol *kcontrol,
> +			     struct snd_ctl_elem_info *uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +	uinfo->count = 1;
> +	uinfo->value.integer.min =
> +		(kcontrol->private_value > 0) ? 0 : 32000;
> +	uinfo->value.integer.max =
> +		(kcontrol->private_value > 0) ? 200000 : 800000;

This better to be like
	switch (kcontrol->private_value) {
	case FOO:
		uinfo->value.integer.min = 0;
		uinfo->value.integer.min = 200000;
		break;
	case BAR:
		uinfo->value.integer.min = 32000;
		uinfo->value.integer.min = 800000;
		break;
	}


thanks,

Takashi

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

[Jussi Laako]

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?

> 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?


	- Jussi

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

[Takashi Iwai]

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