Re: [RFC PATCH 13/16] ALSA: mixer_audient: Add enumerated controls

[Christian Ruppert <arc@gmx.li>]

[-- Attachment #1: Type: text/plain, Size: 11486 bytes --]

Hi Will,

Thanks for this update. I squashed it into patches 13 and 15 before
reviewing. I also tested everything on 7.3.3 and could not find any
issues. Here are my comments about the code, most of them are minor:

patch 12:
- I would declare snd_audient_int_max, snd_audient_int_min and
  snd_audient_int_from_ctl as inline, they are little more than macros.

patch 13:
- I would have snd_audient_enum_item return the negative errno instead of
  e->items and then propagate it. This seems more coherent with the
  convention applied almost everywhere else in the kernel.
- I wonder if it would make the code more readable if we refactor all the
  if-statements snd_ctl_audient_info has grown into something like DIFF 1
  below?

patch 15:
- snd_ctl_audient_enum_get uses ctype->init which is not set by the
  ID14_ENUM() macro. Is this intentionally always constant 0? In this
  case, could the logic be removed?
- Not sure if this is relevant for the iD14 but we might want to apply
  something like DIFF 2 to make update events work on the replaced
  controls 11 and possibly 60 (in case somebody fiddles with the buttons,
  please check if it actually works with the device). BTW: I'll see if we
  can somehow also get update events for the new controls of the EVO4 on
  the occasion. The events the generic mixer driver generates don't work
  for those so I didn't bother initializing the ID until now.
- In mixer_quirks.c: Fallthrough case statements seem to be used almost
  everywhere else, I suggest to replace the repeating call to
  snd_audient_controls_create with a fallthrough as well.

patch 16:
- can we rename 'create' to something which makes it obvious from the name
  that the callback is optional, e.g. 'extra_init' or similar?

  Christian

---- DIFF 1 ----
diff --git a/sound/usb/mixer_audient.c b/sound/usb/mixer_audient.c
index 4180d7d8b325..212a2156087c 100644
--- a/sound/usb/mixer_audient.c
+++ b/sound/usb/mixer_audient.c
@@ -1002,22 +1002,25 @@ static int snd_ctl_audient_info(struct snd_kcontrol *kctl,
 	const struct snd_audient_ctrl_type *const ctype
 						= snd_audient_kctl_type(kctl);
 
-	if (ctype->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
-		return snd_ctl_enum_info(uinfo, snd_audient_kctl_channels(kctl),
-					 ctype->enumeration->items,
-					 ctype->enumeration->texts);
+	switch (ctype->type) {
+		case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
+			return snd_ctl_enum_info(uinfo,
+					snd_audient_kctl_channels(kctl),
+					ctype->enumeration->items,
+					ctype->enumeration->texts);
+		case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
+			uinfo->value.integer.min = 0;
+			uinfo->value.integer.max = 1;
+			break;
+		default:
+			uinfo->value.integer.min = snd_audient_int_min(ctype);
+			uinfo->value.integer.max = snd_audient_int_max(ctype);
+			break;
+	}
 
 	uinfo->type = ctype->type;
 	uinfo->count = snd_audient_kctl_channels(kctl);
 
-	if (ctype->type == SNDRV_CTL_ELEM_TYPE_BOOLEAN) {
-		uinfo->value.integer.min = 0;
-		uinfo->value.integer.max = 1;
-	} else {
-		uinfo->value.integer.min = snd_audient_int_min(ctype);
-		uinfo->value.integer.max = snd_audient_int_max(ctype);
-	}
-
 	return 0;
 }
 

---- DIFF 2 ----
diff --git a/sound/usb/mixer_audient.c b/sound/usb/mixer_audient.c
--- a/sound/usb/mixer_audient.c
+++ b/sound/usb/mixer_audient.c
@@ -1083,6 +1083,7 @@ static int snd_audient_add_ctrl(struct usb_mixer_interface *mixer,
                return -ENOMEM;
 
        elem->head.mixer = mixer;
+       elem->head.id = ctype->unit;
        elem->channels = ctrl->channels;
 
        if (ctype->flags & SND_AUDIENT_CTYPE_FLAGS_CACHE)






On Fri, Nov 06, 2026 at 02:29:43PM -0600, Will Porter wrote:
> Hi Christian,
> 
> The bot found a real problem in my patch 13:
> 
> > Does advertising ctype->enumeration->items + 1 items in
> > snd_ctl_audient_info() while rejecting item index e->items with
> > -EINVAL in snd_ctl_audient_enum_set() break the ALSA control UAPI
> > contract for writable enumerated controls?
> 
> Yes. mixer-test writes every advertised item, and alsactl restore
> fails on a value that store saved as the extra item.
> 
> The diff below drops the extra item from every enum:
> 
> - A device value outside the list makes get return -EIO. I chose an
>   error over a fallback item: an unknown routing value shown as Main
>   Mix could hide a route that bypasses the speaker volume, cut and
>   dim. The device is not known to report such a value.
> - A write-only value that no transfer has confirmed (only after a
>   failed probe write) reads as its init value, as a write-only switch
>   reads off.
> 
> Tested on the iD14 (stable 7.2.15) with the diff applied, with a
> script that does the enum writes of mixer-test on this card only:
> get returned every advertised item after its put (from the cache for
> the write-only controls), and the item past the end was rejected.
> alsactl store and restore of the card succeeded. I did not run the
> full mixer-test, because it writes the controls of every card.
> 
> Does -EIO from get look right to you here? If the diff looks good,
> I'll fold it into patches 13 and 15 in the next version.
> 
> Thanks,
> Will
> 
> -- >8 --
> From: Will Porter <mrwillporter@gmail.com>
> Subject: [RFC PATCH] ALSA: mixer_audient: Advertise only selectable enum items
> 
> Every enumerated control advertised one item more than a put accepts.
> mixer-test writes every advertised item, and alsactl restore fails on
> a stored value of that item. Advertise only the real items.
> 
> A get of a device value outside the list now returns -EIO: an unknown
> routing value shown as Main Mix could hide a route that bypasses the
> speaker volume, cut and dim. After a failed probe write, a write-only
> value reads as its init value; this does not confirm the device
> state.
> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Will Porter <mrwillporter@gmail.com>
> ---
> Proposed change to patches 13 and 15.
> 
>  sound/usb/mixer_audient.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/sound/usb/mixer_audient.c b/sound/usb/mixer_audient.c
> index e65c62e1eac4a..4180d7d8b325c 100644
> --- a/sound/usb/mixer_audient.c
> +++ b/sound/usb/mixer_audient.c
> @@ -67,10 +67,8 @@ static const DECLARE_TLV_DB_MINMAX_MUTE(snd_audient_tlv_mixer, -12800, 600);
>  #define SND_AUDIENT_CTYPE_FLAGS_CACHE		BIT(3)
>  
>  /*
> - * An enumerated control: item i sends values[i]. The texts have one item
> - * more than the values. A put cannot select that last item: a get shows it
> - * for a device value outside the list ("Other"), or for a write-only value
> - * that no write has confirmed ("Unknown").
> + * An enumerated control: item i sends values[i]. A get of a device value
> + * outside the list returns -EIO; it does not show a wrong item.
>   */
>  struct snd_audient_enum {
>  	const char *const *texts;
> @@ -85,7 +83,7 @@ struct snd_audient_enum {
>  		.values = name##_values,				\
>  		.items = ARRAY_SIZE(name##_values),			\
>  	};								\
> -	static_assert(ARRAY_SIZE(texts_) == ARRAY_SIZE(name##_values) + 1)
> +	static_assert(ARRAY_SIZE(texts_) == ARRAY_SIZE(name##_values))
>  
>  /*
>   * iD14 MkI units. The monitor controller, output routing, headphone level,
> @@ -126,7 +124,7 @@ static const DECLARE_TLV_DB_SCALE(snd_audient_tlv_id14_vol, -12800, 100, 0);
>  
>  /* Mono mode: one-speaker sum (Left, Right) or phantom centre */
>  static const char *const snd_audient_id14_mono_mode_texts[] = {
> -	"Left", "Centre", "Right", "Other",
> +	"Left", "Centre", "Right",
>  };
>  SND_AUDIENT_ENUM(snd_audient_id14_mono_mode,
>  		 snd_audient_id14_mono_mode_texts, 1, 0, 2);
> @@ -139,10 +137,10 @@ SND_AUDIENT_ENUM(snd_audient_id14_mono_mode,
>   * mute still act on every headphone source.
>   */
>  static const char *const snd_audient_id14_route_main_texts[] = {
> -	"Main Mix", "Cue Mix", "DAW Thru (full scale)", "Other",
> +	"Main Mix", "Cue Mix", "DAW Thru (full scale)",
>  };
>  static const char *const snd_audient_id14_route_hp_texts[] = {
> -	"Main Mix", "Cue Mix", "DAW Thru", "Other",
> +	"Main Mix", "Cue Mix", "DAW Thru",
>  };
>  SND_AUDIENT_ENUM(snd_audient_id14_route_main_l,
>  		 snd_audient_id14_route_main_texts, 0x14, 0x10, 0x00);
> @@ -156,12 +154,10 @@ SND_AUDIENT_ENUM(snd_audient_id14_route_hp_r,
>  /*
>   * iD key function: the value is the monitor controller selector the key
>   * toggles. ScrollControl (0xffff) toggles nothing: the knob then scrolls
> - * through the HID interface. The last item of a write-only enum shows a
> - * value that is not known.
> + * through the HID interface.
>   */
>  static const char *const snd_audient_id14_id_key_texts[] = {
>  	"ScrollControl", "Mono", "Mono + Polarity", "Dim", "Talkback",
> -	"Unknown",
>  };
>  SND_AUDIENT_ENUM(snd_audient_id14_id_key, snd_audient_id14_id_key_texts,
>  		 0xffff, ID14_MON_MONO, ID14_MON_POLARITY, ID14_MON_DIM,
> @@ -170,13 +166,13 @@ SND_AUDIENT_ENUM(snd_audient_id14_id_key, snd_audient_id14_id_key_texts,
>  /* Talkback source: the routing source number of the input; none = 0xff */
>  static const char *const snd_audient_id14_talkback_texts[] = {
>  	"None", "Mic 1", "Mic 2", "Digi 1", "Digi 2", "Digi 3", "Digi 4",
> -	"Digi 5", "Digi 6", "Digi 7", "Digi 8", "Unknown",
> +	"Digi 5", "Digi 6", "Digi 7", "Digi 8",
>  };
>  SND_AUDIENT_ENUM(snd_audient_id14_talkback, snd_audient_id14_talkback_texts,
>  		 0xff, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13);
>  
>  static const char *const snd_audient_id14_optical_texts[] = {
> -	"ADAT", "S/PDIF", "Unknown",
> +	"ADAT", "S/PDIF",
>  };
>  SND_AUDIENT_ENUM(snd_audient_id14_optical, snd_audient_id14_optical_texts,
>  		 0, 1);
> @@ -933,22 +929,23 @@ static int snd_ctl_audient_enum_get(struct snd_kcontrol *kctl,
>  	guard(mutex)(&mixer->lock);
>  
>  	for (i = 0; i < hwchan; i++) {
> +		unsigned int item;
>  		u32 val;
>  		int ret;
>  
>  		ret = snd_audient_raw_get_one(mixer, ctype,
>  					      snd_audient_cache_slot(elem, i),
>  					      base + i, &val);
> -		if (ret == -ENODATA) {	/* unknown write-only value */
> -			ctl_val->value.enumerated.item[i] =
> -				ctype->enumeration->items;
> -			continue;
> -		} else if (ret < 0) {
> +		if (ret == -ENODATA)	/* unknown write-only value: init */
> +			val = ctype->init;
> +		else if (ret < 0)
>  			return ret;
> -		}
>  
> -		ctl_val->value.enumerated.item[i] =
> -			snd_audient_enum_item(ctype->enumeration, val);
> +		item = snd_audient_enum_item(ctype->enumeration, val);
> +		if (item >= ctype->enumeration->items)
> +			return -EIO;
> +
> +		ctl_val->value.enumerated.item[i] = item;
>  	}
>  
>  	for (; i < nchan; i++)
> @@ -1007,7 +1004,7 @@ static int snd_ctl_audient_info(struct snd_kcontrol *kctl,
>  
>  	if (ctype->type == SNDRV_CTL_ELEM_TYPE_ENUMERATED)
>  		return snd_ctl_enum_info(uinfo, snd_audient_kctl_channels(kctl),
> -					 ctype->enumeration->items + 1,
> +					 ctype->enumeration->items,
>  					 ctype->enumeration->texts);
>  
>  	uinfo->type = ctype->type;

-- 
  Christian Ruppert                                             <arc@gmx.li>

This message is digitally signed with GnuPG.
GPG/PGP Key fingerprint = 4492 2196 75F0 EAA0 981E  B661 1476 F17A 975F 587B

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 870 bytes --]