Federico Simoncelli wrote:
+++ b/drivers/media/usb/usbtv/usbtv-audio.c ... +#include <sound/ac97_codec.h>
What do you need this header for?
+static struct snd_pcm_hardware snd_usbtv_digital_hw = {
- ...
- .period_bytes_min = 11059,
- .period_bytes_max = 13516,
- .periods_max = 98,
- .buffer_bytes_max = 62720 * 8, /* value in usbaudio.c */
Where do these values come from? (There is no "usbaudio.c" file.)
+static int snd_usbtv_pcm_close(struct snd_pcm_substream *substream) +{
- if (atomic_read(&chip->snd_stream)) {
atomic_set(&chip->snd_stream, 0);
Doing _two_ atomic operations is racy.
You probably want a function like test_and_clear_bit().
schedule_work(&chip->snd_trigger);
The device must be closed when the .close callback returns.
+static void usbtv_audio_urb_received(struct urb *urb) +{
- struct usbtv *chip = urb->context;
- struct snd_pcm_substream *substream = chip->snd_substream;
- struct snd_pcm_runtime *runtime = substream->runtime;
- size_t i, frame_bytes, chunk_length, buffer_pos, period_pos;
- int period_elapsed;
- void *urb_current;
- if (!atomic_read(&chip->snd_stream))
return;
And what if the device is closed in the middle of this function?
- snd_pcm_stream_lock(substream);
- chip->snd_buffer_pos = buffer_pos;
- chip->snd_period_pos = period_pos;
- snd_pcm_stream_unlock(substream);
What is the purpose of this lock (besides introducing the chance of a deadlock)?
+static int usbtv_audio_start(struct usbtv *chip) +{ ...
- chip->snd_bulk_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (chip->snd_bulk_urb == NULL)
goto err_alloc_urb;- pipe = usb_rcvbulkpipe(chip->udev, USBTV_AUDIO_ENDP);
- chip->snd_bulk_urb->transfer_buffer = kzalloc(
Mapping this buffer repeatedly is inefficient. Better use usb_alloc_coherent().
USBTV_AUDIO_URBSIZE, GFP_KERNEL);- if (chip->snd_bulk_urb->transfer_buffer == NULL)
goto err_transfer_buffer;- usb_fill_bulk_urb(chip->snd_bulk_urb, chip->udev, pipe,
chip->snd_bulk_urb->transfer_buffer, USBTV_AUDIO_URBSIZE,usbtv_audio_urb_received, chip);- /* starting the stream */
- usbtv_set_regs(chip, setup, ARRAY_SIZE(setup));
- usb_clear_halt(chip->udev, pipe);
Allocating resources should be done in the .hw_params and/or .prepare callbacks. The .trigger callback should do as little as possible.
- usb_submit_urb(chip->snd_bulk_urb, GFP_ATOMIC);
For this single call, you don't need a workqueue.
+static int usbtv_audio_stop(struct usbtv *chip) +{ ...
- if (chip->snd_bulk_urb) {
usb_kill_urb(chip->snd_bulk_urb);kfree(chip->snd_bulk_urb->transfer_buffer);usb_free_urb(chip->snd_bulk_urb);chip->snd_bulk_urb = NULL;- }
- usbtv_set_regs(chip, setup, ARRAY_SIZE(setup));
Freeing resources should be done in the .hw_free and .close callbacks.
+void usbtv_audio_suspend(struct usbtv *usbtv) +{
- if (atomic_read(&usbtv->snd_stream) && usbtv->snd_bulk_urb)
Both tests are racy.
+static int snd_usbtv_card_trigger(struct snd_pcm_substream *substream, int cmd) +...
- case SNDRV_PCM_TRIGGER_RESUME:
This driver does not actually support resuming a PCM device from the position where it stopped playing.
- case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
This driver does not actually support pausing a PCM device. You must at least set the correct SNDRC_PCM_INFO_ flags.
+int usbtv_audio_init(struct usbtv *usbtv)
- ...
- snd_pcm_lib_preallocate_pages_for_all(pcm, SNDRV_DMA_TYPE_CONTINUOUS,
snd_dma_continuous_data(GFP_KERNEL), USBTV_AUDIO_BUFFER,USBTV_AUDIO_BUFFER);
You are not doing DMA to the ALSA buffer, so there is no need for it to be physically contiguous. Better use a vmalloc()-ed buffer.
+++ b/drivers/media/usb/usbtv/usbtv.h +#define USBTV_AUDIO_URBSIZE 20480 +#define USBTV_AUDIO_BUFFER 65536
Where do these values come from?
@@ -91,9 +96,23 @@ struct usbtv {
- size_t snd_buffer_pos;
- size_t snd_period_pos;
Why size_t?
Regards, Clemens