On Fri, 2011-09-02 at 15:36 +0100, Mark Brown wrote:
On Fri, Sep 02, 2011 at 11:36:24AM +0530, Vinod Koul wrote:
- Integrate with ASoC:
- Opening compressed path should also start the codec dai
- TBD how the cpu dai will be viewed and started.
- ASoC should always be optional part
- (we should be able to use this framework in non asoc systems
Just write an ALSA core API, ASoC is an ALSA driver so it can just use the same interfaces as everything else gets.
Actually no. That way devices would get regsitered as /dev/snd_pcmCxDxy which wont be corect. Was thinking of making these appear as something like /dev/snd/comprCxDxy. ANy pointer for above change would be welcome
- Multiple node representation
- driver should be able to register multiple nodes
- Version numbering for API
I'd suggest doing this per ioctl rather than per API.
OK
- ret = misc->compr->ops->open(&data->stream);
- if (ret) {
kfree(runtime);kfree(data);goto out;- }
- runtime->state = SNDRV_PCM_STATE_OPEN;
- init_waitqueue_head(&runtime->sleep);
- data->stream.runtime = runtime;
- f->private_data = (void *)data;
Should we hoist these before we call open(), especially the init of runtime? It seems likely to be more robusy.
Since open is just supposed to let the device know that stream is being created with a particular direction. Event if I move this up, doesn't serve a purpose as driver should touch these a t open. But I agree this helps to make more robust
+static int snd_compr_write(struct file *f, const char __user *buf,
size_t count, loff_t *offset)+{
- struct snd_ioctl_data *data = f->private_data;
- struct snd_compr_stream *stream;
- size_t avail;
- int retval;
- BUG_ON(!data);
- stream = &data->stream;
- /* write is allowed when stream is running or has been steup */
- if (stream->runtime->state != SNDRV_PCM_STATE_SETUP &&
stream->runtime->state != SNDRV_PCM_STATE_RUNNING)return -EPERM;- mutex_lock(&stream->device->lock);
Shouldn't we lock something before we check the stream state?
Yesss...
+static int snd_compr_read(struct file *f, char __user *buf,
size_t count, loff_t *offset)+{
- return -ENXIO;
+}
+static int snd_compr_mmap(struct file *f, struct vm_area_struct *vma) +{
- return -ENXIO;
+}
Do we need to implement noops like these?
Well I kept them placeholder to remind me what else needs implementation, I think I will keep them for a while as we need to implement mmap and recording support as well. Recording is actually trivial after playback.
+unsigned int snd_compr_poll(struct file *f, poll_table *wait) +{
- struct snd_ioctl_data *data = f->private_data;
- struct snd_compr_stream *stream;
- BUG_ON(!data);
- stream = &data->stream;
- if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
return -ENXIO;- poll_wait(f, &stream->runtime->sleep, wait);
- /* this would change after read is implemented, we would need to
* check for direction here */- if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
return POLLOUT | POLLWRNORM;- return 0;
+}
+void snd_compr_period_elapsed(struct snd_compr_stream *stream) +{
- size_t avail;
- avail = snd_compr_get_avail(stream);
- if (avail >= stream->runtime->fragment_size)
wake_up(&stream->runtime->sleep);+} +EXPORT_SYMBOL_GPL(snd_compr_period_elapsed);
I can see why you picked period_elapsed() (for consistency) but it feels wrong to have a time based name for something which is going to end up being data size based.
Yes the notion of period here cannot be time it is in bytes.
+static int snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg) +{
- struct snd_compr_params *params;
- int retval;
- if (stream->runtime->state == SNDRV_PCM_STATE_OPEN) {
It would be more legible to reverse the check I think? Also shouldn't there be more locking in this (and all the other stuff looking at states)?
Ok, Thanks for review