Re: [virtio-dev] [PATCH 1/1] ALSA: virtio: add support for audio control

+enum {
+       VIRTIO_SND_CTL_ROLE_UNDEFINED = 0,
+       VIRTIO_SND_CTL_ROLE_VOLUME,
+       VIRTIO_SND_CTL_ROLE_MUTE,
+       VIRTIO_SND_CTL_ROLE_GAIN
+};

Regarding "role" values:

While a volume-joined control can be represented using a control with "count"=1 and "role"=VIRTIO_SND_CTL_ROLE_VOLUME, it would not be clear on the guest side whether this is a volume control for a one channel sink or joined volume control for a multi channel sink; similar comment for switch-joined (corresponding role would be VIRTIO_SND_CTL_ROLE_MUTE). Is the guest driver supposed to look for a channel map with the same hda_fn_nid value and infer from the relation between audio control "count" value and channel map "channels" value, whether this is joined volume/mute control or not? Could this be addressed in a simpler fashion by having separate "role" bit mask values for joined volume and mute control?
There is no representation for a cswitch-exclusive control, so that the cswitch-exclusive capability can be recognized on the guest side
What is the difference between VIRTIO_SND_CTL_ROLE_VOLUME and VIRTIO_SND_CTL_ROLE_GAIN?
The posted guest driver implementation doesn't make any use of the "role" information provided by the host device as part of struct virtio_snd_ctl_info. Is there a plan to add that in a later revision of the guest driver?

+/* supported value types for control elements */
+enum {
+       VIRTIO_SND_CTL_TYPE_BOOLEAN = 0,
+       VIRTIO_SND_CTL_TYPE_INTEGER,
+       VIRTIO_SND_CTL_TYPE_INTEGER64,
+       VIRTIO_SND_CTL_TYPE_ENUMERATED,
+       VIRTIO_SND_CTL_TYPE_BYTES,
+       VIRTIO_SND_CTL_TYPE_IEC958
+};

Regarding relationship between audio control "role" and "type":

If certain data types are assumed for controls with role VIRTIO_SND_CTL_ROLE_VOLUME, VIRTIO_SND_CTL_ROLE_MUTE or VIRTIO_SND_CTL_ROLE_GAIN, it should be specified (e.g. BOOLEAN for VIRTIO_SND_CTL_ROLE_MUTE)

+/* supported access rights for control elements */
+enum {
+       VIRTIO_SND_CTL_ACCESS_READ = 0,
+       VIRTIO_SND_CTL_ACCESS_WRITE,
+       VIRTIO_SND_CTL_ACCESS_VOLATILE,
+       VIRTIO_SND_CTL_ACCESS_INACTIVE,
+       VIRTIO_SND_CTL_ACCESS_TLV_READ,
+       VIRTIO_SND_CTL_ACCESS_TLV_WRITE,
+       VIRTIO_SND_CTL_ACCESS_TLV_COMMAND
+};

Regarding TLVs:

The TLV type values are considered well known from ALSA; there are only a few common TLV type definitions (like db scale, db min/max, a few more); they could be defined in virtio-snd.h for the completeness of the specification. Otherwise a non ALSA based host that supports reporting db scale and db min/max would need to redefine these to match the ALSA definitions
The original extension proposal used to include the definition of struct virtio_snd_ctl_tlv. It is removed in the guest driver implementation and the guest driver seems to memcpy to/from the equivalent ALSA struct. For the completeness of the specification all data structures used should be described as part of the specification. This is particularly important when the host or guest don't use ALSA.

+struct virtio_snd_ctl_info {
+       /* common header */
+       struct virtio_snd_info hdr;
+       /* element role (VIRTIO_SND_CTL_ROLE_XXX) */
+       __le32 role;
+       /* element value type (VIRTIO_SND_CTL_TYPE_XXX) */
+       __le32 type;
+       /* element access right bit map (1 << VIRTIO_SND_CTL_ACCESS_XXX) */
+       __le32 access;
+       /* # of members in the element value */
+       __le32 count;
+       /* index for an element with a non-unique name */
+       __le32 index;
+       /* name identifier string for the element */
+       __u8 name[44];
+       /* additional information about the element's value */
+       union {
+               /* VIRTIO_SND_CTL_TYPE_INTEGER */
+               struct {
+                       /* minimum supported value */
+                       __le32 min;
+                       /* maximum supported value */
+                       __le32 max;
+                       /* fixed step size for value (0 = variable size) */
+                       __le32 step;
+               } integer;
+               /* VIRTIO_SND_CTL_TYPE_INTEGER64 */
+               struct {
+                       /* minimum supported value */
+                       __le64 min;
+                       /* maximum supported value */
+                       __le64 max;
+                       /* fixed step size for value (0 = variable size) */
+                       __le64 step;
+               } integer64;
+               /* VIRTIO_SND_CTL_TYPE_ENUMERATED */
+               struct {
+                       /* # of options supported for value */
+                       __le32 items;
+               } enumerated;
+       } value;
+};

The field "name" of struct virtio_snd_ctl_info should have length 48 (or another multiple of 8), to ensure 64 bit alignment for the integer64 union member below, regardless of alignment and packing attributes.

virtio-dev message