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

[Srinivas KANDAGATLA]

This is a multi-part message in MIME format.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 <srinivas.kandagatla@xxxxxx>
>>
>> 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