IR: add lirc support to ir-core

Add the core lirc device interface (http://lirc.org/).

This is a 99.9% unaltered lirc_dev device interface (only change being the path to lirc.h), which has been carried in the Fedora kernels and has been built for numerous other distro kernels for years. In the next two patches in this series, ir-core will be wired up to make use of the lirc interface as one of many IR protocol decoder options. In this case, raw IR will be delivered to the lirc userspace daemon, which will then handle the decoding.

Signed-off-by Jarod Wilson <[email protected]>

---
 drivers/media/IR/Kconfig    |   11 +
 drivers/media/IR/Makefile   |    1 +
 drivers/media/IR/lirc_dev.c |  850 +++++++++++++++++++++++++++++++++++++++++++
 drivers/media/IR/lirc_dev.h |  228 ++++++++++++
 include/media/lirc.h        |  159 ++++++++
 5 files changed, 1249 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/IR/lirc_dev.c
 create mode 100644 drivers/media/IR/lirc_dev.h
 create mode 100644 include/media/lirc.h

drivers/media/IR/Kconfig +11 -0

index 7ffa86f..c3010fb 100644
@@ -8,6 +8,17 @@ config VIDEO_IR
 	depends on IR_CORE
 	default IR_CORE
 
+config LIRC
+	tristate
+	default y
+
+	---help---
+	   Enable this option to build the Linux Infrared Remote
+	   Control (LIRC) core device interface driver. The LIRC
+	   interface passes raw IR to and from userspace, where the
+	   LIRC daemon handles protocol decoding for IR reception ann
+	   encoding for IR transmitting (aka "blasting").
+
 source "drivers/media/IR/keymaps/Kconfig"
 
 config IR_NEC_DECODER

drivers/media/IR/Makefile +1 -0

index b43fe36..3ba00bb 100644
@@ -5,6 +5,7 @@ obj-y += keymaps/
 
 obj-$(CONFIG_IR_CORE) += ir-core.o
 obj-$(CONFIG_VIDEO_IR) += ir-common.o
+obj-$(CONFIG_LIRC) += lirc_dev.o
 obj-$(CONFIG_IR_NEC_DECODER) += ir-nec-decoder.o
 obj-$(CONFIG_IR_RC5_DECODER) += ir-rc5-decoder.o
 obj-$(CONFIG_IR_RC6_DECODER) += ir-rc6-decoder.o

drivers/media/IR/lirc_dev.c new file +618 -0

new file mode 100644
index 0000000..7e45800
@@ -0,0 +1,850 @@
+/*
+ * LIRC base driver
+ *
+ * by Artur Lipowski <[email protected]>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/errno.h>
+#include <linux/ioctl.h>
+#include <linux/fs.h>
+#include <linux/poll.h>
+#include <linux/completion.h>
+#include <linux/errno.h>
+#include <linux/mutex.h>
+#include <linux/wait.h>
+#include <linux/unistd.h>
+#include <linux/kthread.h>
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/cdev.h>
+#include <linux/smp_lock.h>
+#ifdef CONFIG_COMPAT
+#include <linux/compat.h>
+#endif
+
+#include <media/lirc.h>
+#include "lirc_dev.h"
+
+static int debug;
+
+#define IRCTL_DEV_NAME	"BaseRemoteCtl"
+#define NOPLUG		-1
+#define LOGHEAD		"lirc_dev (%s[%d]): "
+
+static dev_t lirc_base_dev;
+
+struct irctl {
+	struct lirc_driver d;
+	int attached;
+	int open;
+
+	struct mutex irctl_lock;
+	struct lirc_buffer *buf;
+	unsigned int chunk_size;
+
+	struct task_struct *task;
+	long jiffies_to_wait;
+
+	struct cdev cdev;
+};
+
+static DEFINE_MUTEX(lirc_dev_lock);
+
+static struct irctl *irctls[MAX_IRCTL_DEVICES];
+
+/* Only used for sysfs but defined to void otherwise */
+static struct class *lirc_class;
+
+/*  helper function
+ *  initializes the irctl structure
+ */
+static void init_irctl(struct irctl *ir)
+{
+	dev_dbg(ir->d.dev, LOGHEAD "initializing irctl\n",
+		ir->d.name, ir->d.minor);
+	mutex_init(&ir->irctl_lock);
+	ir->d.minor = NOPLUG;
+}
+
+static void cleanup(struct irctl *ir)
+{
+	dev_dbg(ir->d.dev, LOGHEAD "cleaning up\n", ir->d.name, ir->d.minor);
+
+	device_destroy(lirc_class, MKDEV(MAJOR(lirc_base_dev), ir->d.minor));
+
+	if (ir->buf != ir->d.rbuf) {
+		lirc_buffer_free(ir->buf);
+		kfree(ir->buf);
+	}
+	ir->buf = NULL;
+}
+
+/*  helper function
+ *  reads key codes from driver and puts them into buffer
+ *  returns 0 on success
+ */
+static int add_to_buf(struct irctl *ir)
+{
+	if (ir->d.add_to_buf) {
+		int res = -ENODATA;
+		int got_data = 0;
+
+		/*
+		 * service the device as long as it is returning
+		 * data and we have space
+		 */
+get_data:
+		res = ir->d.add_to_buf(ir->d.data, ir->buf);
+		if (res == 0) {
+			got_data++;
+			goto get_data;
+		}
+
+		if (res == -ENODEV)
+			kthread_stop(ir->task);
+
+		return got_data ? 0 : res;
+	}
+
+	return 0;
+}
+
+/* main function of the polling thread
+ */
+static int lirc_thread(void *irctl)
+{
+	struct irctl *ir = irctl;
+
+	dev_dbg(ir->d.dev, LOGHEAD "poll thread started\n",
+		ir->d.name, ir->d.minor);
+
+	do {
+		if (ir->open) {
+			if (ir->jiffies_to_wait) {
+				set_current_state(TASK_INTERRUPTIBLE);
+				schedule_timeout(ir->jiffies_to_wait);
+			}
+			if (kthread_should_stop())
+				break;
+			if (!add_to_buf(ir))
+				wake_up_interruptible(&ir->buf->wait_poll);
+		} else {
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule();
+		}
+	} while (!kthread_should_stop());
+
+	dev_dbg(ir->d.dev, LOGHEAD "poll thread ended\n",
+		ir->d.name, ir->d.minor);
+
+	return 0;
+}
+
+
+static struct file_operations fops = {
+	.owner		= THIS_MODULE,
+	.read		= lirc_dev_fop_read,
+	.write		= lirc_dev_fop_write,
+	.poll		= lirc_dev_fop_poll,
+	.ioctl		= lirc_dev_fop_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= lirc_dev_fop_compat_ioctl,
+#endif
+	.open		= lirc_dev_fop_open,
+	.release	= lirc_dev_fop_close,
+};
+
+static int lirc_cdev_add(struct irctl *ir)
+{
+	int retval;
+	struct lirc_driver *d = &ir->d;
+
+	if (d->fops) {
+		cdev_init(&ir->cdev, d->fops);
+		ir->cdev.owner = d->owner;
+	} else {
+		cdev_init(&ir->cdev, &fops);
+		ir->cdev.owner = THIS_MODULE;
+	}
+	kobject_set_name(&ir->cdev.kobj, "lirc%d", d->minor);
+
+	retval = cdev_add(&ir->cdev, MKDEV(MAJOR(lirc_base_dev), d->minor), 1);
+	if (retval)
+		kobject_put(&ir->cdev.kobj);
+
+	return retval;
+}
+
+int lirc_register_driver(struct lirc_driver *d)
+{
+	struct irctl *ir;
+	int minor;
+	int bytes_in_key;
+	unsigned int chunk_size;
+	unsigned int buffer_size;
+	int err;
+
+	if (!d) {
+		printk(KERN_ERR "lirc_dev: lirc_register_driver: "
+		       "driver pointer must be not NULL!\n");
+		err = -EBADRQC;
+		goto out;
+	}
+
+	if (MAX_IRCTL_DEVICES <= d->minor) {
+		dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+			"\"minor\" must be between 0 and %d (%d)!\n",
+			MAX_IRCTL_DEVICES-1, d->minor);
+		err = -EBADRQC;
+		goto out;
+	}
+
+	if (1 > d->code_length || (BUFLEN * 8) < d->code_length) {
+		dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+			"code length in bits for minor (%d) "
+			"must be less than %d!\n",
+			d->minor, BUFLEN * 8);
+		err = -EBADRQC;
+		goto out;
+	}
+
+	dev_dbg(d->dev, "lirc_dev: lirc_register_driver: sample_rate: %d\n",
+		d->sample_rate);
+	if (d->sample_rate) {
+		if (2 > d->sample_rate || HZ < d->sample_rate) {
+			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+				"sample_rate must be between 2 and %d!\n", HZ);
+			err = -EBADRQC;
+			goto out;
+		}
+		if (!d->add_to_buf) {
+			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+				"add_to_buf cannot be NULL when "
+				"sample_rate is set\n");
+			err = -EBADRQC;
+			goto out;
+		}
+	} else if (!(d->fops && d->fops->read) && !d->rbuf) {
+		dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+			"fops->read and rbuf cannot all be NULL!\n");
+		err = -EBADRQC;
+		goto out;
+	} else if (!d->rbuf) {
+		if (!(d->fops && d->fops->read && d->fops->poll &&
+		      d->fops->ioctl)) {
+			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+				"neither read, poll nor ioctl can be NULL!\n");
+			err = -EBADRQC;
+			goto out;
+		}
+	}
+
+	mutex_lock(&lirc_dev_lock);
+
+	minor = d->minor;
+
+	if (minor < 0) {
+		/* find first free slot for driver */
+		for (minor = 0; minor < MAX_IRCTL_DEVICES; minor++)
+			if (!irctls[minor])
+				break;
+		if (MAX_IRCTL_DEVICES == minor) {
+			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+				"no free slots for drivers!\n");
+			err = -ENOMEM;
+			goto out_lock;
+		}
+	} else if (irctls[minor]) {
+		dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+			"minor (%d) just registered!\n", minor);
+		err = -EBUSY;
+		goto out_lock;
+	}
+
+	ir = kzalloc(sizeof(struct irctl), GFP_KERNEL);
+	if (!ir) {
+		err = -ENOMEM;
+		goto out_lock;
+	}
+	init_irctl(ir);
+	irctls[minor] = ir;
+	d->minor = minor;
+
+	if (d->sample_rate) {
+		ir->jiffies_to_wait = HZ / d->sample_rate;
+	} else {
+		/* it means - wait for external event in task queue */
+		ir->jiffies_to_wait = 0;
+	}
+
+	/* some safety check 8-) */
+	d->name[sizeof(d->name)-1] = '\0';
+
+	bytes_in_key = BITS_TO_LONGS(d->code_length) +
+			(d->code_length % 8 ? 1 : 0);
+	buffer_size = d->buffer_size ? d->buffer_size : BUFLEN / bytes_in_key;
+	chunk_size  = d->chunk_size  ? d->chunk_size  : bytes_in_key;
+
+	if (d->rbuf) {
+		ir->buf = d->rbuf;
+	} else {
+		ir->buf = kmalloc(sizeof(struct lirc_buffer), GFP_KERNEL);
+		if (!ir->buf) {
+			err = -ENOMEM;
+			goto out_lock;
+		}
+		err = lirc_buffer_init(ir->buf, chunk_size, buffer_size);
+		if (err) {
+			kfree(ir->buf);
+			goto out_lock;
+		}
+	}
+	ir->chunk_size = ir->buf->chunk_size;
+
+	if (d->features == 0)
+		d->features = LIRC_CAN_REC_LIRCCODE;
+
+	ir->d = *d;
+	ir->d.minor = minor;
+
+	device_create(lirc_class, ir->d.dev,
+		      MKDEV(MAJOR(lirc_base_dev), ir->d.minor), NULL,
+		      "lirc%u", ir->d.minor);
+
+	if (d->sample_rate) {
+		/* try to fire up polling thread */
+		ir->task = kthread_run(lirc_thread, (void *)ir, "lirc_dev");
+		if (IS_ERR(ir->task)) {
+			dev_err(d->dev, "lirc_dev: lirc_register_driver: "
+				"cannot run poll thread for minor = %d\n",
+				d->minor);
+			err = -ECHILD;
+			goto out_sysfs;
+		}
+	}
+
+	err = lirc_cdev_add(ir);
+	if (err)
+		goto out_sysfs;
+
+	ir->attached = 1;
+	mutex_unlock(&lirc_dev_lock);
+
+	dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
+		 ir->d.name, ir->d.minor);
+	return minor;
+
+out_sysfs:
+	device_destroy(lirc_class, MKDEV(MAJOR(lirc_base_dev), ir->d.minor));
+out_lock:
+	mutex_unlock(&lirc_dev_lock);
+out:
+	return err;
+}
+EXPORT_SYMBOL(lirc_register_driver);
+
+int lirc_unregister_driver(int minor)
+{
+	struct irctl *ir;
+
+	if (minor < 0 || minor >= MAX_IRCTL_DEVICES) {
+		printk(KERN_ERR "lirc_dev: lirc_unregister_driver: "
+		       "\"minor (%d)\" must be between 0 and %d!\n",
+		       minor, MAX_IRCTL_DEVICES-1);
+		return -EBADRQC;
+	}
+
+	ir = irctls[minor];
+
+	mutex_lock(&lirc_dev_lock);
+
+	if (ir->d.minor != minor) {
+		printk(KERN_ERR "lirc_dev: lirc_unregister_driver: "
+		       "minor (%d) device not registered!", minor);
+		mutex_unlock(&lirc_dev_lock);
+		return -ENOENT;
+	}
+
+	/* end up polling thread */
+	if (ir->task)
+		kthread_stop(ir->task);
+
+	dev_dbg(ir->d.dev, "lirc_dev: driver %s unregistered from minor = %d\n",
+		ir->d.name, ir->d.minor);
+
+	ir->attached = 0;
+	if (ir->open) {
+		dev_dbg(ir->d.dev, LOGHEAD "releasing opened driver\n",
+			ir->d.name, ir->d.minor);
+		wake_up_interruptible(&ir->buf->wait_poll);
+		mutex_lock(&ir->irctl_lock);
+		ir->d.set_use_dec(ir->d.data);
+		module_put(ir->d.owner);
+		mutex_unlock(&ir->irctl_lock);
+		cdev_del(&ir->cdev);
+	} else {
+		cleanup(ir);
+		cdev_del(&ir->cdev);
+		kfree(ir);
+		irctls[minor] = NULL;
+	}
+
+	mutex_unlock(&lirc_dev_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(lirc_unregister_driver);
+
+int lirc_dev_fop_open(struct inode *inode, struct file *file)
+{
+	struct irctl *ir;
+	int retval = 0;
+
+	if (iminor(inode) >= MAX_IRCTL_DEVICES) {
+		printk(KERN_WARNING "lirc_dev [%d]: open result = -ENODEV\n",
+		       iminor(inode));
+		return -ENODEV;
+	}
+
+	if (mutex_lock_interruptible(&lirc_dev_lock))
+		return -ERESTARTSYS;
+
+	ir = irctls[iminor(inode)];
+	if (!ir) {
+		retval = -ENODEV;
+		goto error;
+	}
+
+	dev_dbg(ir->d.dev, LOGHEAD "open called\n", ir->d.name, ir->d.minor);
+
+	if (ir->d.minor == NOPLUG) {
+		retval = -ENODEV;
+		goto error;
+	}
+
+	if (ir->open) {
+		retval = -EBUSY;
+		goto error;
+	}
+
+	if (try_module_get(ir->d.owner)) {
+		++ir->open;
+		retval = ir->d.set_use_inc(ir->d.data);
+
+		if (retval) {
+			module_put(ir->d.owner);
+			--ir->open;
+		} else {
+			lirc_buffer_clear(ir->buf);
+		}
+		if (ir->task)
+			wake_up_process(ir->task);
+	}
+
+error:
+	if (ir)
+		dev_dbg(ir->d.dev, LOGHEAD "open result = %d\n",
+			ir->d.name, ir->d.minor, retval);
+
+	mutex_unlock(&lirc_dev_lock);
+
+	return retval;
+}
+EXPORT_SYMBOL(lirc_dev_fop_open);
+
+int lirc_dev_fop_close(struct inode *inode, struct file *file)
+{
+	struct irctl *ir = irctls[iminor(inode)];
+
+	dev_dbg(ir->d.dev, LOGHEAD "close called\n", ir->d.name, ir->d.minor);
+
+	WARN_ON(mutex_lock_killable(&lirc_dev_lock));
+
+	--ir->open;
+	if (ir->attached) {
+		ir->d.set_use_dec(ir->d.data);
+		module_put(ir->d.owner);
+	} else {
+		cleanup(ir);
+		irctls[ir->d.minor] = NULL;
+		kfree(ir);
+	}
+
+	mutex_unlock(&lirc_dev_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL(lirc_dev_fop_close);
+
+unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait)
+{
+	struct irctl *ir = irctls[iminor(file->f_dentry->d_inode)];
+	unsigned int ret;
+
+	dev_dbg(ir->d.dev, LOGHEAD "poll called\n", ir->d.name, ir->d.minor);
+
+	if (!ir->attached) {
+		mutex_unlock(&ir->irctl_lock);
+		return POLLERR;
+	}
+
+	poll_wait(file, &ir->buf->wait_poll, wait);
+
+	if (ir->buf)
+		if (lirc_buffer_empty(ir->buf))
+			ret = 0;
+		else
+			ret = POLLIN | POLLRDNORM;
+	else
+		ret = POLLERR;
+
+	dev_dbg(ir->d.dev, LOGHEAD "poll result = %d\n",
+		ir->d.name, ir->d.minor, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL(lirc_dev_fop_poll);
+
+int lirc_dev_fop_ioctl(struct inode *inode, struct file *file,
+		       unsigned int cmd, unsigned long arg)
+{
+	unsigned long mode;
+	int result = 0;
+	struct irctl *ir = irctls[iminor(inode)];
+
+	dev_dbg(ir->d.dev, LOGHEAD "ioctl called (0x%x)\n",
+		ir->d.name, ir->d.minor, cmd);
+
+	if (ir->d.minor == NOPLUG || !ir->attached) {
+		dev_dbg(ir->d.dev, LOGHEAD "ioctl result = -ENODEV\n",
+			ir->d.name, ir->d.minor);
+		return -ENODEV;
+	}
+
+	switch (cmd) {
+	case LIRC_GET_FEATURES:
+		result = put_user(ir->d.features, (unsigned long *)arg);
+		break;
+	case LIRC_GET_REC_MODE:
+		if (!(ir->d.features & LIRC_CAN_REC_MASK))
+			return -ENOSYS;
+
+		result = put_user(LIRC_REC2MODE
+				  (ir->d.features & LIRC_CAN_REC_MASK),
+				  (unsigned long *)arg);
+		break;
+	case LIRC_SET_REC_MODE:
+		if (!(ir->d.features & LIRC_CAN_REC_MASK))
+			return -ENOSYS;
+
+		result = get_user(mode, (unsigned long *)arg);
+		if (!result && !(LIRC_MODE2REC(mode) & ir->d.features))
+			result = -EINVAL;
+		/*
+		 * FIXME: We should actually set the mode somehow but
+		 * for now, lirc_serial doesn't support mode changing either
+		 */
+		break;
+	case LIRC_GET_LENGTH:
+		result = put_user(ir->d.code_length, (unsigned long *)arg);
+		break;
+	case LIRC_GET_MIN_TIMEOUT:
+		if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
+		    ir->d.min_timeout == 0)
+			return -ENOSYS;
+
+		result = put_user(ir->d.min_timeout, (int *) arg);
+		break;
+	case LIRC_GET_MAX_TIMEOUT:
+		if (!(ir->d.features & LIRC_CAN_SET_REC_TIMEOUT) ||
+		    ir->d.max_timeout == 0)
+			return -ENOSYS;
+
+		result = put_user(ir->d.max_timeout, (int *) arg);
+		break;
+	default:
+		result = -EINVAL;
+	}
+
+	dev_dbg(ir->d.dev, LOGHEAD "ioctl result = %d\n",
+		ir->d.name, ir->d.minor, result);
+
+	return result;
+}
+EXPORT_SYMBOL(lirc_dev_fop_ioctl);
+
+#ifdef CONFIG_COMPAT
+#define LIRC_GET_FEATURES_COMPAT32     _IOR('i', 0x00000000, __u32)
+
+#define LIRC_GET_SEND_MODE_COMPAT32    _IOR('i', 0x00000001, __u32)
+#define LIRC_GET_REC_MODE_COMPAT32     _IOR('i', 0x00000002, __u32)
+
+#define LIRC_GET_LENGTH_COMPAT32       _IOR('i', 0x0000000f, __u32)
+
+#define LIRC_SET_SEND_MODE_COMPAT32    _IOW('i', 0x00000011, __u32)
+#define LIRC_SET_REC_MODE_COMPAT32     _IOW('i', 0x00000012, __u32)
+
+long lirc_dev_fop_compat_ioctl(struct file *file,
+			       unsigned int cmd32,
+			       unsigned long arg)
+{
+	mm_segment_t old_fs;
+	int ret;
+	unsigned long val;
+	unsigned int cmd;
+
+	switch (cmd32) {
+	case LIRC_GET_FEATURES_COMPAT32:
+	case LIRC_GET_SEND_MODE_COMPAT32:
+	case LIRC_GET_REC_MODE_COMPAT32:
+	case LIRC_GET_LENGTH_COMPAT32:

drivers/media/IR/lirc_dev.c new file +33 -0

+		/*
+		 * tell lirc_dev_fop_ioctl that it's safe to use the pointer
+		 * to val which is in kernel address space and not in
+		 * user address space.
+		 */
+		old_fs = get_fs();
+		set_fs(KERNEL_DS);
+
+		cmd = _IOC(_IOC_DIR(cmd32), _IOC_TYPE(cmd32), _IOC_NR(cmd32),
+		(_IOC_TYPECHECK(unsigned long)));
+		ret = lirc_dev_fop_ioctl(file->f_path.dentry->d_inode, file,
+					 cmd, (unsigned long)(&val));
+
+		set_fs(old_fs);
+		unlock_kernel();
+	switch (cmd) {
+	case LIRC_GET_FEATURES:
+	case LIRC_GET_SEND_MODE:
+	case LIRC_GET_REC_MODE:
+	case LIRC_GET_LENGTH:
+		if (!ret && put_user(val, (__u32 *)compat_ptr(arg)))
+			return -EFAULT;
+		break;
+	}
+	return ret;
+
+	case LIRC_GET_SEND_CARRIER:
+	case LIRC_GET_REC_CARRIER:
+	case LIRC_GET_SEND_DUTY_CYCLE:
+	case LIRC_GET_REC_DUTY_CYCLE:
+	case LIRC_GET_REC_RESOLUTION:
+	case LIRC_SET_SEND_CARRIER:
+	case LIRC_SET_REC_CARRIER:

drivers/media/IR/lirc_dev.c new file +179 -0

+		cmd = cmd32;
+		ret = lirc_dev_fop_ioctl(file->f_path.dentry->d_inode,
+					 file, cmd, arg);
+		unlock_kernel();
+		return ret;
+	default:
+		/* unknown */
+		printk(KERN_ERR "lirc_dev: %s(%s:%d): Unknown cmd %08x\n",
+		       __func__, current->comm, current->pid, cmd32);
+		return -ENOIOCTLCMD;
+	}
+}
+EXPORT_SYMBOL(lirc_dev_fop_compat_ioctl);
+#endif
+
+
+ssize_t lirc_dev_fop_read(struct file *file,
+			  char *buffer,
+			  size_t length,
+			  loff_t *ppos)
+{
+	struct irctl *ir = irctls[iminor(file->f_dentry->d_inode)];
+	unsigned char buf[ir->chunk_size];
+	int ret = 0, written = 0;
+	DECLARE_WAITQUEUE(wait, current);
+
+	dev_dbg(ir->d.dev, LOGHEAD "read called\n", ir->d.name, ir->d.minor);
+
+	if (mutex_lock_interruptible(&ir->irctl_lock))
+		return -ERESTARTSYS;
+	if (!ir->attached) {
+		mutex_unlock(&ir->irctl_lock);
+		return -ENODEV;
+	}
+
+	if (length % ir->chunk_size) {
+		dev_dbg(ir->d.dev, LOGHEAD "read result = -EINVAL\n",
+			ir->d.name, ir->d.minor);
+		mutex_unlock(&ir->irctl_lock);
+		return -EINVAL;
+	}
+
+	/*
+	 * we add ourselves to the task queue before buffer check
+	 * to avoid losing scan code (in case when queue is awaken somewhere
+	 * between while condition checking and scheduling)
+	 */
+	add_wait_queue(&ir->buf->wait_poll, &wait);
+	set_current_state(TASK_INTERRUPTIBLE);
+
+	/*
+	 * while we didn't provide 'length' bytes, device is opened in blocking
+	 * mode and 'copy_to_user' is happy, wait for data.
+	 */
+	while (written < length && ret == 0) {
+		if (lirc_buffer_empty(ir->buf)) {
+			/* According to the read(2) man page, 'written' can be
+			 * returned as less than 'length', instead of blocking
+			 * again, returning -EWOULDBLOCK, or returning
+			 * -ERESTARTSYS */
+			if (written)
+				break;
+			if (file->f_flags & O_NONBLOCK) {
+				ret = -EWOULDBLOCK;
+				break;
+			}
+			if (signal_pending(current)) {
+				ret = -ERESTARTSYS;
+				break;
+			}
+
+			mutex_unlock(&ir->irctl_lock);
+			schedule();
+			set_current_state(TASK_INTERRUPTIBLE);
+
+			if (mutex_lock_interruptible(&ir->irctl_lock)) {
+				ret = -ERESTARTSYS;
+				break;
+			}
+
+			if (!ir->attached) {
+				ret = -ENODEV;
+				break;
+			}
+		} else {
+			lirc_buffer_read(ir->buf, buf);
+			ret = copy_to_user((void *)buffer+written, buf,
+					   ir->buf->chunk_size);
+			written += ir->buf->chunk_size;
+		}
+	}
+
+	remove_wait_queue(&ir->buf->wait_poll, &wait);
+	set_current_state(TASK_RUNNING);
+	mutex_unlock(&ir->irctl_lock);
+
+	dev_dbg(ir->d.dev, LOGHEAD "read result = %s (%d)\n",
+		ir->d.name, ir->d.minor, ret ? "-EFAULT" : "OK", ret);
+
+	return ret ? ret : written;
+}
+EXPORT_SYMBOL(lirc_dev_fop_read);
+
+void *lirc_get_pdata(struct file *file)
+{
+	void *data = NULL;
+
+	if (file && file->f_dentry && file->f_dentry->d_inode &&
+	    file->f_dentry->d_inode->i_rdev) {
+		struct irctl *ir;
+		ir = irctls[iminor(file->f_dentry->d_inode)];
+		data = ir->d.data;
+	}
+
+	return data;
+}
+EXPORT_SYMBOL(lirc_get_pdata);
+
+
+ssize_t lirc_dev_fop_write(struct file *file, const char *buffer,
+			   size_t length, loff_t *ppos)
+{
+	struct irctl *ir = irctls[iminor(file->f_dentry->d_inode)];
+
+	dev_dbg(ir->d.dev, LOGHEAD "write called\n", ir->d.name, ir->d.minor);
+
+	if (!ir->attached)
+		return -ENODEV;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(lirc_dev_fop_write);
+
+
+static int __init lirc_dev_init(void)
+{
+	int retval;
+
+	lirc_class = class_create(THIS_MODULE, "lirc");
+	if (IS_ERR(lirc_class)) {
+		retval = PTR_ERR(lirc_class);
+		printk(KERN_ERR "lirc_dev: class_create failed\n");
+		goto error;
+	}
+
+	retval = alloc_chrdev_region(&lirc_base_dev, 0, MAX_IRCTL_DEVICES,
+				     IRCTL_DEV_NAME);
+	if (retval) {
+		class_destroy(lirc_class);
+		printk(KERN_ERR "lirc_dev: alloc_chrdev_region failed\n");
+		goto error;
+	}
+
+
+	printk(KERN_INFO "lirc_dev: IR Remote Control driver registered, "
+	       "major %d \n", MAJOR(lirc_base_dev));
+
+error:
+	return retval;
+}
+
+
+
+static void __exit lirc_dev_exit(void)
+{
+	class_destroy(lirc_class);
+	unregister_chrdev_region(lirc_base_dev, MAX_IRCTL_DEVICES);
+	printk(KERN_INFO "lirc_dev: module unloaded\n");
+}
+
+module_init(lirc_dev_init);
+module_exit(lirc_dev_exit);
+
+MODULE_DESCRIPTION("LIRC base driver module");
+MODULE_AUTHOR("Artur Lipowski");
+MODULE_LICENSE("GPL");
+
+module_param(debug, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(debug, "Enable debugging messages");

drivers/media/IR/lirc_dev.h new file +228 -0

new file mode 100644
index 0000000..533356d
@@ -0,0 +1,228 @@
+/*
+ * LIRC base driver
+ *
+ * by Artur Lipowski <[email protected]>
+ *        This code is licensed under GNU GPL
+ *
+ */
+
+#ifndef _LINUX_LIRC_DEV_H
+#define _LINUX_LIRC_DEV_H
+
+#define MAX_IRCTL_DEVICES 4
+#define BUFLEN            16
+
+#define mod(n, div) ((n) % (div))
+
+#include <linux/slab.h>
+#include <linux/fs.h>
+#include <linux/ioctl.h>
+#include <linux/poll.h>
+#include <linux/kfifo.h>
+#include <media/lirc.h>
+
+struct lirc_buffer {
+	wait_queue_head_t wait_poll;
+	spinlock_t fifo_lock;
+	unsigned int chunk_size;
+	unsigned int size; /* in chunks */
+	/* Using chunks instead of bytes pretends to simplify boundary checking
+	 * And should allow for some performance fine tunning later */
+	struct kfifo fifo;
+	u8 fifo_initialized;
+};
+
+static inline void lirc_buffer_clear(struct lirc_buffer *buf)
+{
+	unsigned long flags;
+
+	if (buf->fifo_initialized) {
+		spin_lock_irqsave(&buf->fifo_lock, flags);
+		kfifo_reset(&buf->fifo);
+		spin_unlock_irqrestore(&buf->fifo_lock, flags);
+	} else
+		WARN(1, "calling %s on an uninitialized lirc_buffer\n",
+		     __func__);
+}
+
+static inline int lirc_buffer_init(struct lirc_buffer *buf,
+				    unsigned int chunk_size,
+				    unsigned int size)
+{
+	int ret;
+
+	init_waitqueue_head(&buf->wait_poll);
+	spin_lock_init(&buf->fifo_lock);
+	buf->chunk_size = chunk_size;
+	buf->size = size;
+	ret = kfifo_alloc(&buf->fifo, size * chunk_size, GFP_KERNEL);
+	if (ret == 0)
+		buf->fifo_initialized = 1;
+
+	return ret;
+}
+
+static inline void lirc_buffer_free(struct lirc_buffer *buf)
+{
+	if (buf->fifo_initialized) {
+		kfifo_free(&buf->fifo);
+		buf->fifo_initialized = 0;
+	} else
+		WARN(1, "calling %s on an uninitialized lirc_buffer\n",
+		     __func__);
+}
+
+static inline int lirc_buffer_len(struct lirc_buffer *buf)
+{
+	int len;
+	unsigned long flags;
+
+	spin_lock_irqsave(&buf->fifo_lock, flags);
+	len = kfifo_len(&buf->fifo);
+	spin_unlock_irqrestore(&buf->fifo_lock, flags);
+
+	return len;
+}
+
+static inline int lirc_buffer_full(struct lirc_buffer *buf)
+{
+	return lirc_buffer_len(buf) == buf->size * buf->chunk_size;
+}
+
+static inline int lirc_buffer_empty(struct lirc_buffer *buf)
+{
+	return !lirc_buffer_len(buf);
+}
+
+static inline int lirc_buffer_available(struct lirc_buffer *buf)
+{
+	return buf->size - (lirc_buffer_len(buf) / buf->chunk_size);
+}
+
+static inline unsigned int lirc_buffer_read(struct lirc_buffer *buf,
+					    unsigned char *dest)
+{
+	unsigned int ret = 0;
+
+	if (lirc_buffer_len(buf) >= buf->chunk_size)
+		ret = kfifo_out_locked(&buf->fifo, dest, buf->chunk_size,
+				       &buf->fifo_lock);
+	return ret;
+
+}
+
+static inline unsigned int lirc_buffer_write(struct lirc_buffer *buf,
+					     unsigned char *orig)
+{
+	unsigned int ret;
+
+	ret = kfifo_in_locked(&buf->fifo, orig, buf->chunk_size,
+			      &buf->fifo_lock);
+
+	return ret;
+}
+
+struct lirc_driver {
+	char name[40];
+	int minor;
+	unsigned long code_length;
+	unsigned int buffer_size; /* in chunks holding one code each */
+	int sample_rate;
+	unsigned long features;
+
+	unsigned int chunk_size;
+
+	void *data;
+	int min_timeout;
+	int max_timeout;
+	int (*add_to_buf) (void *data, struct lirc_buffer *buf);
+	struct lirc_buffer *rbuf;
+	int (*set_use_inc) (void *data);
+	void (*set_use_dec) (void *data);
+	struct file_operations *fops;
+	struct device *dev;
+	struct module *owner;
+};
+
+/* name:
+ * this string will be used for logs
+ *
+ * minor:
+ * indicates minor device (/dev/lirc) number for registered driver
+ * if caller fills it with negative value, then the first free minor
+ * number will be used (if available)
+ *
+ * code_length:
+ * length of the remote control key code expressed in bits
+ *
+ * sample_rate:
+ *
+ * data:
+ * it may point to any driver data and this pointer will be passed to
+ * all callback functions
+ *
+ * add_to_buf:
+ * add_to_buf will be called after specified period of the time or
+ * triggered by the external event, this behavior depends on value of
+ * the sample_rate this function will be called in user context. This
+ * routine should return 0 if data was added to the buffer and
+ * -ENODATA if none was available. This should add some number of bits
+ * evenly divisible by code_length to the buffer
+ *
+ * rbuf:
+ * if not NULL, it will be used as a read buffer, you will have to
+ * write to the buffer by other means, like irq's (see also
+ * lirc_serial.c).
+ *
+ * set_use_inc:
+ * set_use_inc will be called after device is opened
+ *
+ * set_use_dec:
+ * set_use_dec will be called after device is closed
+ *
+ * fops:
+ * file_operations for drivers which don't fit the current driver model.
+ *
+ * Some ioctl's can be directly handled by lirc_dev if the driver's
+ * ioctl function is NULL or if it returns -ENOIOCTLCMD (see also
+ * lirc_serial.c).
+ *
+ * owner:
+ * the module owning this struct
+ *
+ */
+
+
+/* following functions can be called ONLY from user context
+ *
+ * returns negative value on error or minor number
+ * of the registered device if success
+ * contents of the structure pointed by p is copied
+ */
+extern int lirc_register_driver(struct lirc_driver *d);
+
+/* returns negative value on error or 0 if success
+*/
+extern int lirc_unregister_driver(int minor);
+
+/* Returns the private data stored in the lirc_driver
+ * associated with the given device file pointer.
+ */
+void *lirc_get_pdata(struct file *file);
+
+/* default file operations
+ * used by drivers if they override only some operations
+ */
+int lirc_dev_fop_open(struct inode *inode, struct file *file);
+int lirc_dev_fop_close(struct inode *inode, struct file *file);
+unsigned int lirc_dev_fop_poll(struct file *file, poll_table *wait);
+int lirc_dev_fop_ioctl(struct inode *inode, struct file *file,
+		       unsigned int cmd, unsigned long arg);
+ssize_t lirc_dev_fop_read(struct file *file, char *buffer, size_t length,
+			  loff_t *ppos);
+ssize_t lirc_dev_fop_write(struct file *file, const char *buffer, size_t length,
+			   loff_t *ppos);
+long lirc_dev_fop_compat_ioctl(struct file *file, unsigned int cmd32,
+			       unsigned long arg);
+
+#endif

include/media/lirc.h new file +85 -0

new file mode 100644
index 0000000..9ca6876
@@ -0,0 +1,159 @@
+/*
+ * lirc.h - linux infrared remote control header file
+ * last modified 2007/09/27
+ */
+
+#ifndef _LINUX_LIRC_H
+#define _LINUX_LIRC_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* <obsolete> */
+#define PULSE_BIT       0x01000000
+#define PULSE_MASK      0x00FFFFFF
+/* </obsolete> */
+
+#define LIRC_MODE2_SPACE     0x00000000
+#define LIRC_MODE2_PULSE     0x01000000
+#define LIRC_MODE2_FREQUENCY 0x02000000
+#define LIRC_MODE2_TIMEOUT   0x03000000
+
+#define LIRC_VALUE_MASK      0x00FFFFFF
+#define LIRC_MODE2_MASK      0xFF000000
+
+#define LIRC_SPACE(val) (((val)&LIRC_VALUE_MASK) | LIRC_MODE2_SPACE)
+#define LIRC_PULSE(val) (((val)&LIRC_VALUE_MASK) | LIRC_MODE2_PULSE)
+#define LIRC_FREQUENCY(val) (((val)&LIRC_VALUE_MASK) | LIRC_MODE2_FREQUENCY)
+#define LIRC_TIMEOUT(val) (((val)&LIRC_VALUE_MASK) | LIRC_MODE2_TIMEOUT)
+
+#define LIRC_VALUE(val) ((val)&LIRC_VALUE_MASK)
+#define LIRC_MODE2(val) ((val)&LIRC_MODE2_MASK)
+
+#define LIRC_IS_SPACE(val) (LIRC_MODE2(val) == LIRC_MODE2_SPACE)
+#define LIRC_IS_PULSE(val) (LIRC_MODE2(val) == LIRC_MODE2_PULSE)
+#define LIRC_IS_FREQUENCY(val) (LIRC_MODE2(val) == LIRC_MODE2_FREQUENCY)
+#define LIRC_IS_TIMEOUT(val) (LIRC_MODE2(val) == LIRC_MODE2_TIMEOUT)
+
+/*** lirc compatible hardware features ***/
+
+#define LIRC_MODE2SEND(x) (x)
+#define LIRC_SEND2MODE(x) (x)
+#define LIRC_MODE2REC(x) ((x) << 16)
+#define LIRC_REC2MODE(x) ((x) >> 16)
+
+#define LIRC_MODE_RAW                  0x00000001
+#define LIRC_MODE_PULSE                0x00000002
+#define LIRC_MODE_MODE2                0x00000004
+#define LIRC_MODE_LIRCCODE             0x00000010
+
+
+#define LIRC_CAN_SEND_RAW              LIRC_MODE2SEND(LIRC_MODE_RAW)
+#define LIRC_CAN_SEND_PULSE            LIRC_MODE2SEND(LIRC_MODE_PULSE)
+#define LIRC_CAN_SEND_MODE2            LIRC_MODE2SEND(LIRC_MODE_MODE2)
+#define LIRC_CAN_SEND_LIRCCODE         LIRC_MODE2SEND(LIRC_MODE_LIRCCODE)
+
+#define LIRC_CAN_SEND_MASK             0x0000003f
+
+#define LIRC_CAN_SET_SEND_CARRIER      0x00000100
+#define LIRC_CAN_SET_SEND_DUTY_CYCLE   0x00000200
+#define LIRC_CAN_SET_TRANSMITTER_MASK  0x00000400
+
+#define LIRC_CAN_REC_RAW               LIRC_MODE2REC(LIRC_MODE_RAW)
+#define LIRC_CAN_REC_PULSE             LIRC_MODE2REC(LIRC_MODE_PULSE)
+#define LIRC_CAN_REC_MODE2             LIRC_MODE2REC(LIRC_MODE_MODE2)
+#define LIRC_CAN_REC_LIRCCODE          LIRC_MODE2REC(LIRC_MODE_LIRCCODE)
+
+#define LIRC_CAN_REC_MASK              LIRC_MODE2REC(LIRC_CAN_SEND_MASK)
+
+#define LIRC_CAN_SET_REC_CARRIER       (LIRC_CAN_SET_SEND_CARRIER << 16)
+#define LIRC_CAN_SET_REC_DUTY_CYCLE    (LIRC_CAN_SET_SEND_DUTY_CYCLE << 16)
+
+#define LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE 0x40000000
+#define LIRC_CAN_SET_REC_CARRIER_RANGE    0x80000000
+#define LIRC_CAN_GET_REC_RESOLUTION       0x20000000
+#define LIRC_CAN_SET_REC_TIMEOUT          0x10000000
+#define LIRC_CAN_SET_REC_FILTER           0x08000000
+
+#define LIRC_CAN_MEASURE_CARRIER          0x02000000
+
+#define LIRC_CAN_SEND(x) ((x)&LIRC_CAN_SEND_MASK)
+#define LIRC_CAN_REC(x) ((x)&LIRC_CAN_REC_MASK)
+
+#define LIRC_CAN_NOTIFY_DECODE            0x01000000
+
+/*** IOCTL commands for lirc driver ***/

include/media/lirc.h new file +23 -0

+
+/*
+ * when a timeout != 0 is set the driver will send a
+ * LIRC_MODE2_TIMEOUT data packet, otherwise LIRC_MODE2_TIMEOUT is
+ * never sent, timeout is disabled by default
+ */
+#define LIRC_SET_REC_TIMEOUT           _IOW('i', 0x00000018, uint32_t)
+
+/*
+ * pulses shorter than this are filtered out by hardware (software
+ * emulation in lirc_dev?)
+ */
+#define LIRC_SET_REC_FILTER_PULSE      _IOW('i', 0x00000019, uint32_t)
+/*
+ * spaces shorter than this are filtered out by hardware (software
+ * emulation in lirc_dev?)
+ */
+#define LIRC_SET_REC_FILTER_SPACE      _IOW('i', 0x0000001a, uint32_t)
+/*
+ * if filter cannot be set independantly for pulse/space, this should
+ * be used
+ */
+#define LIRC_SET_REC_FILTER            _IOW('i', 0x0000001b, uint32_t)

Em 01-06-2010 17:51, Jarod Wilson escreveu:

Quoted 22 lines

Add the core lirc device interface (http://lirc.org/).

This is a 99.9% unaltered lirc_dev device interface (only change being the path to lirc.h), which has been carried in the Fedora kernels and has been built for numerous other distro kernels for years. In the next two patches in this series, ir-core will be wired up to make use of the lirc interface as one of many IR protocol decoder options. In this case, raw IR will be delivered to the lirc userspace daemon, which will then handle the decoding.

Signed-off-by Jarod Wilson <[email protected]>

---
 drivers/media/IR/Kconfig    |   11 +
 drivers/media/IR/Makefile   |    1 +
 drivers/media/IR/lirc_dev.c |  850 +++++++++++++++++++++++++++++++++++++++++++
 drivers/media/IR/lirc_dev.h |  228 ++++++++++++
 include/media/lirc.h        |  159 ++++++++
 5 files changed, 1249 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/IR/lirc_dev.c
 create mode 100644 drivers/media/IR/lirc_dev.h
 create mode 100644 include/media/lirc.h

Quoted 25 lines

drivers/media/IR/lirc_dev.c new file +25 -0

+#ifdef CONFIG_COMPAT
+#define LIRC_GET_FEATURES_COMPAT32     _IOR('i', 0x00000000, __u32)
+
+#define LIRC_GET_SEND_MODE_COMPAT32    _IOR('i', 0x00000001, __u32)
+#define LIRC_GET_REC_MODE_COMPAT32     _IOR('i', 0x00000002, __u32)
+
+#define LIRC_GET_LENGTH_COMPAT32       _IOR('i', 0x0000000f, __u32)
+
+#define LIRC_SET_SEND_MODE_COMPAT32    _IOW('i', 0x00000011, __u32)
+#define LIRC_SET_REC_MODE_COMPAT32     _IOW('i', 0x00000012, __u32)
+
+long lirc_dev_fop_compat_ioctl(struct file *file,
+			       unsigned int cmd32,
+			       unsigned long arg)
+{
+	mm_segment_t old_fs;
+	int ret;
+	unsigned long val;
+	unsigned int cmd;
+
+	switch (cmd32) {
+	case LIRC_GET_FEATURES_COMPAT32:
+	case LIRC_GET_SEND_MODE_COMPAT32:
+	case LIRC_GET_REC_MODE_COMPAT32:
+	case LIRC_GET_LENGTH_COMPAT32:

drivers/media/IR/lirc_dev.c line 628

Quoted 10 lines

drivers/media/IR/lirc_dev.c new file +10 -0

+	case LIRC_SET_SEND_MODE_COMPAT32:
+	case LIRC_SET_REC_MODE_COMPAT32:
+		/*
+		 * These commands expect (unsigned long *) arg
+		 * but the 32-bit app supplied (__u32 *).
+		 * Conversion is required.
+		 */
+		if (get_user(val, (__u32 *)compat_ptr(arg)))
+			return -EFAULT;
+		lock_kernel();

Hmm... BKL? Are you sure you need it here? As BKL are being removed, please rewrite it to use another kind of lock.

On a first glance, I suspect that you should be locking &ir->irctl_lock inside lirc_dev_fop_ioctl() and just remove the BKL calls on lirc_dev_fop_compat_ioctl().

Btw, as this is the first in-tree kernel driver for lirc, I would just define the lirc ioctls with __u32 and remove the entire compat stuff.

Quoted 4 lines

+/*
+ * from the next key press on the driver will send
+ * LIRC_MODE2_FREQUENCY packets
+ */

include/media/lirc.h line 159

Quoted 4 lines

include/media/lirc.h new file +4 -0

+#define LIRC_MEASURE_CARRIER_ENABLE    _IO('i', 0x00000021)
+#define LIRC_MEASURE_CARRIER_DISABLE   _IO('i', 0x00000022)
+
+#endif

Wow! There are lots of ioctls that aren't currently used. IMO, the better is to add at the file just the ioctls that are currently in usage, and to put some documentation about them at /Documentation.

Quoted 23 lines

Em 01-06-2010 17:51, Jarod Wilson escreveu:

Quoted 22 lines

Add the core lirc device interface (http://lirc.org/).

This is a 99.9% unaltered lirc_dev device interface (only change being the path to lirc.h), which has been carried in the Fedora kernels and has been built for numerous other distro kernels for years. In the next two patches in this series, ir-core will be wired up to make use of the lirc interface as one of many IR protocol decoder options. In this case, raw IR will be delivered to the lirc userspace daemon, which will then handle the decoding.

Signed-off-by Jarod Wilson <[email protected]>

---
 drivers/media/IR/Kconfig    |   11 +
 drivers/media/IR/Makefile   |    1 +
 drivers/media/IR/lirc_dev.c |  850 +++++++++++++++++++++++++++++++++++++++++++
 drivers/media/IR/lirc_dev.h |  228 ++++++++++++
 include/media/lirc.h        |  159 ++++++++
 5 files changed, 1249 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/IR/lirc_dev.c
 create mode 100644 drivers/media/IR/lirc_dev.h
 create mode 100644 include/media/lirc.h

...

Quoted 39 lines

Quoted 25 lines

drivers/media/IR/lirc_dev.c new file +25 -0

+#ifdef CONFIG_COMPAT
+#define LIRC_GET_FEATURES_COMPAT32     _IOR('i', 0x00000000, __u32)
+
+#define LIRC_GET_SEND_MODE_COMPAT32    _IOR('i', 0x00000001, __u32)
+#define LIRC_GET_REC_MODE_COMPAT32     _IOR('i', 0x00000002, __u32)
+
+#define LIRC_GET_LENGTH_COMPAT32       _IOR('i', 0x0000000f, __u32)
+
+#define LIRC_SET_SEND_MODE_COMPAT32    _IOW('i', 0x00000011, __u32)
+#define LIRC_SET_REC_MODE_COMPAT32     _IOW('i', 0x00000012, __u32)
+
+long lirc_dev_fop_compat_ioctl(struct file *file,
+			       unsigned int cmd32,
+			       unsigned long arg)
+{
+	mm_segment_t old_fs;
+	int ret;
+	unsigned long val;
+	unsigned int cmd;
+
+	switch (cmd32) {
+	case LIRC_GET_FEATURES_COMPAT32:
+	case LIRC_GET_SEND_MODE_COMPAT32:
+	case LIRC_GET_REC_MODE_COMPAT32:
+	case LIRC_GET_LENGTH_COMPAT32:

drivers/media/IR/lirc_dev.c line 628

Quoted 10 lines

drivers/media/IR/lirc_dev.c new file +10 -0

+	case LIRC_SET_SEND_MODE_COMPAT32:
+	case LIRC_SET_REC_MODE_COMPAT32:
+		/*
+		 * These commands expect (unsigned long *) arg
+		 * but the 32-bit app supplied (__u32 *).
+		 * Conversion is required.
+		 */
+		if (get_user(val, (__u32 *)compat_ptr(arg)))
+			return -EFAULT;
+		lock_kernel();

Hmm... BKL? Are you sure you need it here? As BKL are being removed, please rewrite it to use another kind of lock.

Quoted 2 lines

On a first glance, I suspect that you should be locking &ir->irctl_lock inside lirc_dev_fop_ioctl() and just remove the BKL calls on lirc_dev_fop_compat_ioctl().

Ew, yeah, looks like there's some missing locking in lirc_dev_fop_ioctl(), though its never been a problem in practice, from what I've seen... Okay, will add locking there.

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

Quoted 2 lines

Btw, as this is the first in-tree kernel driver for lirc, I would just define the lirc ioctls with __u32 and remove the entire compat stuff.

I've pushed a patch into my ir-wip tree, ir branch, that does this:

http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/ir

I've tested it out, and things still work as expected w/my mceusb port and the ir-lirc-codec ir-core bridge driver, after rebuilding the lirc userspace (64-bit host) to match up the ioctl definitions.

Quoted 13 lines

include/media/lirc.h line 159

Quoted 9 lines

include/media/lirc.h new file +8 -0

+/*
+ * from the next key press on the driver will send
+ * LIRC_MODE2_FREQUENCY packets
+ */
+#define LIRC_MEASURE_CARRIER_ENABLE    _IO('i', 0x00000021)
+#define LIRC_MEASURE_CARRIER_DISABLE   _IO('i', 0x00000022)
+
+#endif

Wow! There are lots of ioctls that aren't currently used. IMO, the better is to add at the file just the ioctls that are currently in usage, and to put some documentation about them at /Documentation.

Several of the ioctls were only very recently (past 4 months) added, but I haven't a clue what they're actually used for... Adding something to Documentation/ would definitely be prudent in any case.

Em 03-06-2010 19:06, Jarod Wilson escreveu:

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

Em 03-06-2010 19:06, Jarod Wilson escreveu:

[...]

Quoted 13 lines

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 8 lines

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

I'm not against doing this change, but it has to be coordinated between drivers and user-space.
Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change.

And I'd also like to collect all other change request to the API if there are any and do all changes in one go.

From my side, as I said before, I'd like to see a documentation of the defined API bits,

From what I'm seeing, those are the current used ioctls:

Em 04-06-2010 12:51, Christoph Bartelmus escreveu:

Quoted 27 lines

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

Em 03-06-2010 19:06, Jarod Wilson escreveu:

[...]

Quoted 13 lines

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 8 lines

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

Quoted 6 lines

I'm not against doing this change, but it has to be coordinated between drivers and user-space.
Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change.

For sure coordination between kernel and userspace is very important. I'm sure that Jarod can help with this sync. Also, after having the changes implemented on userspace, I expect one patch from you adding the minimal lirc requirement at Documentation/Changes.

Quoted 3 lines

And I'd also like to collect all other change request to the API if there are any and do all changes in one go.

You and Jarod are the most indicated people to point for such needs. Also, Jon and David may have some comments.

From my side, as I said before, I'd like to see a documentation of the defined API bits, and the removal of the currently unused ioctls (they can be added later, together with the patches that will introduce the code that handles them) to give my final ack.

From what I'm seeing, those are the current used ioctls:

include/media/lirc.h new file +2 -0

+#define LIRC_GET_MIN_TIMEOUT           _IOR('i', 0x00000008, uint32_t)
+#define LIRC_GET_MAX_TIMEOUT           _IOR('i', 0x00000009, uint32_t)

There is also a defined LIRC_GET_REC_MODE that just returns a mask of GET_FEATURES bits, and a LIRC_SET_REC_MODE that do nothing.

I can't comment about the other ioctls, as currently there's no code using them, but it seems that some of them would be better implemented as ioctl, like the ones that send carrier/burst, etc.

One discussion that may be pertinent is if we should add ioctls for those controls, or if it would be better to just add sysfs nodes for them.

As all those ioctls are related to config stuff, IMO, using sysfs would be better, but I haven't a closed opinion about that matter.

Btw, a lirc userspace that would work with both the out-of-tree and in-tree lirc-dev can easily check for the proper sysfs nodes to know what version of the driver is being used. It will need access sysfs anyway, to enable the lirc decoder.

Cheers,
Mauro.

Quoted 35 lines · Mauro Carvalho Chehab

Em 04-06-2010 12:51, Christoph Bartelmus escreveu:

Quoted 27 lines

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

Em 03-06-2010 19:06, Jarod Wilson escreveu:

[...]

Quoted 13 lines

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 8 lines

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

Quoted 6 lines

I'm not against doing this change, but it has to be coordinated between drivers and user-space.
Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change.

Quoted 4 lines

For sure coordination between kernel and userspace is very important. I'm sure that Jarod can help with this sync. Also, after having the changes implemented on userspace, I expect one patch from you adding the minimal lirc requirement at Documentation/Changes.

Keep the get_version() ioctl stable. The first thing user space would do it call the get_version() ioctl. If user space doesn't support the kernel version error out and print a message with the url of the project web site and tell the user to upgrade.

If you have the ability to upgrade your kernel you should also have the ability to upgrade user space too. I'm no fan of keeping backwards compatibility code around. It just becomes piles of clutter that nobody maintains. A reliable mechanism to determine version mismatch is all that is needed.

Review

Quoted 4 lines

From what I'm seeing, those are the current used ioctls:

(quoted diff) +2 -0

+#define LIRC_GET_FEATURES              _IOR('i', 0x00000000, unsigned long)
+#define LIRC_GET_LENGTH                _IOR('i', 0x0000000f, unsigned long)

Has this been set into stone yet? if not a 64b word would be more future proof.

Checking the existence of features might be better as sysfs attributes. You can have as many sysfs attributes as you want.

Quoted 6 lines

(quoted diff) +2 -0

+#define LIRC_GET_MIN_TIMEOUT           _IOR('i', 0x00000008, uint32_t)
+#define LIRC_GET_MAX_TIMEOUT           _IOR('i', 0x00000009, uint32_t)

There is also a defined LIRC_GET_REC_MODE that just returns a mask of GET_FEATURES bits, and a LIRC_SET_REC_MODE that do nothing.

Quoted 3 lines

I can't comment about the other ioctls, as currently there's no code using them, but it seems that some of them would be better implemented as ioctl, like the ones that send carrier/burst, etc.

If the ioctls haven't been implemented, leave them as comments in the h file. Don't implement them with stubs. Stubs will set the parameters into stone and the parameters may be wrong.

Quoted 5 lines

One discussion that may be pertinent is if we should add ioctls for those controls, or if it would be better to just add sysfs nodes for them.

As all those ioctls are related to config stuff, IMO, using sysfs would be better, but I haven't a closed opinion about that matter.

In general sysfs is used to set options that are static or only change infrequently.

If the parameter is set on every request, use an IOCTL.

Quoted 3 lines

Btw, a lirc userspace that would work with both the out-of-tree and in-tree lirc-dev can easily check for the proper sysfs nodes to know what version of the driver is being used. It will need access sysfs anyway, to enable the lirc decoder.

On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:

Quoted 6 lines

Review

Quoted 4 lines

From what I'm seeing, those are the current used ioctls:

(quoted diff) +2 -0

+#define LIRC_GET_FEATURES              _IOR('i', 0x00000000, unsigned long)
+#define LIRC_GET_LENGTH                _IOR('i', 0x0000000f, unsigned long)

Has this been set into stone yet? if not a 64b word would be more future proof.

Nope, not set in stone at all, nothing has been merged. A patch I was carrying in Fedora changed all unsigned long to u64 and unsigned int to u32, and my current ir wip tree has all u32, but I don't see a reason why if we're going to make a change, it couldn't be to all u64, for as much future-proofing as possible.

Quoted 1 line · Jarod Wilson

On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:

...

Quoted 7 lines

Quoted 6 lines

Review

Quoted 4 lines

From what I'm seeing, those are the current used ioctls:

(quoted diff) +2 -0

+#define LIRC_GET_FEATURES              _IOR('i', 0x00000000, unsigned long)
+#define LIRC_GET_LENGTH                _IOR('i', 0x0000000f, unsigned long)

Has this been set into stone yet? if not a 64b word would be more future proof.

Quoted 5 lines

Nope, not set in stone at all, nothing has been merged. A patch I was carrying in Fedora changed all unsigned long to u64 and unsigned int to u32, and my current ir wip tree has all u32, but I don't see a reason why if we're going to make a change, it couldn't be to all u64, for as much future-proofing as possible.

Hrm, struct file_operations specifies an unsigned long for the ioctl args, so doesn't that mean we're pretty much stuck with only 32-bit for the ioctls?

Even with "only" 32 feature flags, I think we'd still be just fine, there appear to be only 15 feature flags at present, and I doubt many more features need to be added, given how long lirc has been around.

--
Jarod Wilson
[email protected]

Quoted 16 lines · Jarod Wilson

Quoted 1 line · Jarod Wilson

On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:

...

Quoted 12 lines

Quoted 6 lines

Review

Quoted 4 lines

From what I'm seeing, those are the current used ioctls:

(quoted diff) +2 -0

+#define LIRC_GET_FEATURES              _IOR('i', 0x00000000, unsigned long)
+#define LIRC_GET_LENGTH                _IOR('i', 0x0000000f, unsigned long)

Has this been set into stone yet? if not a 64b word would be more future proof.

Nope, not set in stone at all, nothing has been merged. A patch I was carrying in Fedora changed all unsigned long to u64 and unsigned int to u32, and my current ir wip tree has all u32, but I don't see a reason why if we're going to make a change, it couldn't be to all u64, for as much future-proofing as possible.

Quoted 3 lines

Hrm, struct file_operations specifies an unsigned long for the ioctl args, so doesn't that mean we're pretty much stuck with only 32-bit for the ioctls?

I haven't written an IOCTL in a while, but how would you pass a 64b memory address?

Quoted 19 lines · Jon Smirl

Quoted 18 lines · Jarod Wilson

Quoted 1 line · Jarod Wilson

On Fri, Jun 04, 2010 at 02:57:04PM -0400, Jon Smirl wrote:

...

Quoted 14 lines

Quoted 8 lines

Quoted 6 lines

From what I'm seeing, those are the current used ioctls:

+#define LIRC_GET_FEATURES _IOR('i', 0x00000000,
unsigned long)
+#define LIRC_GET_LENGTH _IOR('i', 0x0000000f,
unsigned long)

Has this been set into stone yet? if not a 64b word would be more future proof.

Nope, not set in stone at all, nothing has been merged. A patch I was carrying in Fedora changed all unsigned long to u64 and unsigned int to u32, and my current ir wip tree has all u32, but I don't see a reason why if we're going to make a change, it couldn't be to all u64, for as much future-proofing as possible.

Quoted 6 lines

Quoted 3 lines

Hrm, struct file_operations specifies an unsigned long for the ioctl args, so doesn't that mean we're pretty much stuck with only 32-bit for the ioctls?

I haven't written an IOCTL in a while, but how would you pass a 64b memory address?

Well, you wouldn't use struct file_operations' ioctl definition if you wanted to do so on a 32-bit host. :)

Its definitely possible using a different ioctl definition (see gdth_ioctl_free in drivers/scsi/gdth_proc.c, for example), but we're currently bound by what's there for file_operations.

Quoted 3 lines · Jarod Wilson

Hrm, struct file_operations specifies an unsigned long for the ioctl args, so doesn't that mean we're pretty much stuck with only 32-bit for the ioctls?

I haven't written an IOCTL in a while, but how would you pass a 64b memory address?

Quoted 35 lines · Mauro Carvalho Chehab

Em 04-06-2010 12:51, Christoph Bartelmus escreveu:

Quoted 27 lines

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

Em 03-06-2010 19:06, Jarod Wilson escreveu:

[...]

Quoted 13 lines

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 8 lines

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

Quoted 6 lines

I'm not against doing this change, but it has to be coordinated between drivers and user-space.
Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change.

Quoted 4 lines

For sure coordination between kernel and userspace is very important. I'm sure that Jarod can help with this sync. Also, after having the changes implemented on userspace, I expect one patch from you adding the minimal lirc requirement at Documentation/Changes.

Keep the get_version() ioctl stable. The first thing user space would do it call the get_version() ioctl. If user space doesn't support the kernel version error out and print a message with the url of the project web site and tell the user to upgrade.

If you have the ability to upgrade your kernel you should also have the ability to upgrade user space too. I'm no fan of keeping backwards compatibility code around. It just becomes piles of clutter that nobody maintains. A reliable mechanism to determine version mismatch is all that is needed.

PS - I really don't like having to fix bug reports in compatibility code. That is an incredible waste of support time that can be easily fixed by upgrading the user.

Quoted 13 lines

Quoted 3 lines

And I'd also like to collect all other change request to the API if there are any and do all changes in one go.

You and Jarod are the most indicated people to point for such needs. Also, Jon and David may have some comments.

From my side, as I said before, I'd like to see a documentation of the defined API bits, and the removal of the currently unused ioctls (they can be added later, together with the patches that will introduce the code that handles them) to give my final ack.

From what I'm seeing, those are the current used ioctls:

Review

Quoted 3 lines

(quoted diff) +2 -0

+#define LIRC_GET_FEATURES              _IOR('i', 0x00000000, unsigned long)
 
+#define LIRC_GET_LENGTH                _IOR('i', 0x0000000f, unsigned long)

Has this been set into stone yet? if not a 64b word would be more future proof. Checking the existence of features might be better as sysfs attributes. You can have as many sysfs attributes as you want.

Quoted 3 lines

(quoted diff) +2 -0

+#define LIRC_GET_MIN_TIMEOUT           _IOR('i', 0x00000008, uint32_t)
+#define LIRC_GET_MAX_TIMEOUT           _IOR('i', 0x00000009, uint32_t)

Quoted 6 lines

There is also a defined LIRC_GET_REC_MODE that just returns a mask of GET_FEATURES bits, and a LIRC_SET_REC_MODE that do nothing.

I can't comment about the other ioctls, as currently there's no code using them, but it seems that some of them would be better implemented as ioctl, like the ones that send carrier/burst, etc.

If the ioctls haven't been implemented, leave them as comments in the h file. Don't implement them with stubs. Stubs will set the parameters into stone and the parameters may be wrong.

Quoted 5 lines

One discussion that may be pertinent is if we should add ioctls for those controls, or if it would be better to just add sysfs nodes for them.

As all those ioctls are related to config stuff, IMO, using sysfs would be better, but I haven't a closed opinion about that matter.

In general sysfs is used to set options that are static or only change infrequently.

If the parameter is set on every request, use an IOCTL.

Quoted 81 lines · Jon Smirl

Quoted 35 lines · Mauro Carvalho Chehab

Em 04-06-2010 12:51, Christoph Bartelmus escreveu:

Quoted 27 lines

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

Em 03-06-2010 19:06, Jarod Wilson escreveu:

[...]

Quoted 13 lines

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 8 lines

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

Quoted 6 lines

I'm not against doing this change, but it has to be coordinated between drivers and user-space.
Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change.

Quoted 4 lines

For sure coordination between kernel and userspace is very important. I'm sure that Jarod can help with this sync. Also, after having the changes implemented on userspace, I expect one patch from you adding the minimal lirc requirement at Documentation/Changes.

Keep the get_version() ioctl stable. The first thing user space would do it call the get_version() ioctl. If user space doesn't support the kernel version error out and print a message with the url of the project web site and tell the user to upgrade.

If you have the ability to upgrade your kernel you should also have the ability to upgrade user space too. I'm no fan of keeping backwards compatibility code around. It just becomes piles of clutter that nobody maintains. A reliable mechanism to determine version mismatch is all that is needed.

PS - I really don't like having to fix bug reports in compatibility code. That is an incredible waste of support time that can be easily fixed by upgrading the user.

Quoted 13 lines

Quoted 3 lines

And I'd also like to collect all other change request to the API if there are any and do all changes in one go.

You and Jarod are the most indicated people to point for such needs. Also, Jon and David may have some comments.

From my side, as I said before, I'd like to see a documentation of the defined API bits, and the removal of the currently unused ioctls (they can be added later, together with the patches that will introduce the code that handles them) to give my final ack.

From what I'm seeing, those are the current used ioctls:

Review

Quoted 3 lines

(quoted diff) +2 -0

+#define LIRC_GET_FEATURES              _IOR('i', 0x00000000, unsigned long)
 
+#define LIRC_GET_LENGTH                _IOR('i', 0x0000000f, unsigned long)

Has this been set into stone yet? if not a 64b word would be more future proof. Checking the existence of features might be better as sysfs attributes. You can have as many sysfs attributes as you want.

Quoted 3 lines

(quoted diff) +2 -0

+#define LIRC_GET_MIN_TIMEOUT           _IOR('i', 0x00000008, uint32_t)
+#define LIRC_GET_MAX_TIMEOUT           _IOR('i', 0x00000009, uint32_t)

Quoted 6 lines

Quoted 6 lines

There is also a defined LIRC_GET_REC_MODE that just returns a mask of GET_FEATURES bits, and a LIRC_SET_REC_MODE that do nothing.

I can't comment about the other ioctls, as currently there's no code using them, but it seems that some of them would be better implemented as ioctl, like the ones that send carrier/burst, etc.

So I'll comment on the LIRC ioctl()'s that cx23885 driver/cx23888-ir.c implementation will support.  These ioctl()'s are from the lirc-0.8.7pre1 CVS snapshot I just downloaded.

LIRC_GET_FEATURES
The driver will support this.

LIRC_GET_SEND_MODE
LIRC_GET_REC_MODE
The driver will support these.  It will return hard-coded values, as the hardware and cx23888-ir.c only deal with raw pulse time values because there is no hardware decoding.

LIRC_SET_SEND_MODE
LIRC_SET_REC_MODE
The driver won't support these.  The CX23888 only supports 1 mode for Tx and 1 mode for Rx (namely raw pulses), but these ioctl()'s are marked obsolete anyway.

LIRC_GET_SEND_CARRIER
LIRC_GET_REC_CARRIER
The driver will support these.  It will return that actual value the hardware is working with.  For the CX23888, that may not be *exactly* what the user set, but instead the closest value the hardware could be set to, given what the user requested.  (e.g. 35,904 Hz when 36,000 Hz was requested).

LIRC_SET_SEND_CARRIER
The driver will support this.  Setting it is only valid when transmit modulation is enabled.  It will be set in hardware to a value as close as possible to the user's desired setting, but it is not guaranteed to be exact (e.g. 35,904 Hz when 36,000 Hz was requested)
If the driver is set for basedband transmit, then it is ignored.

LIRC_SET_REC_CARRIER
LIRC_SET_REC_CARRIER_RANGE
The driver will support these.  Setting them is only valid when receive demodulation is enabled.  They will be set in hardware to values as close as possible to the user's desired settings, but not guaranteed to be exact.  If setting REC_CARRIER, a default CARRIER_RANGE window will be set in the hardware.  If setting both, they really should be set atomically, as the possible window settings in hardware really depend on the desired receive carrier center freq.
If receive demodulation is disabled, the values are ignored.

LIRC_GET_SEND_DUTY_CYCLE
LIRC_SET_SEND_DUTY_CYCLE
The driver will support these.  The value returned for GET will be what the hardware is set to, given what the user requested.  That value may not be exactly what the user asked for.  The CX23888 hardware supports settings of n/16 * 100%  for n in [1,16].  Note, that setting a value of 100% is *not* the same as setting baseband transmit modulation as far as the hardware is concerned (although the physical effect may be the same).

LIRC_GET_REC_DUTY_CYCLE
LIRC_SET_REC_DUTY_CYCLE LIRC_SET_REC_DUTY_CYCLE_RANGE
The driver won't support these.  There is no hardware support.

LIRC_GET_REC_RESOLUTION
The driver will support this.  For CX23888 hardware, the value returned is not a constant, as the current pulse measurment resolution depends on how the hardware is set up.
The driver could also support this for Tx as well, but there is no lirc ioctl().

LIRC_GET_MIN_TIMEOUT
LIRC_GET_MAX_TIMEOUT
The driver will support this.  With Rx demodulation enabled, the value returned will be the same for both MIN and MAX, and it is a function of the selected carrier frequency.  With Rx demodulation disabled (baseband pulse reception), two distinct values can be returned: MIN_TIMEOUT would be the one that corresponds to settings for maximum measurement resolution of pulses, MAX_TIMEOUT corresponds to to settings for minimum measurment resolution of pulses.

LIRC_SET_REC_TIMEOUT
The driver will support this.  The current cx23888-ir.c code implements it as setting the maximum measurable pulse width when RX demodulation is disabled (baseband pulses being received.).  Note that this setting affects the Rx measurment resolution.  When Rx demodulation is enabled, the receive tiomeout is determined by the Rx carrier setting and this setting will be ignored.
Currently, since the cx23888-ir.c implementation is interrupt driven, the driver does not have code to support a setting of 0.  To me a setting of 0 implies polling the Rx hardware FIFO.

(I'll note the baseband Rx reception is not moot for some TV cards. Some cards with an CX23888, like the HVR-1850, only provide baseband Rx pulses to the CX23888 receive circuitry.  Apparently such cards have external components integrating the received carrier pulses into baseband pulses.  For these cards, the CX23888 only sees baseband pulses on receive and Rx demodulation must remain disabled in normal operation.)

LIRC_SET_REC_TIMEOUT_REPORTS
The driver will support this.  When the width measurment timer expires, a special measurement value is put on the Rx FIFO indicating a timeout (in band signaling).  The driver uses this internally and can pass the value along if requested.

LIRC_GET_MIN_FILTER_PULSE
LIRC_GET_MAX_FILTER_PULSE
The driver will support these.  The CX23888 has a hardware glith filter for pulses.  It also has a hardware setting for inverting polarity of the receive signal before the glitch filter, so the cx2888-ir.c will be able to ensure the glitch filer is always applied to pulses and never spaces.  The values returned for this are not constant but depend on the carrier frequency if Rx demodulation is enabled, or the pulse measurment resolution if Rx demodulation is disabled.

LIRC_GET_MIN_FILTER_SPACE
LIRC_GET_MAX_FILTER_SPACE
The driver for the CX23888 will not support these.  Older Conexant IR core implementations don't have hardware to invert incoming Rx signals, so it could be the case that the hardware glitch filter is applied to spaces instead of pulses depending on how the TV card is wired up. Polarity inversion is done in software after the glitch filter for those older chips.  There are no IR drivers currently in kernel for these older chips

LIRC_SET_REC_FILTER_PULSE
LIRC_SET_REC_FILTER
The driver will support these.  They will affect the pulse glitch filter.

LIRC_SET_REC_FILTER_SPACE
The driver will not support this.

LIRC_GET_LENGTH
The driver will not support this.

LIRC_SET_TRANSMITTER_MASK
The driver will not support this.  The CX23888 can support up to two output LEDs, but I doubt I'll ever see a video card wired up to do such. Also the current cx23888-ir.c doesn't have code to support it.

LIRC_SET_MEASURE_CARRIER_MODE
The driver will not support this.

LIRC_NOTIFY_DECODE
I'm not sure what this one is for.  If it means send up a notification when the hardware pulse measurment times out (so start decoding), then the driver can support it.

LIRC_SETUP_START
LIRC_SETUP_END
The driver will support these (I beleive I asked for them).  To change anything with the transmitter or receiver on the CX23888, the transmitter or receiver (respectively) needs to be disabled, have its interrupts masked, the setting made, the interrupts unmasked, and the unit reenabled.  These ioctl()'s would allow setting parameters in a batch, which is more efficient and probably smoother than operating with patial settings where one could possibly get interrupts unexpectedly.

What I don't see in the LIRC ioctl() set are:

1. ioctl()'s to enable/disable Tx modulation onto a carrier and Rx demodulation from a carrier.

2. an ioctl() to enable hardware loopback or other test modes.

I'm not sure if they would be useful beyond testing anyway.

Quoted 9 lines

If the ioctls haven't been implemented, leave them as comments in the h file. Don't implement them with stubs. Stubs will set the parameters into stone and the parameters may be wrong.

Quoted 5 lines

One discussion that may be pertinent is if we should add ioctls for those controls, or if it would be better to just add sysfs nodes for them.

As all those ioctls are related to config stuff, IMO, using sysfs would be better, but I haven't a closed opinion about that matter.

I personally don't like sysfs:

Cons of sysfs:
- it's linux specific  (Linux's own version of vendor lock-in: Where's the standards document?)
- the hierarchy can easily get too deep
- difficult to set multiple things atomically
- Oops'es from races with sysfs nodes that appears and disappear and user space has managed to open it before it disappears.  (firmware loading process is an example)

Pros of sysfs
- long term scalability for linux kernel configuration
- manipulated with human readable, and typeable, strings.

Maybe it's just that I've been playing with Unix systems since 1989 and am very used to ioctl().

However, in my experience, the best interfaces are the ones with the most complete documentation. :)

Quoted 4 lines

In general sysfs is used to set options that are static or only change infrequently.

If the parameter is set on every request, use an IOCTL.

That makes sense to me.

Regards,
Andy

Em 04-06-2010 12:51, Christoph Bartelmus escreveu:

Quoted 27 lines

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

Em 03-06-2010 19:06, Jarod Wilson escreveu:

[...]

Quoted 13 lines

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 8 lines

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

Quoted 6 lines

I'm not against doing this change, but it has to be coordinated between drivers and user-space.
Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change.

For sure coordination between kernel and userspace is very important. I'm sure that Jarod can help with this sync. Also, after having the changes implemented on userspace, I expect one patch from you adding the minimal lirc requirement at Documentation/Changes.

Quoted 2 lines

And I'd also like to collect all other change request to the API if there are any and do all changes in one go.

You and Jarod are the most indicated people to point for such needs. Also, Jon and David may have some comments.

Quoted 40 lines · Mauro Carvalho Chehab

Em 04-06-2010 12:51, Christoph Bartelmus escreveu:

Quoted 27 lines

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

Em 03-06-2010 19:06, Jarod Wilson escreveu:

[...]

Quoted 13 lines

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 8 lines

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

Quoted 6 lines

I'm not against doing this change, but it has to be coordinated between drivers and user-space.
Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change.

For sure coordination between kernel and userspace is very important. I'm sure that Jarod can help with this sync. Also, after having the changes implemented on userspace, I expect one patch from you adding the minimal lirc requirement at Documentation/Changes.

Quoted 5 lines

Quoted 2 lines

And I'd also like to collect all other change request to the API if there are any and do all changes in one go.

You and Jarod are the most indicated people to point for such needs. Also, Jon and David may have some comments.

David (who has been absent, sorry about that, life got in the way) thinks that the lirc raw decoder should implement the minimum amount of ioctl's possible while still being usable by the lirc userspace and without going beyond being a raw pulse/space "decoder" or we'll risk having to extend the entire decoder API just to support the lirc compatability decoder.

Over time, I hope that rc-core will grow it's own chardev with a clean set of ioctls (or sysfs controls, the jury hasn't returned a verdict yet). lircd can then be upgraded to support both the in-kernel native mode and the legacy lirc mode, and with time, the lirc raw decoder can be phased out.

Quoted 47 lines · David Härdeman

Quoted 40 lines · Mauro Carvalho Chehab

Em 04-06-2010 12:51, Christoph Bartelmus escreveu:

Quoted 27 lines

Hi Mauro,

Quoted 1 line · Mauro Carvalho Chehab

Em 03-06-2010 19:06, Jarod Wilson escreveu:

[...]

Quoted 13 lines

Quoted 5 lines

As for the compat bits... I actually pulled them out of the Fedora kernel and userspace for a while, and there were only a few people who really ran into issues with it, but I think if the new userspace and kernel are rolled out at the same time in a new distro release (i.e., Fedora 14, in our particular case), it should be mostly transparent to users.

For sure this will happen on all distros that follows upstream: they'll update lirc to fulfill the minimal requirement at Documentation/Changes.

The issue will appear only to people that manually compile kernel and lirc. Those users are likely smart enough to upgrade to a newer lirc version if they notice a trouble, and to check at the forums.

Quoted 8 lines

Quoted 4 lines

Christoph
wasn't a fan of the change, and actually asked me to revert it, so I'm cc'ing him here for further feedback, but I'm inclined to say that if this is the price we pay to get upstream, so be it.

I understand Christoph view, but I think that having to deal with compat stuff forever is a high price to pay, as the impact of this change is transitory and shouldn't be hard to deal with.

Quoted 6 lines

I'm not against doing this change, but it has to be coordinated between drivers and user-space.
Just changing lirc.h is not enough. You also have to change all user-space applications that use the affected ioctls to use the correct types. That's what Jarod did not address last time so I asked him to revert the change.

For sure coordination between kernel and userspace is very important. I'm sure that Jarod can help with this sync. Also, after having the changes implemented on userspace, I expect one patch from you adding the minimal lirc requirement at Documentation/Changes.

Quoted 5 lines

Quoted 2 lines

And I'd also like to collect all other change request to the API if there are any and do all changes in one go.

You and Jarod are the most indicated people to point for such needs. Also, Jon and David may have some comments.

Quoted 6 lines

David (who has been absent, sorry about that, life got in the way) thinks that the lirc raw decoder should implement the minimum amount of ioctl's possible while still being usable by the lirc userspace and without going beyond being a raw pulse/space "decoder" or we'll risk having to extend the entire decoder API just to support the lirc compatability decoder.

Thus far, I can get 100% feature-parity w/lirc_mceusb in my ir-core mceusb driver by adding only 3 tx-specific callbacks to ir_dev_props, and they're all callbacks (set output mask, set carrier and ir tx function) that any rc-core native tx solution is also going to need.

From my side, as I said before, I'd like to see a documentation of the defined API bits, and the removal of the currently unused ioctls (they can be added later, together with the patches that will introduce the code that handles them) to give my final ack.

This patch series adds the core lirc device interface, lirc_dev, and an ir-core bridge driver. Currently, only the receive side is wired up in the bridge driver, but adding transmit support is up next.

Currently, adding this code allows any raw IR ir-core device driver to pass raw IR out to the lirc userspace, without the driver having to have any actual knowledge of lirc -- its just feeding data to another IR protocol decoder engine.

This also (hopefully) makes life easier for any currently out-of-tree pure lirc device drivers, as they can count on the lirc core bits being present. This is a Good Thing(tm) while we work on porting additional lirc device drivers to ir-core, and also makes life easier for users to migrate from lirc decoding to in-kernel decoding (where possible) when their device's driver gets ported.

This patchset has been tested with the ir-core mceusb driver, and IR receive behaves 100% identical in lirc mode to the old lirc_mceusb.

[PATCH 1/3] IR: add core lirc device interface [PATCH 2/3] IR: add an empty lirc "protocol" keymap [PATCH 3/3] IR: add ir-core to lirc interface bridge driver

First ever crack at creating docbook documentation... Contains a bevy of information on the various lirc device interface ioctls, as well as a bit about the read and write interfaces.