Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Thu, Oct 10, 2019 at 8:24 PM Benjamin Tissoires
wrote:
>
> On Wed, Oct 9, 2019 at 2:54 PM Candle Sun wrote:
> >
> > From: Candle Sun
> >
> > Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> > to Main item") adds support for Usage Page item after Usage ID items
> > (such as keyboards manufactured by Primax).
> >
> > Usage Page concatenation in Main item works well for following report
> > descriptor patterns:
> >
> > USAGE_PAGE (Keyboard) 05 07
> > USAGE_MINIMUM (Keyboard LeftControl)19 E0
> > USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
> > LOGICAL_MINIMUM (0) 15 00
> > LOGICAL_MAXIMUM (1) 25 01
> > REPORT_SIZE (1) 75 01
> > REPORT_COUNT (8)95 08
> > INPUT (Data,Var,Abs)81 02
> >
> > -
> >
> > USAGE_MINIMUM (Keyboard LeftControl)19 E0
> > USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
> > LOGICAL_MINIMUM (0) 15 00
> > LOGICAL_MAXIMUM (1) 25 01
> > REPORT_SIZE (1) 75 01
> > REPORT_COUNT (8)95 08
> > USAGE_PAGE (Keyboard) 05 07
> > INPUT (Data,Var,Abs)81 02
> >
> > But it makes the parser act wrong for the following report
> > descriptor pattern(such as some Gamepads):
> >
> > USAGE_PAGE (Button) 05 09
> > USAGE (Button 1)09 01
> > USAGE (Button 2)09 02
> > USAGE (Button 4)09 04
> > USAGE (Button 5)09 05
> > USAGE (Button 7)09 07
> > USAGE (Button 8)09 08
> > USAGE (Button 14) 09 0E
> > USAGE (Button 15) 09 0F
> > USAGE (Button 13) 09 0D
> > USAGE_PAGE (Consumer Devices) 05 0C
> > USAGE (Back)0a 24 02
> > USAGE (HomePage)0a 23 02
> > LOGICAL_MINIMUM (0) 15 00
> > LOGICAL_MAXIMUM (1) 25 01
> > REPORT_SIZE (1) 75 01
> > REPORT_COUNT (11) 95 0B
> > INPUT (Data,Var,Abs)81 02
> >
> > With Usage Page concatenation in Main item, parser recognizes all the
> > 11 Usages as consumer keys, it is not the HID device's real intention.
> >
> > This patch adds usage_page_last to flag whether Usage Page is after
> > Usage ID items. usage_page_last is false default, it is set as true
> > once Usage Page item is encountered and is reverted by next Usage ID
> > item.
> >
> > Usage Page concatenation on the currently defined Usage Page will do
> > firstly in Local parsing when Usage ID items encountered.
> >
> > When Main item is parsing, concatenation will do again with last
> > defined Usage Page if usage_page_last flag is true.
> >
> > Signed-off-by: Candle Sun
> > Signed-off-by: Nianfu Bai
> > ---
> > Changes in v2:
> > - Update patch title
> > - Add GET_COMPLETE_USAGE macro
> > - Change the logic of checking whether to concatenate usage page again
> > in main parsing
> > ---
> > drivers/hid/hid-core.c | 31 +--
> > include/linux/hid.h| 1 +
> > 2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 3eaee2c..3394222 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -35,6 +35,8 @@
> >
> > #include "hid-ids.h"
> >
> > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0x))
> > +
> > /*
> > * Version Information
> > */
> > @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
> > unsigned usage, u8 size)
> > hid_err(parser->device, "usage index exceeded\n");
> > return -1;
> > }
> > - parser->local.usage[parser->local.usage_index] = usage;
> > +
> > + if (size <= 2) {
> > + parser->local.usage_page_last = false;
> > + parser->local.usage[parser->local.usage_index] =
> > + GET_COMPLETE_USAGE(parser->global.usage_page,
> > usage);
> > + } else {
> > + parser->local.usage[parser->local.usage_index] = usage;
> > + }
> > +
> > parser->local.usage_size[parser->local.usage_index] = size;
> > parser->local.collection_index[parser->local.usage_index] =
> > parser->collection_stack_ptr ?
> > @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser,
> > struct hid_item *item)
> >
> > case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
> > parser->global.usage_page = item_udata(item);
> > + parser->local.usage_page_last = true;
> >
Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Thu, Oct 10, 2019 at 8:17 PM Benjamin Tissoires
wrote:
>
> On Thu, Oct 10, 2019 at 5:19 AM Candle Sun wrote:
> >
> > On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina wrote:
> > >
> > > On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
> > >
> > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > > index 3eaee2c..3394222 100644
> > > > > --- a/drivers/hid/hid-core.c
> > > > > +++ b/drivers/hid/hid-core.c
> > > > > @@ -35,6 +35,8 @@
> > > > >
> > > > > #include "hid-ids.h"
> > > > >
> > > > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) &
> > > > > 0x))
> > > >
> > > > Not sure I like the macro. I'd rather have the explicit code. That
> > > > said, lets
> > > > see what Benjamin has to say.
> > >
> > > Not sure about Benjamin :) but I personally would ask for putting it
> > > somewhere into hid.h as static inline.
> > >
> > > And even if it's for some reason insisted on this staying macro, please at
> > > least put it as close to the place(s) it's being used as possible, in
> > > order to maintain some code sanity.
> > >
> > > Thanks,
> > >
> > > --
> > > Jiri Kosina
> > > SUSE Labs
> > >
> >
> > Thanks Nicolas and Jiri,
> > If macro is not good, I will change it to static function. But the
> > funciton is only used in hid-core.c,
> > maybe placing it into hid.h is not good?
>
> I would rather use a function too (in hid-core.c, as it's not reused
> anywhere else), and we can make it simpler from the caller point of
> view (if I am not mistaken):
> ---
> static void concatenate_usage_page(struct hid_parser *parser, int index)
> {
> parser->local.usage[index] &= 0x;
> parser->local.usage[index] |= (parser->global.usage_page & 0x) << 16;
> }
>
> // Which can then be called as:
> + parser->local.usage[parser->local.usage_index] = usage;
> + if (size <= 2)
> + concatenate_usage_page(parser, parser->local.usage_index);
> +
>
> // And
> for (i = 0; i < parser->local.usage_index; i++)
> - if (parser->local.usage_size[i] <= 2)
> - parser->local.usage[i] +=
> parser->global.usage_page << 16;
> + if (parser->local.usage_size[i] <= 2) {
> + concatenate_usage_page(parser, i);
> + }
> }
> ---
>
> And now that I have written this, the check on the size could also be
> very well integrated in concatenate_usage_page().
>
> Cheers,
> Benjamin
>
Thanks Benjamin's detailed directions. I will amend the patch.
Candle
> >
> > Regards,
> > Candle
>
Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Wed, Oct 9, 2019 at 2:54 PM Candle Sun wrote:
>
> From: Candle Sun
>
> Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> to Main item") adds support for Usage Page item after Usage ID items
> (such as keyboards manufactured by Primax).
>
> Usage Page concatenation in Main item works well for following report
> descriptor patterns:
>
> USAGE_PAGE (Keyboard) 05 07
> USAGE_MINIMUM (Keyboard LeftControl)19 E0
> USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
> LOGICAL_MINIMUM (0) 15 00
> LOGICAL_MAXIMUM (1) 25 01
> REPORT_SIZE (1) 75 01
> REPORT_COUNT (8)95 08
> INPUT (Data,Var,Abs)81 02
>
> -
>
> USAGE_MINIMUM (Keyboard LeftControl)19 E0
> USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
> LOGICAL_MINIMUM (0) 15 00
> LOGICAL_MAXIMUM (1) 25 01
> REPORT_SIZE (1) 75 01
> REPORT_COUNT (8)95 08
> USAGE_PAGE (Keyboard) 05 07
> INPUT (Data,Var,Abs)81 02
>
> But it makes the parser act wrong for the following report
> descriptor pattern(such as some Gamepads):
>
> USAGE_PAGE (Button) 05 09
> USAGE (Button 1)09 01
> USAGE (Button 2)09 02
> USAGE (Button 4)09 04
> USAGE (Button 5)09 05
> USAGE (Button 7)09 07
> USAGE (Button 8)09 08
> USAGE (Button 14) 09 0E
> USAGE (Button 15) 09 0F
> USAGE (Button 13) 09 0D
> USAGE_PAGE (Consumer Devices) 05 0C
> USAGE (Back)0a 24 02
> USAGE (HomePage)0a 23 02
> LOGICAL_MINIMUM (0) 15 00
> LOGICAL_MAXIMUM (1) 25 01
> REPORT_SIZE (1) 75 01
> REPORT_COUNT (11) 95 0B
> INPUT (Data,Var,Abs)81 02
>
> With Usage Page concatenation in Main item, parser recognizes all the
> 11 Usages as consumer keys, it is not the HID device's real intention.
>
> This patch adds usage_page_last to flag whether Usage Page is after
> Usage ID items. usage_page_last is false default, it is set as true
> once Usage Page item is encountered and is reverted by next Usage ID
> item.
>
> Usage Page concatenation on the currently defined Usage Page will do
> firstly in Local parsing when Usage ID items encountered.
>
> When Main item is parsing, concatenation will do again with last
> defined Usage Page if usage_page_last flag is true.
>
> Signed-off-by: Candle Sun
> Signed-off-by: Nianfu Bai
> ---
> Changes in v2:
> - Update patch title
> - Add GET_COMPLETE_USAGE macro
> - Change the logic of checking whether to concatenate usage page again
> in main parsing
> ---
> drivers/hid/hid-core.c | 31 +--
> include/linux/hid.h| 1 +
> 2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3eaee2c..3394222 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -35,6 +35,8 @@
>
> #include "hid-ids.h"
>
> +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0x))
> +
> /*
> * Version Information
> */
> @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
> unsigned usage, u8 size)
> hid_err(parser->device, "usage index exceeded\n");
> return -1;
> }
> - parser->local.usage[parser->local.usage_index] = usage;
> +
> + if (size <= 2) {
> + parser->local.usage_page_last = false;
> + parser->local.usage[parser->local.usage_index] =
> + GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> + } else {
> + parser->local.usage[parser->local.usage_index] = usage;
> + }
> +
> parser->local.usage_size[parser->local.usage_index] = size;
> parser->local.collection_index[parser->local.usage_index] =
> parser->collection_stack_ptr ?
> @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser,
> struct hid_item *item)
>
> case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
> parser->global.usage_page = item_udata(item);
> + parser->local.usage_page_last = true;
> return 0;
>
> case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
> @@ -543,13 +554,21 @@ static int hid_parser_local(struct hid_parser *parser,
> struct hid_item *item)
> * usage value."
> */
>
> -static void hid_concatenate_usage_page(struct hid_parser *parser)
> +static void
Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Thu, Oct 10, 2019 at 5:19 AM Candle Sun wrote:
>
> On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina wrote:
> >
> > On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote:
> >
> > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > index 3eaee2c..3394222 100644
> > > > --- a/drivers/hid/hid-core.c
> > > > +++ b/drivers/hid/hid-core.c
> > > > @@ -35,6 +35,8 @@
> > > >
> > > > #include "hid-ids.h"
> > > >
> > > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0x))
> > >
> > > Not sure I like the macro. I'd rather have the explicit code. That said,
> > > lets
> > > see what Benjamin has to say.
> >
> > Not sure about Benjamin :) but I personally would ask for putting it
> > somewhere into hid.h as static inline.
> >
> > And even if it's for some reason insisted on this staying macro, please at
> > least put it as close to the place(s) it's being used as possible, in
> > order to maintain some code sanity.
> >
> > Thanks,
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >
>
> Thanks Nicolas and Jiri,
> If macro is not good, I will change it to static function. But the
> funciton is only used in hid-core.c,
> maybe placing it into hid.h is not good?
I would rather use a function too (in hid-core.c, as it's not reused
anywhere else), and we can make it simpler from the caller point of
view (if I am not mistaken):
---
static void concatenate_usage_page(struct hid_parser *parser, int index)
{
parser->local.usage[index] &= 0x;
parser->local.usage[index] |= (parser->global.usage_page & 0x) << 16;
}
// Which can then be called as:
+ parser->local.usage[parser->local.usage_index] = usage;
+ if (size <= 2)
+ concatenate_usage_page(parser, parser->local.usage_index);
+
// And
for (i = 0; i < parser->local.usage_index; i++)
- if (parser->local.usage_size[i] <= 2)
- parser->local.usage[i] +=
parser->global.usage_page << 16;
+ if (parser->local.usage_size[i] <= 2) {
+ concatenate_usage_page(parser, i);
+ }
}
---
And now that I have written this, the check on the size could also be
very well integrated in concatenate_usage_page().
Cheers,
Benjamin
>
> Regards,
> Candle
Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Thu, 2019-10-10 at 11:05 +0800, Candle Sun wrote:
> > > -static void hid_concatenate_usage_page(struct hid_parser *parser)
> > > +static void hid_concatenate_last_usage_page(struct hid_parser *parser)
> > > {
> > > int i;
> > > + unsigned int usage;
> > > + unsigned int usage_page = parser->global.usage_page;
> > > +
> > > + if (!parser->local.usage_page_last)
> > > + return;
> > >
> > > for (i = 0; i < parser->local.usage_index; i++)
> >
> > Technically correct but it's preferred if you use braces here.
> >
>
> Nicolas, do you mean this:
>
> @@ -563,12 +563,13 @@ static void
> hid_concatenate_last_usage_page(struct hid_parser *parser)
> if (!parser->local.usage_page_last)
> return;
>
> - for (i = 0; i < parser->local.usage_index; i++)
> + for (i = 0; i < parser->local.usage_index; i++) {
> if (parser->local.usage_size[i] <= 2) {
> usage = parser->local.usage[i];
> parser->local.usage[i] =
> GET_COMPLETE_USAGE(usage_page, usage);
> }
> + }
> }
Yes, thanks!
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Thu, Oct 10, 2019 at 2:00 AM Jiri Kosina wrote: > > On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote: > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > > index 3eaee2c..3394222 100644 > > > --- a/drivers/hid/hid-core.c > > > +++ b/drivers/hid/hid-core.c > > > @@ -35,6 +35,8 @@ > > > > > > #include "hid-ids.h" > > > > > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0x)) > > > > Not sure I like the macro. I'd rather have the explicit code. That said, > > lets > > see what Benjamin has to say. > > Not sure about Benjamin :) but I personally would ask for putting it > somewhere into hid.h as static inline. > > And even if it's for some reason insisted on this staying macro, please at > least put it as close to the place(s) it's being used as possible, in > order to maintain some code sanity. > > Thanks, > > -- > Jiri Kosina > SUSE Labs > Thanks Nicolas and Jiri, If macro is not good, I will change it to static function. But the funciton is only used in hid-core.c, maybe placing it into hid.h is not good? Regards, Candle
Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Thu, Oct 10, 2019 at 1:01 AM Nicolas Saenz Julienne
wrote:
>
> On Wed, 2019-10-09 at 20:53 +0800, Candle Sun wrote:
> > From: Candle Sun
> >
> > Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> > to Main item") adds support for Usage Page item after Usage ID items
> > (such as keyboards manufactured by Primax).
> >
> > Usage Page concatenation in Main item works well for following report
> > descriptor patterns:
> >
> > USAGE_PAGE (Keyboard) 05 07
> > USAGE_MINIMUM (Keyboard LeftControl)19 E0
> > USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
> > LOGICAL_MINIMUM (0) 15 00
> > LOGICAL_MAXIMUM (1) 25 01
> > REPORT_SIZE (1) 75 01
> > REPORT_COUNT (8)95 08
> > INPUT (Data,Var,Abs)81 02
> >
> > -
> >
> > USAGE_MINIMUM (Keyboard LeftControl)19 E0
> > USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
> > LOGICAL_MINIMUM (0) 15 00
> > LOGICAL_MAXIMUM (1) 25 01
> > REPORT_SIZE (1) 75 01
> > REPORT_COUNT (8)95 08
> > USAGE_PAGE (Keyboard) 05 07
> > INPUT (Data,Var,Abs)81 02
> >
> > But it makes the parser act wrong for the following report
> > descriptor pattern(such as some Gamepads):
> >
> > USAGE_PAGE (Button) 05 09
> > USAGE (Button 1)09 01
> > USAGE (Button 2)09 02
> > USAGE (Button 4)09 04
> > USAGE (Button 5)09 05
> > USAGE (Button 7)09 07
> > USAGE (Button 8)09 08
> > USAGE (Button 14) 09 0E
> > USAGE (Button 15) 09 0F
> > USAGE (Button 13) 09 0D
> > USAGE_PAGE (Consumer Devices) 05 0C
> > USAGE (Back)0a 24 02
> > USAGE (HomePage)0a 23 02
> > LOGICAL_MINIMUM (0) 15 00
> > LOGICAL_MAXIMUM (1) 25 01
> > REPORT_SIZE (1) 75 01
> > REPORT_COUNT (11) 95 0B
> > INPUT (Data,Var,Abs)81 02
> >
> > With Usage Page concatenation in Main item, parser recognizes all the
> > 11 Usages as consumer keys, it is not the HID device's real intention.
> >
> > This patch adds usage_page_last to flag whether Usage Page is after
> > Usage ID items. usage_page_last is false default, it is set as true
> > once Usage Page item is encountered and is reverted by next Usage ID
> > item.
> >
> > Usage Page concatenation on the currently defined Usage Page will do
> > firstly in Local parsing when Usage ID items encountered.
> >
> > When Main item is parsing, concatenation will do again with last
> > defined Usage Page if usage_page_last flag is true.
>
> Functionally I think this is the right approach. Sadly I don't have access to
> any Primax device anymore so I can't test it. But I suggest you update
> hid-tools' parser and add a new unit test to verify we aren't missing
> anything.
>
> You can base your code on this:
>
> https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37/commits
>
Thanks Nicolas. I will check and try to do it.
Candle
> > Signed-off-by: Candle Sun
> > Signed-off-by: Nianfu Bai
> > ---
> > Changes in v2:
> > - Update patch title
> > - Add GET_COMPLETE_USAGE macro
> > - Change the logic of checking whether to concatenate usage page again
> > in main parsing
> > ---
> > drivers/hid/hid-core.c | 31 +--
> > include/linux/hid.h| 1 +
> > 2 files changed, 26 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 3eaee2c..3394222 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -35,6 +35,8 @@
> >
> > #include "hid-ids.h"
> >
> > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0x))
>
> Not sure I like the macro. I'd rather have the explicit code. That said, lets
> see what Benjamin has to say.
>
> > +
> > /*
> > * Version Information
> > */
> > @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
> > unsigned usage, u8 size)
> > hid_err(parser->device, "usage index exceeded\n");
> > return -1;
> > }
> > - parser->local.usage[parser->local.usage_index] = usage;
> > +
> > + if (size <= 2) {
> > + parser->local.usage_page_last = false;
> > + parser->local.usage[parser->local.usage_index] =
> > + GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> > + } else {
> > + parser->local.usage[parser->local.usage_index] = usage;
> >
Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Wed, 9 Oct 2019, Nicolas Saenz Julienne wrote: > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 3eaee2c..3394222 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -35,6 +35,8 @@ > > > > #include "hid-ids.h" > > > > +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0x)) > > Not sure I like the macro. I'd rather have the explicit code. That said, lets > see what Benjamin has to say. Not sure about Benjamin :) but I personally would ask for putting it somewhere into hid.h as static inline. And even if it's for some reason insisted on this staying macro, please at least put it as close to the place(s) it's being used as possible, in order to maintain some code sanity. Thanks, -- Jiri Kosina SUSE Labs
Re: [PATCH v2] HID: core: check whether usage page item is after usage id item
On Wed, 2019-10-09 at 20:53 +0800, Candle Sun wrote:
> From: Candle Sun
>
> Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
> to Main item") adds support for Usage Page item after Usage ID items
> (such as keyboards manufactured by Primax).
>
> Usage Page concatenation in Main item works well for following report
> descriptor patterns:
>
> USAGE_PAGE (Keyboard) 05 07
> USAGE_MINIMUM (Keyboard LeftControl)19 E0
> USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
> LOGICAL_MINIMUM (0) 15 00
> LOGICAL_MAXIMUM (1) 25 01
> REPORT_SIZE (1) 75 01
> REPORT_COUNT (8)95 08
> INPUT (Data,Var,Abs)81 02
>
> -
>
> USAGE_MINIMUM (Keyboard LeftControl)19 E0
> USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
> LOGICAL_MINIMUM (0) 15 00
> LOGICAL_MAXIMUM (1) 25 01
> REPORT_SIZE (1) 75 01
> REPORT_COUNT (8)95 08
> USAGE_PAGE (Keyboard) 05 07
> INPUT (Data,Var,Abs)81 02
>
> But it makes the parser act wrong for the following report
> descriptor pattern(such as some Gamepads):
>
> USAGE_PAGE (Button) 05 09
> USAGE (Button 1)09 01
> USAGE (Button 2)09 02
> USAGE (Button 4)09 04
> USAGE (Button 5)09 05
> USAGE (Button 7)09 07
> USAGE (Button 8)09 08
> USAGE (Button 14) 09 0E
> USAGE (Button 15) 09 0F
> USAGE (Button 13) 09 0D
> USAGE_PAGE (Consumer Devices) 05 0C
> USAGE (Back)0a 24 02
> USAGE (HomePage)0a 23 02
> LOGICAL_MINIMUM (0) 15 00
> LOGICAL_MAXIMUM (1) 25 01
> REPORT_SIZE (1) 75 01
> REPORT_COUNT (11) 95 0B
> INPUT (Data,Var,Abs)81 02
>
> With Usage Page concatenation in Main item, parser recognizes all the
> 11 Usages as consumer keys, it is not the HID device's real intention.
>
> This patch adds usage_page_last to flag whether Usage Page is after
> Usage ID items. usage_page_last is false default, it is set as true
> once Usage Page item is encountered and is reverted by next Usage ID
> item.
>
> Usage Page concatenation on the currently defined Usage Page will do
> firstly in Local parsing when Usage ID items encountered.
>
> When Main item is parsing, concatenation will do again with last
> defined Usage Page if usage_page_last flag is true.
Functionally I think this is the right approach. Sadly I don't have access to
any Primax device anymore so I can't test it. But I suggest you update
hid-tools' parser and add a new unit test to verify we aren't missing anything.
You can base your code on this:
https://gitlab.freedesktop.org/libevdev/hid-tools/merge_requests/37/commits
> Signed-off-by: Candle Sun
> Signed-off-by: Nianfu Bai
> ---
> Changes in v2:
> - Update patch title
> - Add GET_COMPLETE_USAGE macro
> - Change the logic of checking whether to concatenate usage page again
> in main parsing
> ---
> drivers/hid/hid-core.c | 31 +--
> include/linux/hid.h| 1 +
> 2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 3eaee2c..3394222 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -35,6 +35,8 @@
>
> #include "hid-ids.h"
>
> +#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0x))
Not sure I like the macro. I'd rather have the explicit code. That said, lets
see what Benjamin has to say.
> +
> /*
> * Version Information
> */
> @@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
> unsigned usage, u8 size)
> hid_err(parser->device, "usage index exceeded\n");
> return -1;
> }
> - parser->local.usage[parser->local.usage_index] = usage;
> +
> + if (size <= 2) {
> + parser->local.usage_page_last = false;
> + parser->local.usage[parser->local.usage_index] =
> + GET_COMPLETE_USAGE(parser->global.usage_page, usage);
> + } else {
> + parser->local.usage[parser->local.usage_index] = usage;
> + }
> +
> parser->local.usage_size[parser->local.usage_index] = size;
> parser->local.collection_index[parser->local.usage_index] =
> parser->collection_stack_ptr ?
> @@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser,
> struct hid_item *item)
>
> case
[PATCH v2] HID: core: check whether usage page item is after usage id item
From: Candle Sun
Upstream commit 58e75155009c ("HID: core: move Usage Page concatenation
to Main item") adds support for Usage Page item after Usage ID items
(such as keyboards manufactured by Primax).
Usage Page concatenation in Main item works well for following report
descriptor patterns:
USAGE_PAGE (Keyboard) 05 07
USAGE_MINIMUM (Keyboard LeftControl)19 E0
USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
LOGICAL_MINIMUM (0) 15 00
LOGICAL_MAXIMUM (1) 25 01
REPORT_SIZE (1) 75 01
REPORT_COUNT (8)95 08
INPUT (Data,Var,Abs)81 02
-
USAGE_MINIMUM (Keyboard LeftControl)19 E0
USAGE_MAXIMUM (Keyboard Right GUI) 29 E7
LOGICAL_MINIMUM (0) 15 00
LOGICAL_MAXIMUM (1) 25 01
REPORT_SIZE (1) 75 01
REPORT_COUNT (8)95 08
USAGE_PAGE (Keyboard) 05 07
INPUT (Data,Var,Abs)81 02
But it makes the parser act wrong for the following report
descriptor pattern(such as some Gamepads):
USAGE_PAGE (Button) 05 09
USAGE (Button 1)09 01
USAGE (Button 2)09 02
USAGE (Button 4)09 04
USAGE (Button 5)09 05
USAGE (Button 7)09 07
USAGE (Button 8)09 08
USAGE (Button 14) 09 0E
USAGE (Button 15) 09 0F
USAGE (Button 13) 09 0D
USAGE_PAGE (Consumer Devices) 05 0C
USAGE (Back)0a 24 02
USAGE (HomePage)0a 23 02
LOGICAL_MINIMUM (0) 15 00
LOGICAL_MAXIMUM (1) 25 01
REPORT_SIZE (1) 75 01
REPORT_COUNT (11) 95 0B
INPUT (Data,Var,Abs)81 02
With Usage Page concatenation in Main item, parser recognizes all the
11 Usages as consumer keys, it is not the HID device's real intention.
This patch adds usage_page_last to flag whether Usage Page is after
Usage ID items. usage_page_last is false default, it is set as true
once Usage Page item is encountered and is reverted by next Usage ID
item.
Usage Page concatenation on the currently defined Usage Page will do
firstly in Local parsing when Usage ID items encountered.
When Main item is parsing, concatenation will do again with last
defined Usage Page if usage_page_last flag is true.
Signed-off-by: Candle Sun
Signed-off-by: Nianfu Bai
---
Changes in v2:
- Update patch title
- Add GET_COMPLETE_USAGE macro
- Change the logic of checking whether to concatenate usage page again
in main parsing
---
drivers/hid/hid-core.c | 31 +--
include/linux/hid.h| 1 +
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 3eaee2c..3394222 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -35,6 +35,8 @@
#include "hid-ids.h"
+#define GET_COMPLETE_USAGE(page, id) (((page) << 16) + ((id) & 0x))
+
/*
* Version Information
*/
@@ -221,7 +223,15 @@ static int hid_add_usage(struct hid_parser *parser,
unsigned usage, u8 size)
hid_err(parser->device, "usage index exceeded\n");
return -1;
}
- parser->local.usage[parser->local.usage_index] = usage;
+
+ if (size <= 2) {
+ parser->local.usage_page_last = false;
+ parser->local.usage[parser->local.usage_index] =
+ GET_COMPLETE_USAGE(parser->global.usage_page, usage);
+ } else {
+ parser->local.usage[parser->local.usage_index] = usage;
+ }
+
parser->local.usage_size[parser->local.usage_index] = size;
parser->local.collection_index[parser->local.usage_index] =
parser->collection_stack_ptr ?
@@ -366,6 +376,7 @@ static int hid_parser_global(struct hid_parser *parser,
struct hid_item *item)
case HID_GLOBAL_ITEM_TAG_USAGE_PAGE:
parser->global.usage_page = item_udata(item);
+ parser->local.usage_page_last = true;
return 0;
case HID_GLOBAL_ITEM_TAG_LOGICAL_MINIMUM:
@@ -543,13 +554,21 @@ static int hid_parser_local(struct hid_parser *parser,
struct hid_item *item)
* usage value."
*/
-static void hid_concatenate_usage_page(struct hid_parser *parser)
+static void hid_concatenate_last_usage_page(struct hid_parser *parser)
{
int i;
+ unsigned int usage;
+ unsigned int usage_page = parser->global.usage_page;
+
+ if (!parser->local.usage_page_last)
+ return;
for (i = 0; i < parser->local.usage_index;
