Re: [RFC] What are the goals for the architecture of an in-kernel IR system?

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Dmitry Torokhov wrote:
> On Sun, Dec 06, 2009 at 09:34:26PM +0100, Krzysztof Halasa wrote:
>> Jon Smirl <jonsmirl@gmail.com> writes:
>>
>>>> Once again: how about agreement about the LIRC interface
>>>> (kernel-userspace) and merging the actual LIRC code first? In-kernel
>>>> decoding can wait a bit, it doesn't change any kernel-user interface.
>>> I'd like to see a semi-complete design for an in-kernel IR system
>>> before anything is merged from any source.
>> This is a way to nowhere, there is no logical dependency between LIRC
>> and input layer IR.
>>
>> There is only one thing which needs attention before/when merging LIRC:
>> the LIRC user-kernel interface. In-kernel "IR system" is irrelevant and,
>> actually, making a correct IR core design without the LIRC merged can be
>> only harder.
> 
> This sounds like "merge first, think later"...
> 
> The question is why we need to merge lirc interface right now, before we
> agreed on the sybsystem architecture? Noone _in kernel_ user lirc-dev
> yet and, looking at the way things are shaping, no drivers will be
> _directly_ using it after it is complete. So, even if we merge it right
> away, the code will have to be restructured and reworked. Unfortunately,
> just merging what Jarod posted, will introduce sysfs hierarchy which
> is userspace interface as well (although we not as good maintaining it
> at times) and will add more constraints on us.
> 
> That is why I think we should go the other way around - introduce the
> core which receivers could plug into and decoder framework and once it
> is ready register lirc-dev as one of the available decoders.
> 

I've committed already some IR restruct code on my linux-next -git tree:

http://git.kernel.org/?p=linux/kernel/git/mchehab/linux-next.git

The code there basically moves the input/evdev registering code and 
scancode/keycode management code into a separate ir-core module.

To make my life easy, I've moved the code temporarily into drivers/media/IR.
This way, it helps me to move V4L specific code outside ir-core and to later
use it for DVB. After having it done, probably the better is to move it to
be under /drivers or /drivers/input.

The enclosed patch just adds a skeleton for the new sysfs class for remote
controllers and registers an yet unused ir_protocol attribute, creating this
tree:

