[RFC] media: lirc: Allow lirc dev to talk to rc device

Commit Message

Srinivas KANDAGATLA July 15, 2013, 9:25 a.m. UTC

Thanks Sean for the comments,
On 12/07/13 13:46, Sean Young wrote:
> On Fri, Jul 12, 2013 at 09:55:28AM +0100, Srinivas KANDAGATLA wrote:
>> From: Srinivas Kandagatla <[e-mail]>
>>
>> The use case is simple, if any rc device has allowed protocols =
>> RC_TYPE_LIRC and map_name = RC_MAP_LIRC set, the driver open will be never
>> called. The reason for this is, all of the key maps except lirc have some
>> KEYS in there map, so during rc_register_device process these keys are
>> matched against the input drivers and open is performed, so for the case
>> of RC_MAP_EMPTY, a tty/vt/keyboard is matched and the driver open is
>> performed.
>>
>> In case of lirc, there is no match and result is that there is no open
>> performed, however the lirc-dev will go ahead and create a /dev/lirc0
>> node. Now when lircd/mode2 opens this device, no data is available
>> because the driver was never opened.
> 
> The rc device gets opened via the input interface. If the input device is
> never opened (e.g. embedded with no console) then the rc open is never 
> called and lirc will not work either. So that's another case.

Yes, this might be another case to add to the list.

>> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
>> index e456126..d5ad27f 100644

>> +	rc_dev = ir->d.rc_dev;
>> +	if (rc_dev && rc_dev->open) {
>> +		retval = rc_dev->open(rc_dev);
>> +		if (retval)
>> +			goto error;
>> +	}
>> +
> 
> Now the rc device can have its open called twice; once via the input 
> system and then (while it is already opened) via lirc. The rc drivers do
> not expect this.
I did think about this, and I thought managing this would be much easy
in the actual driver itself.

