At Tue, 22 Nov 2011 14:21:59 +0530, Vinod Koul wrote:
This patch adds core.c, the file which implements the ioctls and registers the devices
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
A general note: it's not necessarily to split playback and capture devices. For example, rawmidi has a single device for full-duplex. In the case of PCM, we had to split them because of mmap. The mmap for the capture was supposed not to be read-only, some apps may want to add mark on the buffer. But, if this isn't the case for compression devices, you can have a single device serving for both playback and capture. The direction can be checked by the open flag.
+#define FORMAT(fmt) "%s: %d: " fmt, __func__, __LINE__ +#define pr_fmt(fmt) KBUILD_MODNAME ": " FORMAT(fmt)
Is this referred anywhere?
+#include <sound/snd_compress_params.h> +#include <sound/compress_offload.h> +#include <sound/compress_driver.h> +#include <sound/core.h> +#include <sound/initval.h>
In general, sound/core.h is included at first.
+static DEFINE_MUTEX(device_mutex);
+struct snd_compr_file {
- unsigned long caps;
- struct snd_compr_stream stream;
+};
+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);
Do you need a global lock for opening a device, too? Just to be sure, since I see no obvious need. (Anyway you don't need to have a lock at this early place before a bunch of kmallocs. The lock should be done as small as possible.)
+static int snd_compr_free(struct inode *inode, struct file *f) +{
- struct snd_compr_file *data = f->private_data;
- mutex_lock(&device_mutex);
- data->stream.ops->free(&data->stream);
- kfree(data->stream.runtime->buffer);
- kfree(data->stream.runtime);
- kfree(data);
- mutex_unlock(&device_mutex);
Ditto. Do you need a global lock?
+static int +snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg) +{
- struct snd_compr_avail ioctl_avail;
- snd_compr_calc_avail(stream, &ioctl_avail);
- if (copy_to_user((unsigned long __user *)arg,
&ioctl_avail, sizeof(ioctl_avail)))
snd_compr_calc_avail() doesn't initialize the pointer, so this may leak some kernel space.
+unsigned int snd_compr_pb_poll(struct file *f, poll_table *wait)
Missing static?
+unsigned int snd_compr_cap_poll(struct file *f, poll_table *wait)
Ditto.
+void snd_compr_fragment_elapsed(struct snd_compr_stream *stream) +{
- pr_debug("fragment elapsed called\n");
- if (stream->direction != SNDRV_DEVICE_TYPE_COMPR_PLAYBACK)
return;- wake_up(&stream->runtime->sleep);
+} +EXPORT_SYMBOL_GPL(snd_compr_fragment_elapsed);
+void snd_compr_frame_elapsed(struct snd_compr_stream *stream) +{
- if (stream->direction != SNDRV_DEVICE_TYPE_COMPR_CAPTURE)
return;- wake_up(&stream->runtime->sleep);
+} +EXPORT_SYMBOL_GPL(snd_compr_frame_elapsed);
These are better to be inlined... (And fragment and frame for different directions? Hm, not so intuitive :)
+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);
Avoid BUG_ON() unless you really want to stop the whole operation at this option (such as memory corruption).
- mutex_lock(&stream->device->lock);
- 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;
What's this?
retval = snd_compr_drain(stream);break;- }
- mutex_unlock(&stream->device->lock);
- return retval;
thanks,
Takashi