[alsa-devel] [PATCH 5/6] compress: add the core file

[Nallasellan, Singaravelan]

> +static int snd_compr_open(struct file *f,  struct snd_compr *compr, int
> +type) {
> +	struct snd_compr_file *data;
> +	struct snd_compr_runtime *runtime;
> +	int ret;
> +
> +	mutex_lock(&device_mutex);
> +
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	data->stream.ops = compr->ops;
NULL check for compr would help here.

> +	data->stream.direction = type;
> +	data->stream.private_data = compr->private_data;
> +	data->stream.device = compr;
> +	runtime = kzalloc(sizeof(*runtime), GFP_KERNEL);
> +	if (!runtime) {
> +		ret = -ENOMEM;
> +		kfree(data);
> +		goto out;
> +	}
> +	runtime->state = SNDRV_PCM_STATE_OPEN;
> +	init_waitqueue_head(&runtime->sleep);
> +	data->stream.runtime = runtime;
> +	f->private_data = (void *)data;
> +	ret = compr->ops->open(&data->stream);
NULL check for ops would help here.

> +	if (ret) {
> +		kfree(runtime);
> +		kfree(data);
> +		goto out;
> +	}
> +out:
> +	mutex_unlock(&device_mutex);
> +	return ret;
> +}
> +

> +static int snd_compr_free(struct inode *inode, struct file *f) {
> +	struct snd_compr_file *data = f->private_data;
> +	mutex_lock(&device_mutex);
Can we have mutex for each file opened instead of having a single mutex for everything?
I think it is not required to sequence all the streams.

> +	data->stream.ops->free(&data->stream);
> +	kfree(data->stream.runtime->buffer);
> +	kfree(data->stream.runtime);
> +	kfree(data);
> +	mutex_unlock(&device_mutex);
> +	return 0;
> +}
> +
> +static void snd_compr_update_tstamp(struct snd_compr_stream *stream,
> +		struct snd_compr_tstamp *tstamp)
> +{
> +	stream->ops->pointer(stream, tstamp);
How to ensure that the caller always checks for NULL pointer? 

> +	pr_debug("dsp consumed till %d total %d bytes\n",
> +		tstamp->copied_bytes, tstamp->copied_total);
> +	stream->runtime->hw_pointer = tstamp->copied_bytes;
> +	stream->runtime->bytes_copied = tstamp->copied_total; }
> +
> +static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
> +		struct snd_compr_avail *avail)
> +{
> +	long avail_calc; /*this needs to be signed variable */
> +
> +	snd_compr_update_tstamp(stream, &avail->tstamp);
> +
> +	if (stream->runtime->bytes_written == 0 &&
> +			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> +		pr_debug("detected init and someone forgot to do a write\n");
> +		return stream->runtime->buffer_size;
> +	}
> +	pr_debug("app wrote %d, DSP consumed %d\n",
> +		stream->runtime->bytes_written, stream->runtime->bytes_copied);
> +	if (stream->runtime->bytes_written == stream->runtime->bytes_copied) {
> +		pr_debug("both pointers are same, returning full avail\n");
> +		return stream->runtime->buffer_size;
> +	}
> +
> +	avail_calc = stream->runtime->buffer_size -
> +		(stream->runtime->app_pointer - stream->runtime->hw_pointer);
> +	pr_debug("calc avail as %ld, app_ptr %d, hw+ptr %d\n", avail_calc,
> +		stream->runtime->app_pointer, stream->runtime->hw_pointer);
> +	if (avail_calc >= stream->runtime->buffer_size)
> +		avail_calc -= stream->runtime->buffer_size;
> +	pr_debug("ret avail as %ld\n", avail_calc);
> +	avail->avail = avail_calc;
> +	return avail_calc;
> +}
> +
> +static size_t snd_compr_get_avail(struct snd_compr_stream *stream) {
> +	struct snd_compr_avail avail;
> +
Can we update the timestamp before calculating the avail? This will allow partial   
Fragment to be written as well.
> +	return snd_compr_calc_avail(stream, &avail); }
> +
> +static int
> +snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long
> +arg) {
> +	struct snd_compr_avail ioctl_avail;
> +
Can we update the timestamp before calculating the avail? This will allow partial   
Fragment to be written as well.

> +	snd_compr_calc_avail(stream, &ioctl_avail);
> +
> +	if (copy_to_user((unsigned long __user *)arg,
> +				&ioctl_avail, sizeof(ioctl_avail)))
> +		return -EFAULT;
> +	return 0;
> +}
> +


> +static ssize_t snd_compr_write(struct file *f, const char __user *buf,
> +		size_t count, loff_t *offset)
> +{
> +
> +	/* while initiating the stream, write should be called before START
> +	 * call, so in setup move state */
> +	if (stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
> +		stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
Can we implement kind of start threshold similar to PCM path here to initiate the playback?

> +		pr_debug("stream prepared, Houston we are good to go\n");
> +	}
> +
> +	mutex_unlock(&stream->device->lock);
> +	return retval;
> +}