But given the generic nature, add this logic to rc infrastructure makes
much sense,
What I have done is added a users to rc_dev which will track howmany
users are actively using this device.
Here is the new patch, with the users count.
> 
>>  	cdev = ir->cdev;
>>  	if (try_module_get(cdev->owner)) {
>>  		ir->open++;
>> @@ -499,6 +508,7 @@ int lirc_dev_fop_close(struct inode *inode, struct file *file)
>>  {
>>  	struct irctl *ir = irctls[iminor(inode)];
>>  	struct cdev *cdev;
>> +	struct rc_dev *rc_dev;
>>  
>>  	if (!ir) {
>>  		printk(KERN_ERR "%s: called with invalid irctl\n", __func__);
>> @@ -511,6 +521,10 @@ int lirc_dev_fop_close(struct inode *inode, struct file *file)
>>  
>>  	WARN_ON(mutex_lock_killable(&lirc_dev_lock));
>>  
>> +	rc_dev = ir->d.rc_dev;
>> +	if (rc_dev && rc_dev->close)
>> +		rc_dev->close(rc_dev);
>> +
> 
> iguanair, nuvoton and ene_ir will disable the ir receiver when their close
> function is called. If the device was also opened via the input interface,
> the input interface will receive no new ir activity.
> 
> I think there should be some sort of (atomic) use counter so that the 
> rc device open or close only gets called once, whether opened via the 
> input interface or via lirc. 
new patch should address this using a users a use counter.

Thanks,
srini

Patch

From 06435a6ebd2374b500fbd0037e16a0451668f193 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <[e-mail]>
Date: Fri, 12 Jul 2013 09:07:37 +0100
Subject: [PATCH RFC] media: lirc: Allow lirc dev to talk to rc device

The use case is simple, if any rc device has allowed protocols =
RC_TYPE_LIRC and map_name = RC_MAP_LIRC set, the driver open will be never
called. The reason for this is, all of the key maps except lirc have some
KEYS in there map, so during rc_register_device process these keys are
matched against the input drivers and open is performed, so for the case
of RC_MAP_EMPTY, a vt/keyboard is matched and the driver open is
performed.

In case of lirc, there is no match and result is that there is no open
performed, however the lirc-dev will go ahead and create a /dev/lirc0
node. Now when lircd/mode2 opens this device, no data is available
because the driver was never opened.

lirc_dev seems to have no link with actual rc device w.r.t open/close.
This patch adds rc_dev pointer to lirc_driver structure for cases like
this, so that it can do the open/close of the real driver in accordance
to lircd/mode2 open/close.

Without this patch its impossible to open a rc device which has
RC_TYPE_LIRC ad RC_MAP_LIRC set.

Signed-off-by: Srinivas Kandagatla <[e-mail]>
---
 drivers/media/rc/ir-lirc-codec.c |    1 +
 drivers/media/rc/lirc_dev.c      |   16 ++++++++++++++++
 drivers/media/rc/rc-main.c       |   11 +++++++++--
 include/media/lirc_dev.h         |    1 +
 include/media/rc-core.h          |    1 +
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index e456126..d5ad27f 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -375,6 +375,7 @@  static int ir_lirc_register(struct rc_dev *dev)
 	drv->code_length = sizeof(struct ir_raw_event) * 8;
 	drv->fops = &lirc_fops;
 	drv->dev = &dev->dev;
+	drv->rc_dev = dev;
 	drv->owner = THIS_MODULE;
 
 	drv->minor = lirc_register_driver(drv);
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index 8dc057b..249e6ab 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -35,6 +35,7 @@ 
 #include <linux/device.h>
 #include <linux/cdev.h>
 
+#include <media/rc-core.h>
 #include <media/lirc.h>
 #include <media/lirc_dev.h>
 
@@ -437,6 +438,7 @@  EXPORT_SYMBOL(lirc_unregister_driver);
 int lirc_dev_fop_open(struct inode *inode, struct file *file)
 {
 	struct irctl *ir;
+	struct rc_dev *rc_dev;
 	struct cdev *cdev;
 	int retval = 0;
 
@@ -467,6 +469,15 @@  int lirc_dev_fop_open(struct inode *inode, struct file *file)
 		goto error;
 	}
 
+	rc_dev = ir->d.rc_dev;
+	if (!rc_dev->users++ && rc_dev && rc_dev->open) {
+		retval = rc_dev->open(rc_dev);
+		if (retval) {
+			rc_dev->users--;
+			goto error;
+		}
+	}
+
 	cdev = ir->cdev;
 	if (try_module_get(cdev->owner)) {
 		ir->open++;
@@ -499,6 +510,7 @@  int lirc_dev_fop_close(struct inode *inode, struct file *file)
 {
 	struct irctl *ir = irctls[iminor(inode)];
 	struct cdev *cdev;
+	struct rc_dev *rc_dev;
 
 	if (!ir) {
 		printk(KERN_ERR "%s: called with invalid irctl\n", __func__);
@@ -511,6 +523,10 @@  int lirc_dev_fop_close(struct inode *inode, struct file *file)
 
 	WARN_ON(mutex_lock_killable(&lirc_dev_lock));
 
+	rc_dev = ir->d.rc_dev;
+	if (rc_dev && !--rc_dev->users && rc_dev->close)
+		rc_dev->close(rc_dev);
+
 	ir->open--;
 	if (ir->attached) {
 		ir->d.set_use_dec(ir->d.data);
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 1cf382a..e800b96 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -702,15 +702,22 @@  EXPORT_SYMBOL_GPL(rc_keydown_notimeout);
 static int ir_open(struct input_dev *idev)
 {
 	struct rc_dev *rdev = input_get_drvdata(idev);
+	int rval = 0;
 
-	return rdev->open(rdev);
+	if (!rdev->users++)
+		rval = rdev->open(rdev);
+
+	if (rval)
+		rdev->users--;
+
+	return rval;
 }
 
 static void ir_close(struct input_dev *idev)
 {
 	struct rc_dev *rdev = input_get_drvdata(idev);
 
-	 if (rdev)
+	 if (rdev && !--rdev->users)
 		rdev->close(rdev);
 }
 
diff --git a/include/media/lirc_dev.h b/include/media/lirc_dev.h
index 168dd0b..96dccb6 100644
--- a/include/media/lirc_dev.h
+++ b/include/media/lirc_dev.h
@@ -139,6 +139,7 @@  struct lirc_driver {
 	struct lirc_buffer *rbuf;
 	int (*set_use_inc) (void *data);
 	void (*set_use_dec) (void *data);
+	struct rc_dev *rc_dev;
 	const struct file_operations *fops;
 	struct device *dev;
 	struct module *owner;
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 06a75de..b42016a 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -101,6 +101,7 @@  struct rc_dev {
 	bool				idle;
 	u64				allowed_protos;
 	u64				enabled_protocols;
+	u32				users;
 	u32				scanmask;
 	void				*priv;
 	spinlock_t			keylock;
-- 
1.7.6.5