/sys/class/irrcv/
|-- irrcv0
|   |-- ir_protocol
|   |-- power
|   |   `-- wakeup
|   |-- subsystem -> ../../irrcv
|   `-- uevent
`-- irrcv1
    |-- ir_protocol
    |-- power
    |   `-- wakeup
    |-- subsystem -> ../../irrcv
    `-- uevent

While writing the code, it occurred to me that calling it as "IR" is not the better way,
since there's nothing on the code that is related to infra-red, but, instead, is
is related to remote controller.

So, if it is ok for everybudy, IMO, we should use, instead "rc" meaning remote controller,
naming the core module as "rc-core", putting it into drivers/rc.

Also, since the same rc chip can have a receiver and a transmitter, maybe we can create the
class as:
	/sys/class/rc
		rcrcv0/
		rcrcv1/
		...
		rctx0/
		rctx1/
		...

Comments?


---
 linux/drivers/media/IR/Makefile      |    2 
 linux/drivers/media/IR/ir-keytable.c |   17 +++++-
 linux/drivers/media/IR/ir-sysfs.c    |   94 +++++++++++++++++++++++++++++++++++
 linux/include/media/ir-core.h        |   12 +++-
 4 files changed, 119 insertions(+), 6 deletions(-)

--- master.orig/linux/drivers/media/IR/Makefile
+++ master/linux/drivers/media/IR/Makefile
@@ -1,5 +1,5 @@
 ir-common-objs  := ir-functions.o ir-keymaps.o
-ir-core-objs	:= ir-keytable.o
+ir-core-objs	:= ir-keytable.o ir-sysfs.o
 
 obj-$(CONFIG_IR_CORE) += ir-core.o
 obj-$(CONFIG_VIDEO_IR) += ir-common.o
--- master.orig/linux/drivers/media/IR/ir-keytable.c
+++ master/linux/drivers/media/IR/ir-keytable.c
@@ -448,12 +448,21 @@ int ir_input_register(struct input_dev *
 	input_set_drvdata(input_dev, ir_dev);
 
 	rc = input_register_device(input_dev);
+	if (rc < 0)
+		goto err;
+
+	rc = ir_register_class(input_dev);
 	if (rc < 0) {
-		kfree(rc_tab->scan);
-		kfree(ir_dev);
-		input_set_drvdata(input_dev, NULL);
+		input_unregister_device(input_dev);
+		goto err;
 	}
 
+	return 0;
+
+err:
+	kfree(rc_tab->scan);
+	kfree(ir_dev);
+	input_set_drvdata(input_dev, NULL);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ir_input_register);
@@ -473,6 +482,8 @@ void ir_input_unregister(struct input_de
 	kfree(rc_tab->scan);
 	rc_tab->scan = NULL;
 
+	ir_unregister_class(dev);
+
 	kfree(ir_dev);
 	input_unregister_device(dev);
 }
--- /dev/null
+++ master/linux/drivers/media/IR/ir-sysfs.c
@@ -0,0 +1,94 @@
+/* ir-register.c - handle IR scancode->keycode tables
+ *
+ * Copyright (C) 2009 by Mauro Carvalho Chehab <mchehab@redhat.com>
+ *
+ * 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 version 2 of the License.
+ *
+ *  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.
+ */
+
+#include <linux/input.h>
+#include <linux/device.h>
+#include <media/ir-core.h>
+
+#define IRRCV_NUM_DEVICES	256
+
+unsigned long ir_core_dev_number;
+
+static struct class *ir_input_class;
+
+static DEVICE_ATTR(ir_protocol, S_IRUGO | S_IWUSR, NULL, NULL);
+
+static struct attribute *ir_dev_attrs[] = {
+	&dev_attr_ir_protocol.attr,
+};
+
+int ir_register_class(struct input_dev *input_dev)
+{
+	int rc;
+	struct kobject *kobj;
+
+	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	int devno = find_first_zero_bit(&ir_core_dev_number,
+				        IRRCV_NUM_DEVICES);
+
+	if (unlikely(devno < 0))
+		return devno;
+
+	ir_dev->attr.attrs = ir_dev_attrs;
+	ir_dev->class_dev = device_create(ir_input_class, NULL,
+					  input_dev->dev.devt, ir_dev,
+					  "irrcv%d", devno);
+	kobj= &ir_dev->class_dev->kobj;
+
+	printk(KERN_WARNING "Creating IR device %s\n", kobject_name(kobj));
+	rc = sysfs_create_group(kobj, &ir_dev->attr);
+	if (unlikely (rc < 0)) {
+		device_destroy(ir_input_class, input_dev->dev.devt);
+		return -ENOMEM;
+	}
+
+	ir_dev->devno = devno;
+	set_bit(devno, &ir_core_dev_number);
+
+	return 0;
+};
+
+void ir_unregister_class(struct input_dev *input_dev)
+{
+	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
+	struct kobject *kobj;
+
+	clear_bit(ir_dev->devno, &ir_core_dev_number);
+
+	kobj= &ir_dev->class_dev->kobj;
+
+	sysfs_remove_group(kobj, &ir_dev->attr);
+	device_destroy(ir_input_class, input_dev->dev.devt);
+
+	kfree(ir_dev->attr.name);
+}
+
+static int __init ir_core_init(void)
+{
+	ir_input_class = class_create(THIS_MODULE, "irrcv");
+	if (IS_ERR(ir_input_class)) {
+		printk(KERN_ERR "ir_core: unable to register irrcv class\n");
+		return PTR_ERR(ir_input_class);
+	}
+
+	return 0;
+}
+
+static void __exit ir_core_exit(void)
+{
+	class_destroy(ir_input_class);
+}
+
+module_init(ir_core_init);
+module_exit(ir_core_exit);
--- master.orig/linux/include/media/ir-core.h
+++ master/linux/include/media/ir-core.h
@@ -42,8 +42,11 @@ struct ir_scancode_table {
 };
 
 struct ir_input_dev {
-	struct input_dev		*dev;
-	struct ir_scancode_table	rc_tab;
+	struct input_dev		*dev;		/* Input device*/
+	struct ir_scancode_table	rc_tab;		/* scan/key table */
+	unsigned long			devno;		/* device number */
+	struct attribute_group		attr;		/* IR attributes */
+	struct device			*class_dev;	/* virtual class dev */
 };
 
 /* Routines from ir-keytable.c */
@@ -59,4 +62,9 @@ int ir_input_register(struct input_dev *
 		      struct ir_scancode_table *ir_codes);
 void ir_input_unregister(struct input_dev *input_dev);
 
+/* Routines from ir-sysfs.c */
+
+int ir_register_class(struct input_dev *input_dev);
+void ir_unregister_class(struct input_dev *input_dev);
+
 #endif