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