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@xxxxxxxxxxxxx>
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
