HID: Allow changing not-yet-mapped usages

HID: Allow changing not-yet-mapped usages

[Dmitry Torokhov]

Jiri,

Currently HID only allows re-mapping of usages that have already been
mapped by hid-input or one of the sub-drivers as keys. This,
unfortunately, leads to sub-drivers multiplying by the hour and many
of them only do initial setup of usages and waste memory once that is
done.

How about we also allow EVIOCSKEYCODE to establish mapping for
not-yet-unmapped usages (usage->type == 0)? Then we could offload the
task of setting up keymaps to udev.

This depends on the large keycode handling patches that are in my tree
in 'next' branch. Not tested past booting...

-- 
Dmitry

Input: hid-input - allow mapping unknown usages

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Currently HID layer only allows to remap keycodes for known usages,
and responds with -EINVAL when user tries to map new usage code.
This precludes us form relying on udev/keymap for establishing correct
mappings and forces us to write dummy HID drivers responsible only for
setting up keymaps.

Let's allow remapping not only usages that have been set up as keys
(usage->type == EV_KEY) but also yet-unmapped usages (usage->type == 0).

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/hid/hid-input.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)


diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index b12c07e..8dd17ce 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -77,7 +77,10 @@ static bool match_scancode(struct hid_usage *usage,
 static bool match_keycode(struct hid_usage *usage,
 			  unsigned int cur_idx, unsigned int keycode)
 {
-	return usage->code == keycode;
+	/*
+	 * We should exclude unmapped usages when doing lookup by keycode.
+	 */
+	return usage->type == EV_KEY && usage->code == keycode;
 }
 
 static bool match_index(struct hid_usage *usage,
@@ -103,7 +106,7 @@ static struct hid_usage *hidinput_find_key(struct hid_device *hid,
 			for (i = 0; i < report->maxfield; i++) {
 				for (j = 0; j < report->field[i]->maxusage; j++) {
 					usage = report->field[i]->usage + j;
-					if (usage->type == EV_KEY) {
+					if (usage->type == EV_KEY || usage->type == 0) {
 						if (match(usage, cur_idx, value)) {
 							if (usage_idx)
 								*usage_idx = cur_idx;
@@ -144,7 +147,8 @@ static int hidinput_getkeycode(struct input_dev *dev,
 
 	usage = hidinput_locate_usage(hid, ke, &index);
 	if (usage) {
-		ke->keycode = usage->code;
+		ke->keycode = usage->type == EV_KEY ?
+				usage->code : KEY_RESERVED;
 		ke->index = index;
 		scancode = usage->hid & (HID_USAGE_PAGE | HID_USAGE);
 		ke->len = sizeof(scancode);
@@ -164,7 +168,8 @@ static int hidinput_setkeycode(struct input_dev *dev,
 
 	usage = hidinput_locate_usage(hid, ke, NULL);
 	if (usage) {
-		*old_keycode = usage->code;
+		*old_keycode = usage->type == EV_KEY ?
+				usage->code : KEY_RESERVED;
 		usage->code = ke->keycode;
 
 		clear_bit(*old_keycode, dev->keybit);

Re: HID: Allow changing not-yet-mapped usages

[Jonathan Conder]

  On 15/09/10 16:58, Dmitry Torokhov wrote:
> Jiri,
>
> Currently HID only allows re-mapping of usages that have already been
> mapped by hid-input or one of the sub-drivers as keys. This,
> unfortunately, leads to sub-drivers multiplying by the hour and many
> of them only do initial setup of usages and waste memory once that is
> done.
>
> How about we also allow EVIOCSKEYCODE to establish mapping for
> not-yet-unmapped usages (usage->type == 0)? Then we could offload the
> task of setting up keymaps to udev.
>
> This depends on the large keycode handling patches that are in my tree
> in 'next' branch. Not tested past booting...
>
This is fine with me, if that matters. However, I'm not sure it could be 
a catch-all solution. For example, the five numbered keys on these 
Microsoft keyboards put the key number in the value field (rather than 
usage->hid). Maybe you could reduce the number of sub-drivers needed by 
using the vendor id alone for at least some of the quirks.

Jonathan

Re: HID: Allow changing not-yet-mapped usages

[Dmitry Torokhov]

On Wed, Sep 15, 2010 at 06:51:11PM +1200, Jonathan Conder wrote:
>  On 15/09/10 16:58, Dmitry Torokhov wrote:
> >Jiri,
> >
> >Currently HID only allows re-mapping of usages that have already been
> >mapped by hid-input or one of the sub-drivers as keys. This,
> >unfortunately, leads to sub-drivers multiplying by the hour and many
> >of them only do initial setup of usages and waste memory once that is
> >done.
> >
> >How about we also allow EVIOCSKEYCODE to establish mapping for
> >not-yet-unmapped usages (usage->type == 0)? Then we could offload the
> >task of setting up keymaps to udev.
> >
> >This depends on the large keycode handling patches that are in my tree
> >in 'next' branch. Not tested past booting...
> >
> This is fine with me, if that matters. However, I'm not sure it
> could be a catch-all solution. For example, the five numbered keys
> on these Microsoft keyboards put the key number in the value field
> (rather than usage->hid). Maybe you could reduce the number of
> sub-drivers needed by using the vendor id alone for at least some of
> the quirks.
> 

Yeah, it obviously won't cover every "interesting" way vendors come up
with... Still should cover some.

-- 
Dmitry

Re: HID: Allow changing not-yet-mapped usages

[Jarod Wilson]

On Tue, Sep 14, 2010 at 09:58:22PM -0700, Dmitry Torokhov wrote:
> Jiri,
> 
> Currently HID only allows re-mapping of usages that have already been
> mapped by hid-input or one of the sub-drivers as keys. This,
> unfortunately, leads to sub-drivers multiplying by the hour and many
> of them only do initial setup of usages and waste memory once that is
> done.
> 
> How about we also allow EVIOCSKEYCODE to establish mapping for
> not-yet-unmapped usages (usage->type == 0)? Then we could offload the
> task of setting up keymaps to udev.
> 
> This depends on the large keycode handling patches that are in my tree
> in 'next' branch. Not tested past booting...
> 
> -- 
> Dmitry
> 
> Input: hid-input - allow mapping unknown usages
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Currently HID layer only allows to remap keycodes for known usages,
> and responds with -EINVAL when user tries to map new usage code.
> This precludes us form relying on udev/keymap for establishing correct
> mappings and forces us to write dummy HID drivers responsible only for
> setting up keymaps.
> 
> Let's allow remapping not only usages that have been set up as keys
> (usage->type == EV_KEY) but also yet-unmapped usages (usage->type == 0).
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Seems like a very good idea to me, code looks sane.

Acked-by: Jarod Wilson <jarod@redhat.com>

-- 
Jarod Wilson
jarod@redhat.com

Re: HID: Allow changing not-yet-mapped usages

[Jiri Kosina]

On Tue, 14 Sep 2010, Dmitry Torokhov wrote:

> Jiri,
> 
> Currently HID only allows re-mapping of usages that have already been
> mapped by hid-input or one of the sub-drivers as keys. This,
> unfortunately, leads to sub-drivers multiplying by the hour and many
> of them only do initial setup of usages and waste memory once that is
> done.
> 
> How about we also allow EVIOCSKEYCODE to establish mapping for
> not-yet-unmapped usages (usage->type == 0)? Then we could offload the
> task of setting up keymaps to udev.
> 
> This depends on the large keycode handling patches that are in my tree
> in 'next' branch. Not tested past booting...

Hi Dmitry,

fully agreed. I would love to shovel all the special drivers which only 
establish mappings down to userspace.

> 
> -- 
> Dmitry
> 
> Input: hid-input - allow mapping unknown usages
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Currently HID layer only allows to remap keycodes for known usages,
> and responds with -EINVAL when user tries to map new usage code.
> This precludes us form relying on udev/keymap for establishing correct
> mappings and forces us to write dummy HID drivers responsible only for
> setting up keymaps.
> 
> Let's allow remapping not only usages that have been set up as keys
> (usage->type == EV_KEY) but also yet-unmapped usages (usage->type == 0).
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
> 
>  drivers/hid/hid-input.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index b12c07e..8dd17ce 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -77,7 +77,10 @@ static bool match_scancode(struct hid_usage *usage,
>  static bool match_keycode(struct hid_usage *usage,
>  			  unsigned int cur_idx, unsigned int keycode)
>  {
> -	return usage->code == keycode;
> +	/*
> +	 * We should exclude unmapped usages when doing lookup by keycode.
> +	 */
> +	return usage->type == EV_KEY && usage->code == keycode;

This is for some reason hurting my eyes. It'd seem much more readable to 
me if the condition would be enclosed in brackets, purely for sake for 
readability. What do you think?

>  }
>  
>  static bool match_index(struct hid_usage *usage,
> @@ -103,7 +106,7 @@ static struct hid_usage *hidinput_find_key(struct hid_device *hid,
>  			for (i = 0; i < report->maxfield; i++) {
>  				for (j = 0; j < report->field[i]->maxusage; j++) {
>  					usage = report->field[i]->usage + j;
> -					if (usage->type == EV_KEY) {
> +					if (usage->type == EV_KEY || usage->type == 0) {
>  						if (match(usage, cur_idx, value)) {
>  							if (usage_idx)
>  								*usage_idx = cur_idx;
> @@ -144,7 +147,8 @@ static int hidinput_getkeycode(struct input_dev *dev,
>  
>  	usage = hidinput_locate_usage(hid, ke, &index);
>  	if (usage) {
> -		ke->keycode = usage->code;
> +		ke->keycode = usage->type == EV_KEY ?
> +				usage->code : KEY_RESERVED;
>  		ke->index = index;
>  		scancode = usage->hid & (HID_USAGE_PAGE | HID_USAGE);
>  		ke->len = sizeof(scancode);
> @@ -164,7 +168,8 @@ static int hidinput_setkeycode(struct input_dev *dev,
>  
>  	usage = hidinput_locate_usage(hid, ke, NULL);
>  	if (usage) {
> -		*old_keycode = usage->code;
> +		*old_keycode = usage->type == EV_KEY ?
> +				usage->code : KEY_RESERVED;
>  		usage->code = ke->keycode;
>  
>  		clear_bit(*old_keycode, dev->keybit);

I guess you will be taking it through your tree together with all your 
keycode handling patches, right?

Thanks!

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: HID: Allow changing not-yet-mapped usages

[Dmitry Torokhov]

Hi Jiri,

On Wed, Sep 15, 2010 at 04:48:42PM +0200, Jiri Kosina wrote:
> > @@ -77,7 +77,10 @@ static bool match_scancode(struct hid_usage *usage,
> >  static bool match_keycode(struct hid_usage *usage,
> >  			  unsigned int cur_idx, unsigned int keycode)
> >  {
> > -	return usage->code == keycode;
> > +	/*
> > +	 * We should exclude unmapped usages when doing lookup by keycode.
> > +	 */
> > +	return usage->type == EV_KEY && usage->code == keycode;
> 
> This is for some reason hurting my eyes. It'd seem much more readable to 
> me if the condition would be enclosed in brackets, purely for sake for 
> readability. What do you think?
> 

Like this:

	return (usage->type == EV_KEY && usage->code == keycode);

?

We normally do not enclose return expression in parenthesis but why
not...

Alternatively we could code it as an "if" statement.

> >  }
> >  
> >  static bool match_index(struct hid_usage *usage,
> > @@ -103,7 +106,7 @@ static struct hid_usage *hidinput_find_key(struct hid_device *hid,
> >  			for (i = 0; i < report->maxfield; i++) {
> >  				for (j = 0; j < report->field[i]->maxusage; j++) {
> >  					usage = report->field[i]->usage + j;
> > -					if (usage->type == EV_KEY) {
> > +					if (usage->type == EV_KEY || usage->type == 0) {
> >  						if (match(usage, cur_idx, value)) {
> >  							if (usage_idx)
> >  								*usage_idx = cur_idx;
> > @@ -144,7 +147,8 @@ static int hidinput_getkeycode(struct input_dev *dev,
> >  
> >  	usage = hidinput_locate_usage(hid, ke, &index);
> >  	if (usage) {
> > -		ke->keycode = usage->code;
> > +		ke->keycode = usage->type == EV_KEY ?
> > +				usage->code : KEY_RESERVED;
> >  		ke->index = index;
> >  		scancode = usage->hid & (HID_USAGE_PAGE | HID_USAGE);
> >  		ke->len = sizeof(scancode);
> > @@ -164,7 +168,8 @@ static int hidinput_setkeycode(struct input_dev *dev,
> >  
> >  	usage = hidinput_locate_usage(hid, ke, NULL);
> >  	if (usage) {
> > -		*old_keycode = usage->code;
> > +		*old_keycode = usage->type == EV_KEY ?
> > +				usage->code : KEY_RESERVED;
> >  		usage->code = ke->keycode;
> >  
> >  		clear_bit(*old_keycode, dev->keybit);
> 
> I guess you will be taking it through your tree together with all your 
> keycode handling patches, right?
> 

Yes, as long as you are OK with it.

Thanks.

-- 
Dmitry

Re: HID: Allow changing not-yet-mapped usages

[Jiri Kosina]

On Wed, 15 Sep 2010, Dmitry Torokhov wrote:

> Hi Jiri,
> 
> On Wed, Sep 15, 2010 at 04:48:42PM +0200, Jiri Kosina wrote:
> > > @@ -77,7 +77,10 @@ static bool match_scancode(struct hid_usage *usage,
> > >  static bool match_keycode(struct hid_usage *usage,
> > >  			  unsigned int cur_idx, unsigned int keycode)
> > >  {
> > > -	return usage->code == keycode;
> > > +	/*
> > > +	 * We should exclude unmapped usages when doing lookup by keycode.
> > > +	 */
> > > +	return usage->type == EV_KEY && usage->code == keycode;
> > 
> > This is for some reason hurting my eyes. It'd seem much more readable to 
> > me if the condition would be enclosed in brackets, purely for sake for 
> > readability. What do you think?
> > 
> 
> Like this:
> 
> 	return (usage->type == EV_KEY && usage->code == keycode);
> 
> ?
> 
> We normally do not enclose return expression in parenthesis but why
> not...

Yeah, that's what I usually do in my code. But I don't mind too much 
either way, that was just really a minor cosmetic thing.

> Alternatively we could code it as an "if" statement.
> 
> > >  }
> > >  
> > >  static bool match_index(struct hid_usage *usage,
> > > @@ -103,7 +106,7 @@ static struct hid_usage *hidinput_find_key(struct hid_device *hid,
> > >  			for (i = 0; i < report->maxfield; i++) {
> > >  				for (j = 0; j < report->field[i]->maxusage; j++) {
> > >  					usage = report->field[i]->usage + j;
> > > -					if (usage->type == EV_KEY) {
> > > +					if (usage->type == EV_KEY || usage->type == 0) {
> > >  						if (match(usage, cur_idx, value)) {
> > >  							if (usage_idx)
> > >  								*usage_idx = cur_idx;
> > > @@ -144,7 +147,8 @@ static int hidinput_getkeycode(struct input_dev *dev,
> > >  
> > >  	usage = hidinput_locate_usage(hid, ke, &index);
> > >  	if (usage) {
> > > -		ke->keycode = usage->code;
> > > +		ke->keycode = usage->type == EV_KEY ?
> > > +				usage->code : KEY_RESERVED;
> > >  		ke->index = index;
> > >  		scancode = usage->hid & (HID_USAGE_PAGE | HID_USAGE);
> > >  		ke->len = sizeof(scancode);
> > > @@ -164,7 +168,8 @@ static int hidinput_setkeycode(struct input_dev *dev,
> > >  
> > >  	usage = hidinput_locate_usage(hid, ke, NULL);
> > >  	if (usage) {
> > > -		*old_keycode = usage->code;
> > > +		*old_keycode = usage->type == EV_KEY ?
> > > +				usage->code : KEY_RESERVED;
> > >  		usage->code = ke->keycode;
> > >  
> > >  		clear_bit(*old_keycode, dev->keybit);
> > 
> > I guess you will be taking it through your tree together with all your 
> > keycode handling patches, right?
> 
> Yes, as long as you are OK with it.

Absolutely. Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.