[alsa-devel] [RFC 4/5] compress: add the core file

[Mark Brown]

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.

> + * - 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.

> +	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.

> +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?

> +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?

> +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.

> +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)?