[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v4] virtio-snd: add virtio sound device specification
On 08.01.2020 11:34, Gerd Hoffmann wrote:
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?
Yes, good idea.
+\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 ...
Well, I'm not sure here. Previously, I was told to mark explicitly every field.
+\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 ...
Okey, then I will rearrange the structure.
+\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?
We can get rid of pause/unpause at the moment. Later, they can be added as a stream feature, if necessary.
Also, regarding requirement for populating tx/rx queues with period size buffers. Maybe it's better to replace MUST with SHOULD? It would give more flexibility for the driver. What do you think?
-- Anton Yakovlev Senior Software Engineer OpenSynergy GmbH Rotherstr. 20, 10245 Berlin
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]