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:
- For some reason, in the previous patch series, several patches were squashed. Fixed this issue to make the review easier.
- Added mst@redhat.com to the MAINTAINERS.
- When creating virtqueues, now only the event virtqueue is disabled. It's enabled only after successful initialization of the device.
- Added additional comments to the reset worker function: [2/9] virtio_card.c:virtsnd_reset_fn()
- Added check that VIRTIO_F_VERSION_1 feature bit is set.
- Added additional comments to the device removing function: [2/9] virtio_card.c:virtsnd_remove()
- Added additional comments to the tx/rx interrupt handler: [5/9] virtio_pcm_msg.c:virtsnd_pcm_msg_complete()
- 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