V4L/DVB: ir: Add a link to associate /sys/class/ir/irrcv with the input device

In order to allow userspace programs to autoload an IR table, a link is needed to point to the corresponding input device.

$ tree /sys/class/irrcv/irrcv0
/sys/class/irrcv/irrcv0
|-- current_protocol
|-- input -> ../../../pci0000:00/0000:00:0b.1/usb1/1-3/input/input22
|-- power
|   `-- wakeup
|-- subsystem -> ../../../../class/irrcv
`-- uevent

It is now easy to associate an irrcv device with the corresponding device node, at the input interface.

index bf5fbcd..7de32e7 100644
@@ -138,6 +138,7 @@ int ir_register_class(struct input_dev *input_dev)
 {
 	int rc;
 	struct kobject *kobj;
+	const char *path;
 
 	struct ir_input_dev *ir_dev = input_get_drvdata(input_dev);
 	int devno = find_first_zero_bit(&ir_core_dev_number,

@@ -152,13 +153,26 @@ int ir_register_class(struct input_dev *input_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;
 	}
 
+	rc = sysfs_create_link(kobj, &input_dev->dev.kobj, "input");
+	if (unlikely(rc < 0)) {
+		sysfs_remove_group(kobj, &ir_dev->attr);
+		device_destroy(ir_input_class, input_dev->dev.devt);
+		return -ENOMEM;
+	}
+
+	path = kobject_get_path(&input_dev->dev.kobj, GFP_KERNEL);
+	printk(KERN_INFO "%s: %s associated with sysfs %s\n",
+		kobject_name(kobj),
+		input_dev->name ? input_dev->name : "Unspecified device",
+		path ? path : "N/A");
+	kfree(path);
+
 	ir_dev->devno = devno;
 	set_bit(devno, &ir_core_dev_number);
 

@@ -181,6 +195,8 @@ void ir_unregister_class(struct input_dev *input_dev)
 
 	kobj = &ir_dev->class_dev->kobj;
 
+	sysfs_remove_link(kobj, "input");
+
 	sysfs_remove_group(kobj, &ir_dev->attr);
 	device_destroy(ir_input_class, input_dev->dev.devt);
 

1.6.6.1

Hi Mauro,

I guess the question is why don't you make input device a child of your irrcvX device? Then I believe driver core will link them properly. It will also ensure proper power management hierarchy.

That probably will require you changing from class_dev into device but that's the direction kernel is going to anyway.

Thanks.

Dmitry

Dmitry,

I remember you asked me to create a separate class for IR. The current code does it, using class_create(). Once the class is created, I'm using device_create() to create the nodes.

The current code is:

int ir_register_class(struct input_dev *input_dev)

{
	...
	ir_dev->class_dev = device_create(ir_input_class, NULL,
					  input_dev->dev.devt, ir_dev,
					  "irrcv%d", devno);
	...
	rc = sysfs_create_group(kobj, &ir_dev->attr);
	...
	rc = sysfs_create_link(kobj, &input_dev->dev.kobj, "input");
	...
	return 0;
};

int ir_input_register(struct input_dev *input_dev,
const struct ir_scancode_table *rc_tab, const struct ir_dev_props *props)

{
	...
	rc = input_register_device(input_dev);
	...
	rc = ir_register_class(input_dev);
	...
}

static int __init ir_core_init(void)

{
	ir_input_class = class_create(THIS_MODULE, "irrcv");
	...
}

I couldn't find any other way to create the link without explicitly calling sysfs_create_link().

Do you have a better idea?

Cheers,
Mauro

Done, see enclosed. It is now using class_register/device_register. The newly created device for irrcv is used as the parent for input_dev->dev.

The resulting code looked cleaner after the change ;)

Cheers,
Mauro

---

V4L/DVB: ir: use a real device instead of a virtual class

Change the ir-sysfs approach to create irrcv0 as a device, instead of using class_dev. Also, change the way input is registered, in order to make its parent to be the irrcv device.

Due to this change, now the event device is created under /sys/class/ir/irrcv class:

/sys/class/irrcv/irrcv0/

|-- current_protocol
|-- device -> ../../../1-3
|-- input9
|   |-- capabilities
|   |   |-- abs

...

index 0903f53..c9c0a54 100644
@@ -448,22 +448,15 @@ int ir_input_register(struct input_dev *input_dev,
 	input_dev->setkeycode = ir_setkeycode;
 	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) {
-		input_unregister_device(input_dev);
+	if (rc < 0)
 		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);

@@ -492,7 +485,6 @@ void ir_input_unregister(struct input_dev *dev)
 	ir_unregister_class(dev);
 
 	kfree(ir_dev);
-	input_unregister_device(dev);
 }
 EXPORT_SYMBOL_GPL(ir_input_unregister);
 

