+/* supported roles for control elements */
+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:

  1.  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?
  2.  There is no representation for a cswitch-exclusive control, so that the 
cswitch-exclusive capability can be recognized on the guest side
  3.  What is the difference between VIRTIO_SND_CTL_ROLE_VOLUME and 
VIRTIO_SND_CTL_ROLE_GAIN?
  4.  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:

  1.  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
  2.  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.

----------------------------------------------------------------------
This transmission (including any attachments) may contain confidential 
information, privileged material (including material protected by the 
solicitor-client or other applicable privileges), or constitute non-public 
information. Any use of this information by anyone other than the intended 
recipient is prohibited. If you have received this transmission in error, 
please immediately reply to the sender and delete this information from your 
system. Use, dissemination, distribution, or reproduction of this transmission 
by unintended recipients is not authorized and may be unlawful.

Reply via email to