On Sun, 24 Jan 2021 17:53:59 +0100,
Anton Yakovlev wrote:
>
> This series implements a driver part of the virtio sound device
> specification v8 [1].
>
> The driver supports PCM playback and capture substreams, jack and
> channel map controls. A message-based transport is used to write/read
> PCM frames to/from a device.
>
> The series is based (and was actually tested) on Linus's master
> branch [2], on top of
>
> commit 1e2a199f6ccd ("Merge tag 'spi-fix-v5.11-rc4' of ...")
>
> As a device part was used OpenSynergy proprietary implementation.
>
> Any comments are very welcome.
>
> v1->v2 changes:
>
> 1. For some reason, in the previous patch series, several patches were
> squashed. Fixed this issue to make the review easier.
> 2. Added mst@xxxxxxxxxx to the MAINTAINERS.
> 3. When creating virtqueues, now only the event virtqueue is disabled.
> It's enabled only after successful initialization of the device.
> 4. Added additional comments to the reset worker function:
> [2/9] virtio_card.c:virtsnd_reset_fn()
> 5. Added check that VIRTIO_F_VERSION_1 feature bit is set.
> 6. Added additional comments to the device removing function:
> [2/9] virtio_card.c:virtsnd_remove()
> 7. Added additional comments to the tx/rx interrupt handler:
> [5/9] virtio_pcm_msg.c:virtsnd_pcm_msg_complete()
> 8. Added additional comments to substream release wait function.
> [6/9] virtio_pcm_ops.c:virtsnd_pcm_released()
>
> [1] https://lists.oasis-open.org/archives/virtio-dev/202003/msg00185.html
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>
>
> Anton Yakovlev (9):
> uapi: virtio_ids: add a sound device type ID from OASIS spec
> ALSA: virtio: add virtio sound driver
> ALSA: virtio: handling control messages
> ALSA: virtio: build PCM devices and substream hardware descriptors
> ALSA: virtio: handling control and I/O messages for the PCM device
> ALSA: virtio: PCM substream operators
> ALSA: virtio: introduce jack support
> ALSA: virtio: introduce PCM channel map support
> ALSA: virtio: introduce device suspend/resume support
Through a quick glance over the new series, below are some comments
(sorry not putting in each patch):
- The PCM buffer management can be simplified with the recently
introduced API, snd_pcm_set_managed_buffer() and *_all().
BTW, I wondered why you need to iterate do preallocation for each;
can't the *_all() function work in a shot?
- The PCM ops has sync_stop that is performed before prepare, hw_parms
and else after a stream is stopped via trigger(STOP). This can be
used for synchronizing things as found in your driver code.
- I'm wondering whether you can use the non-atomic PCM ops. Is any
interrupt handling (or spinlocked context) involved in the
interface? If not, you can set nonatomic flag, and this would
simply things.
- Does the buffer type have to be CONTINUOUS? Can't the vmalloc
buffer work?
- Is the buffer supposed to be aligned with the period size? i.e.
is the configuration like period=200 buffer=500 supposed to work?
If the period-size alignment is needed, the driver requires an
additional hw_constraint setup to make PERIODS integer.
- In general, "stream", and "subtream" variables are used in mixed
ways for both ALSA and virtio objects, and it's hard to follow.
Maybe some prefix would help.
- Don't PCM stream names need to be unique? They are all the same
string.
- Leave the card->id without changing unless you need to set it up
explicitly by some extra reason. Also, card->shortname should be a
bit more verbose and descriptive. It's a free-string while
card->drier is an ID string that is used as a lookup key in
alsa-lib.
thanks,
Takashi