index 61c223b..ce9f347 100644
@@ -47,11 +47,9 @@ struct ir_dev_props {
 
 
 struct ir_input_dev {
-	struct input_dev		*dev;		/* Input device*/
+	struct device			dev;		/* 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 */
 	const struct ir_dev_props	*props;		/* Device properties */
 };
 #define to_ir_input_dev(_attr) container_of(_attr, struct ir_input_dev, attr)

It is indeed better, however I wonder if current hierarchy expresses the hardware in best way. You currently have irrcv devices grow in parallel with input devices whereas I would expect input devices be children of irrcv devices:

	parent (PCI board, USB) -> irrcvX -> input1
                                          -> input2
					 ...

This way your PM sequence as follows - input core does its thing and releases all pressed keys, etc, then you can shut off the receiver and then board driver can shut doen the main piece. Otherwise irrcv0 suspend may be racing with input suspend and so forth.

Thanks.

Dmitry

It is representing it right:

usb1/1-3 -> irrcv -> irrcv0 -> input7 -> event7

The only extra attribute there is the class name "irrcv", but this seems coherent with the other classes on this device (dvb, sound, power, video4linux).

The created tree is:

$ tree /sys/class/irrcv/
/sys/class/irrcv/
`-- irrcv0 -> ../../devices/pci0000:00/0000:00:0b.1/usb1/1-3/irrcv/irrcv0

$ tree /sys/devices/pci0000:00/0000:00:0b.1/usb1/1-3/
/sys/devices/pci0000:00/0000:00:0b.1/usb1/1-3/
|-- 1-3:1.0
|   |-- bAlternateSetting
|   |-- bInterfaceClass
|   |-- bInterfaceNumber
|   |-- bInterfaceProtocol
|   |-- bInterfaceSubClass
|   |-- bNumEndpoints
|   |-- driver -> ../../../../../../bus/usb/drivers/em28xx
|   |-- ep_81
|   |   |-- bEndpointAddress
|   |   |-- bInterval
|   |   |-- bLength
|   |   |-- bmAttributes
|   |   |-- direction
|   |   |-- interval
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- type
|   |   |-- uevent
|   |   `-- wMaxPacketSize
|   |-- ep_82
|   |   |-- bEndpointAddress
|   |   |-- bInterval
|   |   |-- bLength
|   |   |-- bmAttributes
|   |   |-- direction
|   |   |-- interval
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- type
|   |   |-- uevent
|   |   `-- wMaxPacketSize
|   |-- ep_83
|   |   |-- bEndpointAddress
|   |   |-- bInterval
|   |   |-- bLength
|   |   |-- bmAttributes
|   |   |-- direction
|   |   |-- interval
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- type
|   |   |-- uevent
|   |   `-- wMaxPacketSize
|   |-- ep_84
|   |   |-- bEndpointAddress
|   |   |-- bInterval
|   |   |-- bLength
|   |   |-- bmAttributes
|   |   |-- direction
|   |   |-- interval
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- type
|   |   |-- uevent
|   |   `-- wMaxPacketSize
|   |-- modalias
|   |-- power
|   |   `-- wakeup
|   |-- subsystem -> ../../../../../../bus/usb
|   |-- supports_autosuspend
|   |-- uevent
|   `-- video4linux
|       |-- vbi2
|       |   |-- dev
|       |   |-- device -> ../../../1-3:1.0
|       |   |-- index
|       |   |-- name
|       |   |-- power
|       |   |   `-- wakeup
|       |   |-- subsystem -> ../../../../../../../../class/video4linux
|       |   `-- uevent
|       `-- video2
|           |-- dev
|           |-- device -> ../../../1-3:1.0
|           |-- index
|           |-- name
|           |-- power
|           |   `-- wakeup
|           |-- subsystem -> ../../../../../../../../class/video4linux
|           `-- uevent
|-- authorized
|-- bcdDevice
|-- bConfigurationValue
|-- bDeviceClass
|-- bDeviceProtocol
|-- bDeviceSubClass
|-- bmAttributes
|-- bMaxPacketSize0
|-- bMaxPower
|-- bNumConfigurations
|-- bNumInterfaces
|-- busnum
|-- configuration
|-- descriptors
|-- dev
|-- devnum
|-- devpath
|-- driver -> ../../../../../bus/usb/drivers/usb
|-- dvb
|   |-- dvb0.demux0
|   |   |-- dev
|   |   |-- device -> ../../../1-3
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- subsystem -> ../../../../../../../class/dvb
|   |   `-- uevent
|   |-- dvb0.dvr0
|   |   |-- dev
|   |   |-- device -> ../../../1-3
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- subsystem -> ../../../../../../../class/dvb
|   |   `-- uevent
|   |-- dvb0.frontend0
|   |   |-- dev
|   |   |-- device -> ../../../1-3
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- subsystem -> ../../../../../../../class/dvb
|   |   `-- uevent
|   `-- dvb0.net0
|       |-- dev
|       |-- device -> ../../../1-3
|       |-- power
|       |   `-- wakeup
|       |-- subsystem -> ../../../../../../../class/dvb
|       `-- uevent
|-- ep_00
|   |-- bEndpointAddress
|   |-- bInterval
|   |-- bLength
|   |-- bmAttributes
|   |-- direction
|   |-- interval
|   |-- power
|   |   `-- wakeup
|   |-- type
|   |-- uevent
|   `-- wMaxPacketSize
|-- i2c-3
|   |-- 3-005c
|   |   |-- driver -> ../../../../../../../bus/i2c/drivers/tvp5150
|   |   |-- modalias
|   |   |-- name
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- subsystem -> ../../../../../../../bus/i2c
|   |   `-- uevent
|   |-- 3-0061
|   |   |-- driver -> ../../../../../../../bus/i2c/drivers/tuner
|   |   |-- modalias
|   |   |-- name
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- subsystem -> ../../../../../../../bus/i2c
|   |   `-- uevent
|   |-- delete_device
|   |-- device -> ../../1-3
|   |-- name
|   |-- new_device
|   |-- power
|   |   `-- wakeup
|   |-- subsystem -> ../../../../../../bus/i2c
|   `-- uevent
|-- idProduct
|-- idVendor
|-- irrcv
|   `-- irrcv0
|       |-- current_protocol
|       |-- device -> ../../../1-3
|       |-- input7
|       |   |-- capabilities
|       |   |   |-- abs
|       |   |   |-- ev
|       |   |   |-- ff
|       |   |   |-- key
|       |   |   |-- led
|       |   |   |-- msc
|       |   |   |-- rel
|       |   |   |-- snd
|       |   |   `-- sw
|       |   |-- device -> ../../irrcv0
|       |   |-- event7
|       |   |   |-- dev
|       |   |   |-- device -> ../../input7
|       |   |   |-- power
|       |   |   |   `-- wakeup
|       |   |   |-- subsystem -> ../../../../../../../../../class/input
|       |   |   `-- uevent
|       |   |-- id
|       |   |   |-- bustype
|       |   |   |-- product
|       |   |   |-- vendor
|       |   |   `-- version
|       |   |-- modalias
|       |   |-- name
|       |   |-- phys
|       |   |-- power
|       |   |   `-- wakeup
|       |   |-- subsystem -> ../../../../../../../../class/input
|       |   |-- uevent
|       |   `-- uniq
|       |-- power
|       |   `-- wakeup
|       |-- subsystem -> ../../../../../../../class/irrcv
|       `-- uevent
|-- maxchild
|-- power
|   |-- active_duration
|   |-- autosuspend
|   |-- connected_duration
|   |-- level
|   |-- persist
|   `-- wakeup
|-- product
|-- quirks
|-- remove
|-- serial
|-- sound
|   `-- card1
|       |-- controlC1
|       |   |-- dev
|       |   |-- device -> ../../card1
|       |   |-- power
|       |   |   `-- wakeup
|       |   |-- subsystem -> ../../../../../../../../class/sound
|       |   `-- uevent
|       |-- device -> ../../../1-3
|       |-- id
|       |-- number
|       |-- pcmC1D0c
|       |   |-- dev
|       |   |-- device -> ../../card1
|       |   |-- pcm_class
|       |   |-- power
|       |   |   `-- wakeup
|       |   |-- subsystem -> ../../../../../../../../class/sound
|       |   `-- uevent
|       |-- power
|       |   `-- wakeup
|       |-- subsystem -> ../../../../../../../class/sound
|       `-- uevent
|-- speed
|-- subsystem -> ../../../../../bus/usb
|-- uevent
|-- urbnum
`-- version

$ tree /sys/class/irrcv/irrcv0/
/sys/class/irrcv/irrcv0/
|-- current_protocol
|-- device -> ../../../1-3
|-- input7
|   |-- capabilities
|   |   |-- abs
|   |   |-- ev
|   |   |-- ff
|   |   |-- key
|   |   |-- led
|   |   |-- msc
|   |   |-- rel
|   |   |-- snd
|   |   `-- sw
|   |-- device -> ../../irrcv0
|   |-- event7
|   |   |-- dev
|   |   |-- device -> ../../input7
|   |   |-- power
|   |   |   `-- wakeup
|   |   |-- subsystem -> ../../../../../../../../../class/input
|   |   `-- uevent
|   |-- id
|   |   |-- bustype
|   |   |-- product
|   |   |-- vendor
|   |   `-- version
|   |-- modalias
|   |-- name
|   |-- phys
|   |-- power
|   |   `-- wakeup
|   |-- subsystem -> ../../../../../../../../class/input
|   |-- uevent
|   `-- uniq
|-- power
|   `-- wakeup
|-- subsystem -> ../../../../../../../class/irrcv
`-- uevent

13 directories, 25 files

Cheers,
Mauro

Ah, yes, I saw where you take input device's parent and use it as irrcv's parent but missed the piece where you substitute original input device's parent with irrcv. I bit sneaky to my taste but I guess can be cleaned up later.

Dmitry

Yes. I opted to do this little trick to avoid changing the interfaces on the other modules, and to minimize the needs when porting from input. We can later clean it if/when needed.

Cheers,
Mauro