[PATCH v2 0/9] ALSA: add virtio sound driver

[Takashi Iwai]

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 at redhat.com 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