On Fri, Mar 26, 2010 at 02:22:51PM -0300, Mauro Carvalho Chehab wrote:

Dmitry Torokhov wrote:

quoted

On Fri, Mar 26, 2010 at 11:40:41AM -0300, Mauro Carvalho Chehab wrote:

quoted

David Härdeman wrote:

quoted

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?

quoted

quoted

quoted

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