> +static int snd_compr_resume(struct snd_compr_stream *stream) {
> +	int retval;
> +
> +	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
If the stream is running, I guess it can return 0.

> +		return -EPERM;
> +	retval = stream->ops->trigger(stream,
> SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> +	if (!retval)
> +		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> +	return retval;
> +}
> +
> +static int snd_compr_start(struct snd_compr_stream *stream) {
> +	int retval;
> +
> +	if (stream->runtime->state != SNDRV_PCM_STATE_PREPARED)
> +		return -EPERM;
> +	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_START);
> +	stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
Set to running state if start is successful.

> +	return retval;
> +}
> +
> +static int snd_compr_stop(struct snd_compr_stream *stream) {
> +	int retval;
> +
> +	if (stream->runtime->state != SNDRV_PCM_STATE_PREPARED)
> +		return -EPERM;
> +	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP);
> +	if (!retval) {
> +		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> +		wake_up(&stream->runtime->sleep);
> +	}
> +	return retval;
> +}
> +
> +static int snd_compr_drain(struct snd_compr_stream *stream) {
> +	int retval;
> +
> +	if (stream->runtime->state != SNDRV_PCM_STATE_PREPARED ||
> +			stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
> +		return -EPERM;
What happens if the stream is in running state? This will return -EPERM. It should send DRAIN in this case.

> +	retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_DRAIN);
> +	if (!retval) {
> +		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> +		wake_up(&stream->runtime->sleep);
> +	}
> +	return retval;
> +}
> +
> +static long snd_compr_ioctl(struct file *f, unsigned int cmd, unsigned
> +long arg) {
> +	struct snd_compr_file *data = f->private_data;
> +	struct snd_compr_stream *stream;
> +	int retval = -ENOTTY;
> +
> +	BUG_ON(!data);
> +	stream = &data->stream;
> +	BUG_ON(!stream);
> +	mutex_lock(&stream->device->lock);
Simple structure with ioctl functions will reduce lots of code.
It is not anyway doing any specific processing for each ioctl.
> +	switch (_IOC_NR(cmd)) {
> +	case _IOC_NR(SNDRV_COMPRESS_IOCTL_VERSION):
> +		put_user(SNDRV_COMPRESS_VERSION,
> +				(int __user *)arg) ? -EFAULT : 0;
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_GET_CAPS):
> +		retval = snd_compr_get_caps(stream, arg);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_GET_CODEC_CAPS):
> +		retval = snd_compr_get_codec_caps(stream, arg);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_SET_PARAMS):
> +		retval = snd_compr_set_params(stream, arg);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_GET_PARAMS):
> +		retval = snd_compr_get_params(stream, arg);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_TSTAMP):
> +		retval = snd_compr_tstamp(stream, arg);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_AVAIL):
> +		retval = snd_compr_ioctl_avail(stream, arg);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_PAUSE):
> +		retval = snd_compr_pause(stream);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_RESUME):
> +		retval = snd_compr_resume(stream);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_START):
> +		retval = snd_compr_start(stream);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_STOP):
> +		retval = snd_compr_stop(stream);
> +		break;
> +	case _IOC_NR(SNDRV_COMPRESS_DRAIN):
> +		cmd = SND_COMPR_TRIGGER_DRAIN;
> +		retval = snd_compr_drain(stream);
> +		break;
> +	}
> +	mutex_unlock(&stream->device->lock);
> +	return retval;
> +}
> +
> +static const struct file_operations snd_compr_file_ops[2] = {
> +	{
> +		.owner =	THIS_MODULE,
> +		.open =		snd_compr_pb_open,
> +		.release =	snd_compr_free,
> +		.write =	snd_compr_write,
> +		.unlocked_ioctl = snd_compr_ioctl,
> +		.mmap =		snd_compr_mmap,
Suggest you to populate it when implemented.
> +		.poll =		snd_compr_pb_poll,
> +	},
> +	{
> +		.owner =	THIS_MODULE,
> +		.open =		snd_compr_cap_open,
> +		.release =	snd_compr_free,
> +		.read =		snd_compr_read,
> +		.unlocked_ioctl = snd_compr_ioctl,
> +		.mmap =		snd_compr_mmap,
> +		.poll =		snd_compr_cap_poll,
> +	}
> +};
> +
> +static int snd_compress_dev_register(struct snd_device *device) {
> +	int ret;
> +	char str[16];
> +	struct snd_compr *compr;
> +	int devtype, cidx;
> +
> +	if (snd_BUG_ON(!device || !device->device_data))
> +		return -ENXIO;
> +	compr = device->device_data;
> +
> +	for (cidx = 0; cidx < 2; cidx++) {
> +		switch (cidx) {
> +		case 0:
> +			if (!compr->pb)
Can we have the device type or direction in this variable. Suggest you not to use
two variables (cap and pb) here.
> +				continue;
> +			sprintf(str, "comprC%iD%ip", compr->card->number,
> +				compr->device);
> +			pr_debug("reg %s for device %s\n", str, compr->name);
> +			devtype = SNDRV_DEVICE_TYPE_COMPR_PLAYBACK;
> +			break;
> +