Message-ID: <20100326190741.GA8743@hardeman.nu>
Date:	Fri, 26 Mar 2010 20:07:41 +0100
From:	David Härdeman <david@...deman.nu>
To:	Mauro Carvalho Chehab <mchehab@...hat.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jon Smirl <jonsmirl@...il.com>, Pavel Machek <pavel@....cz>,
	Krzysztof Halasa <khc@...waw.pl>,
	hermann pitton <hermann-pitton@...or.de>,
	Christoph Bartelmus <lirc@...telmus.de>, awalls@...ix.net,
	j@...nau.net, jarod@...hat.com, jarod@...sonet.com,
	kraxel@...hat.com, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	superm1@...ntu.com
Subject: Re: [RFC] What are the goals for the architecture of an in-kernel
 IR system?

On Fri, Mar 26, 2010 at 02:22:51PM -0300, Mauro Carvalho Chehab wrote:
> Dmitry Torokhov wrote:
> > On Fri, Mar 26, 2010 at 11:40:41AM -0300, Mauro Carvalho Chehab wrote:
> >> David Härdeman wrote:
> >>> I'd suggest:
> >>>
> >>> struct keycode_table_entry {
> >>> 	unsigned keycode;
> >>> 	unsigned index;
> >>> 	unsigned len;
> >>> 	char scancode[];
> >>> };
> >>>
> >>> Use index in EVIOCGKEYCODEBIG to look up a keycode (all other fields are 
> >>> ignored), that way no special function to clear the table is necessary, 
> >>> instead you do a loop with:
> >>>
> >>> EVIOCGKEYCODEBIG (with index 0)
> >>> EVIOCSKEYCODEBIG (with the returned struct from EVIOCGKEYCODEBIG and
> >>> 		  keycode = KEY_RESERVED)
> >>>
> >>> until EVIOCGKEYCODEBIG returns an error.
> >> Makes sense.
> > 
> > Yes, I think so too. Just need a nice way to handle transition, I'd
> > like in the end to have drivers implement only the improved methods and
> > map legacy methods in evdev.
> 
> Ok. I'll prepare the patches for adding the new ioctl, in a way that it will
> also handle the legacy methods, and post for review.

If EVIOCGKEYCODEBIG is going to be used as a superset of the old ioctl, 
might it be a good idea change the proposed struct to:

struct keycode_table_entry {
	unsigned keycode;
	unsigned index;
	unsigned type;
	unsigned len;
	char scancode[];
};

Where "type" is used to give a hint of how the scancode[] member should 
be interpreted?

>>>> On a related note, I really think the interface would benefit from 
>>>> allowing more than one keytable per irrcv device with an input 
>>>> device created per keytable. That way you can have one input device 
>>>> per remote control. This implies that EVIOCLEARKEYCODEBIG is a bit 
>>>> misplaced as an evdev IOCTL since there's an N-1 mapping between 
>>>> input devices and irrcv devices.
>>> I don't think that an ioctl over one /dev/input/event should be the 
>>> proper way
>>> to ask kernel to create another filtered /dev/input/event. As it 
>>> were commented
>>> that the multimedia keys on some keyboards could benefit on having a 
>>> filter
>>> capability, maybe we may have a sysfs node at class input that would 
>>> allow
>>> the creation/removal of the filtered event interface.
>> 
>> No, if you want separate event devices just create a new instance 
>> of
>> input device for every keymap and have driver/irrcv class route 
>> events
>> to proper input device.

I fully agree!

> This don't solve the issue about how to signalize to kernel that more than one
> input device is needed. 
> 
> As the userspace will request the creation of those keymaps, we need some way
> to receive such requests from userspace. 
> 
> I can see a few ways for doing it:
> 
> 1) create a control device for the irrcv device as a hole,
> that would handle such requests via ioctl (/dev/irctl[0-9]* ?)
>
> 2) create a read/write sysfs node that would indicate the number of event/keymaps
> associated with a given IR. By writing a bigger number, it would create new devices.
> By writing a smaller number, it will delete some maps. There's an issue though: 
> what criteria would be used to delete? The newly created ones?

This won't work for the reason you've already set out...which keymap 
should be deleted?
 
> 3) create a fixed number of event devices, and add a sysfs attribute to enable
> or disable it;

You really seem to prefer sysfs over ioctls :)

> 4) create a fixed number of sysfs attributes to represent the keymaps. For example:
> /sys/class/irrcv/irrcv0/keymap0/enabled
> 	...
> /sys/class/irrcv/irrcv0/keymap7/enabled
> 
> The input/event node will be created only when the enabled=1.

This sounds like 3)

> I don't like (2) or (3), because removing a table with (2) may end by removing the wrong
> table, and (3) will create more event interfaces than probably needed by the majority
> of IR users.
> 
> maybe (4) is the better one.

Personally I think 1) is the best approach. Having a device for the 
irrcv device allows for three kinds of operations:

read
----
which corresponds to RX...you're eventually going to want to let 
userspace devices read IR commands which have no entries in a keytable 
yet in order to create keytables for new remotes, the same interface can 
also be used for lirc-type user-space apps which want to access the raw 
pulse/space timings for userspace decoding.

write
-----
which would correspond to TX...I'd suggest a stream of s32 integers to 
imply pulse/space timings.

ioctl
-----
for controlling the RX/TX parameters, creating/destroying additional 
keytables, etc...

Basically, we'll end up with a lirc_dev 2.0. And the "irrcv" class name 
will be misleading since it will be irrcv + irsnd :)

-- 
David Härdeman
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists