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

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

[Will Porter]

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;

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

[Will Porter]

The iD14 has selectors with a fixed set of device values: output
routing, mono mode, the iD key function, talkback source and the
digital input mode. Add an enumerated control type with a value list.

The item list has one item more than the values. A put cannot select
it. A get shows it for a device value outside the list, so a value set
by other means stays visible as such, not as a wrong item, and for a
write-only value that no write has confirmed.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Will Porter <mrwillporter@gmail.com>
---
 sound/usb/mixer_audient.c | 130 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/sound/usb/mixer_audient.c b/sound/usb/mixer_audient.c
index c4ce2340f8bb..3e6cd8981b4f 100644
--- a/sound/usb/mixer_audient.c
+++ b/sound/usb/mixer_audient.c
@@ -42,6 +42,27 @@ static const DECLARE_TLV_DB_MINMAX_MUTE(snd_audient_tlv_mixer, -12800, 600);
 #define SND_AUDIENT_CTYPE_FLAGS_FORCE_SET	BIT(2)
 #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").
+ */
+struct snd_audient_enum {
+	const char *const *texts;
+	const u32 *values;
+	unsigned int items;
+};
+
+#define SND_AUDIENT_ENUM(name, texts_, ...)				\
+	static const u32 name##_values[] = { __VA_ARGS__ };		\
+	static const struct snd_audient_enum name = {			\
+		.texts = texts_,					\
+		.values = name##_values,				\
+		.items = ARRAY_SIZE(name##_values),			\
+	};								\
+	static_assert(ARRAY_SIZE(texts_) == ARRAY_SIZE(name##_values) + 1)
+
 static const struct snd_audient_ctrl_type {
 	snd_ctl_elem_type_t type;
 	unsigned long flags;
@@ -54,6 +75,7 @@ static const struct snd_audient_ctrl_type {
 	} max;		/* for booleans holds physical int value for true */
 	u32 init;	/* hardware and cache are initialized to this value */
 	unsigned int res;	/* integers: device units per control step, 0 = 1 */
+	const struct snd_audient_enum *enumeration;
 	const unsigned int *tlv;
 	u8 unit;
 	u8 sel;
@@ -560,12 +582,116 @@ static int snd_ctl_audient_integer_set(struct snd_kcontrol *kctl,
 	return changed;
 }
 
+static unsigned int snd_audient_enum_item(const struct snd_audient_enum *e,
+					  u32 value)
+{
+	unsigned int item;
+
+	for (item = 0; item < e->items; item++)
+		if (e->values[item] == value)
+			break;
+
+	return item;	/* e->items: not in the list */
+}
+
+static int snd_ctl_audient_enum_get(struct snd_kcontrol *kctl,
+				    struct snd_ctl_elem_value *ctl_val)
+{
+	struct usb_mixer_elem_info *const elem = snd_kcontrol_chip(kctl);
+	struct usb_mixer_interface *const mixer = elem->head.mixer;
+
+	const unsigned int nchan = snd_audient_kctl_channels(kctl);
+	const unsigned int hwchan = snd_audient_kctl_hwchannels(kctl);
+	const struct snd_audient_ctrl_type *const ctype
+						= snd_audient_kctl_type(kctl);
+	const u8 base = snd_audient_kctl_cidxoffs(kctl);
+
+	unsigned int i;
+
+	CLASS(snd_usb_lock, pm)(mixer->chip);
+	if (pm.err < 0)
+		return pm.err;
+	/* serializes the transfers and the cache, also between gets */
+	guard(mutex)(&mixer->lock);
+
+	for (i = 0; i < hwchan; i++) {
+		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) {
+			return ret;
+		}
+
+		ctl_val->value.enumerated.item[i] =
+			snd_audient_enum_item(ctype->enumeration, val);
+	}
+
+	for (; i < nchan; i++)
+		ctl_val->value.enumerated.item[i] = 0;
+
+	return 0;
+}
+
+static int snd_ctl_audient_enum_set(struct snd_kcontrol *kctl,
+				    struct snd_ctl_elem_value *ctl_val)
+{
+	struct usb_mixer_elem_info *const elem = snd_kcontrol_chip(kctl);
+	struct usb_mixer_interface *const mixer = elem->head.mixer;
+
+	const unsigned int hwchan = snd_audient_kctl_hwchannels(kctl);
+	const struct snd_audient_ctrl_type *const ctype
+						= snd_audient_kctl_type(kctl);
+	const struct snd_audient_enum *const e = ctype->enumeration;
+	const u8 base = snd_audient_kctl_cidxoffs(kctl);
+
+	unsigned int i;
+	int changed = 0;
+
+	for (i = 0; i < hwchan; i++)
+		if (ctl_val->value.enumerated.item[i] >= e->items)
+			return -EINVAL;
+
+	CLASS(snd_usb_lock, pm)(mixer->chip);
+	if (pm.err < 0)
+		return pm.err;
+	/* serializes the transfers and the cache, also between gets */
+	guard(mutex)(&mixer->lock);
+
+	for (i = 0; i < hwchan; i++) {
+		const unsigned int item = ctl_val->value.enumerated.item[i];
+		int ret;
+
+		ret = snd_audient_raw_set_one(mixer, ctype,
+					      snd_audient_cache_slot(elem, i),
+					      base + i, e->values[item], false);
+		if (ret < 0)
+			return ret;
+
+		if (ret)
+			changed = 1;
+	}
+
+	return changed;
+}
+
 static int snd_ctl_audient_info(struct snd_kcontrol *kctl,
 			    struct snd_ctl_elem_info *uinfo)
 {
 	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 + 1,
+					 ctype->enumeration->texts);
+
 	uinfo->type = ctype->type;
 	uinfo->count = snd_audient_kctl_channels(kctl);
 
@@ -613,6 +739,10 @@ static int snd_audient_add_ctrl(struct usb_mixer_interface *mixer,
 		knew.get = snd_ctl_audient_boolean_get;
 		knew.put = snd_ctl_audient_boolean_set;
 		break;
+	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
+		knew.get = snd_ctl_audient_enum_get;
+		knew.put = snd_ctl_audient_enum_set;
+		break;
 	default:
 		return -EINVAL;
 	}
-- 
2.47.3