Re: [PATCH v4] virtio-snd: add virtio sound device specification

[Gerd Hoffmann <kraxel@redhat.com>]

On Tue, Jan 07, 2020 at 01:50:49PM +0100, Anton Yakovlev wrote:
> This patch proposes virtio specification for a new virtio sound device,
> that may be useful in case when having audio is required but a device
> passthrough or emulation is not an option.

Looks pretty good overall.  Some small nits:

> +/* a common header */
> +struct virtio_snd_hdr {
> +    le32 code;
> +};
> +
> +/* an event notification */
> +struct virtio_snd_event {
> +    le32 type;

Use "struct virtio_snd_hdr hdr" instead of "type", for consistency with
the other structs?

> +\begin{description}
> +\item[\field{code}] (device-readable) specifies a device request type
> +(VIRTIO_SND_R_*).
> +\end{description}
> +
> +A response part has, or consists of, a common header containing the following
> +field:
> +
> +\begin{description}
> +\item[\field{code}] (device-writable) specifies a device request status
> +(VIRTIO_SND_S_*).
> +\end{description}

Repeating "device-readable" or "device-writable" for each struct field
looks a bit odd because this applies to the whole struct.  Not so much
for these structs with a single field only, but there are structs with
more fields further down the spec ...

> +\begin{description}
> +\item[\field{type}] (device-writable) specifies an event type (VIRTIO_SND_EVT_*).
> +\item[\field{data}] (device-writable) specifies an optional event data.
> +\end{description}

... here for example.

> +/* a response containing PCM stream capabilities */
> +struct virtio_snd_pcm_caps {
> +    struct virtio_snd_hdr hdr; /* VIRTIO_SND_S_XXX */
> +    u8 directions; /* 1 << VIRTIO_SND_PCM_T_XXX */
> +    u8 channels_min;
> +    u8 channels_max;
> +    u8 features; /* 1 << VIRTIO_SND_PCM_F_XXX */
> +    le64 formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */
> +    le32 rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */

Use le64 for rates too, just in case?

For features a few more bits don't hurt too, we already have 5 defined,
so having only 8 total available isn't that much for possible
extensions ...

> +\paragraph{VIRTIO_SND_R_PCM_PAUSE}
> +\paragraph{VIRTIO_SND_R_PCM_UNPAUSE}

Still here.

So there are good reasons to keep them instead of just using stop/start?

cheers,
  Gerd