| Message ID | 1359649351-11092-2-git-send-email-benjamin.tissoires@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi Benjamin, > This callback is called when the parsing of the report has been done > by hid-core (so after the calls to .event). The hid drivers can now > have access to the whole report by relying on the values stored in > the different fields. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com> > --- > drivers/hid/hid-core.c | 4 ++++ > include/linux/hid.h | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 5ae2cb1..b671e4e 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, > { > struct hid_report_enum *report_enum = hid->report_enum + type; > struct hid_report *report; > + struct hid_driver *hdrv; > unsigned int a; > int rsize, csize = size; > u8 *cdata = data; > @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, > if (hid->claimed != HID_CLAIMED_HIDRAW) { > for (a = 0; a < report->maxfield; a++) > hid_input_field(hid, report->field[a], cdata, interrupt); > + hdrv = hid->driver; > + if (hdrv && hdrv->report) > + hdrv->report(hid, report); I think this is more useful if called before the individual fields. In fact, it seems raw_event() is already doing exactly that. No need for a new callback, in other words. > } > > if (hid->claimed & HID_CLAIMED_INPUT) > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 828726c..e14b465 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -589,6 +589,7 @@ struct hid_usage_id { > * @raw_event: if report in report_table, this hook is called (NULL means nop) > * @usage_table: on which events to call event (NULL means all) > * @event: if usage in usage_table, this hook is called (NULL means nop) > + * @report: this hook is called after parsing a report (NULL means nop) > * @report_fixup: called before report descriptor parsing (NULL means nop) > * @input_mapping: invoked on input registering before mapping an usage > * @input_mapped: invoked on input registering after mapping an usage > @@ -627,6 +628,7 @@ struct hid_driver { > const struct hid_usage_id *usage_table; > int (*event)(struct hid_device *hdev, struct hid_field *field, > struct hid_usage *usage, __s32 value); > + void (*report)(struct hid_device *hdev, struct hid_report *report); > > __u8 *(*report_fixup)(struct hid_device *hdev, __u8 *buf, > unsigned int *size); > -- > 1.8.1 > Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Henrik, On Sun, Feb 3, 2013 at 1:27 PM, Henrik Rydberg <rydberg@euromail.se> wrote: > Hi Benjamin, > >> This callback is called when the parsing of the report has been done >> by hid-core (so after the calls to .event). The hid drivers can now >> have access to the whole report by relying on the values stored in >> the different fields. >> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com> >> --- >> drivers/hid/hid-core.c | 4 ++++ >> include/linux/hid.h | 2 ++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >> index 5ae2cb1..b671e4e 100644 >> --- a/drivers/hid/hid-core.c >> +++ b/drivers/hid/hid-core.c >> @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, >> { >> struct hid_report_enum *report_enum = hid->report_enum + type; >> struct hid_report *report; >> + struct hid_driver *hdrv; >> unsigned int a; >> int rsize, csize = size; >> u8 *cdata = data; >> @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, >> if (hid->claimed != HID_CLAIMED_HIDRAW) { >> for (a = 0; a < report->maxfield; a++) >> hid_input_field(hid, report->field[a], cdata, interrupt); >> + hdrv = hid->driver; >> + if (hdrv && hdrv->report) >> + hdrv->report(hid, report); > > I think this is more useful if called before the individual fields. In > fact, it seems raw_event() is already doing exactly that. No need for > a new callback, in other words. I'm afraid this does not work: - calling it before the individual fields is not possible unless more intrusive changes to hid-core: * "hid_input_field" does the actual parsing of the input report, so without the call the .value field is still set to the previous one (not very useful then) * the "hid_input_field" requires to know the previous report, due to Array type reports which send the current keys down, and not the release. - the raw_event() callback contains the report _unparsed_, which means that it's up to the hid specific driver to split the fields depending on the report descriptor (for instance, get the right bit for tipswitch in a byte containing tipswitch, inrange, confidence, contactId). - the raw_event() mechanism was the one in place in the v1 of the patch series, in which the new callback was asked. Cheers, Benjamin > >> } >> >> if (hid->claimed & HID_CLAIMED_INPUT) >> diff --git a/include/linux/hid.h b/include/linux/hid.h >> index 828726c..e14b465 100644 >> --- a/include/linux/hid.h >> +++ b/include/linux/hid.h >> @@ -589,6 +589,7 @@ struct hid_usage_id { >> * @raw_event: if report in report_table, this hook is called (NULL means nop) >> * @usage_table: on which events to call event (NULL means all) >> * @event: if usage in usage_table, this hook is called (NULL means nop) >> + * @report: this hook is called after parsing a report (NULL means nop) >> * @report_fixup: called before report descriptor parsing (NULL means nop) >> * @input_mapping: invoked on input registering before mapping an usage >> * @input_mapped: invoked on input registering after mapping an usage >> @@ -627,6 +628,7 @@ struct hid_driver { >> const struct hid_usage_id *usage_table; >> int (*event)(struct hid_device *hdev, struct hid_field *field, >> struct hid_usage *usage, __s32 value); >> + void (*report)(struct hid_device *hdev, struct hid_report *report); >> >> __u8 *(*report_fixup)(struct hid_device *hdev, __u8 *buf, >> unsigned int *size); >> -- >> 1.8.1 >> > > Thanks, > Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 3 Feb 2013, Henrik Rydberg wrote: > > This callback is called when the parsing of the report has been done > > by hid-core (so after the calls to .event). The hid drivers can now > > have access to the whole report by relying on the values stored in > > the different fields. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > --- > > drivers/hid/hid-core.c | 4 ++++ > > include/linux/hid.h | 2 ++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 5ae2cb1..b671e4e 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, > > { > > struct hid_report_enum *report_enum = hid->report_enum + type; > > struct hid_report *report; > > + struct hid_driver *hdrv; > > unsigned int a; > > int rsize, csize = size; > > u8 *cdata = data; > > @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, > > if (hid->claimed != HID_CLAIMED_HIDRAW) { > > for (a = 0; a < report->maxfield; a++) > > hid_input_field(hid, report->field[a], cdata, interrupt); > > + hdrv = hid->driver; > > + if (hdrv && hdrv->report) > > + hdrv->report(hid, report); > > I think this is more useful if called before the individual fields. In > fact, it seems raw_event() is already doing exactly that. No need for > a new callback, in other words. raw_event() doesn't provide equivalent functionality though; namely, the report is not parsed. Or have I missed your point? Thanks for the extensive review, Henrik, it's really helpful.
On Mon, Feb 4, 2013 at 10:34 AM, Jiri Kosina <jkosina@suse.cz> wrote: > On Sun, 3 Feb 2013, Henrik Rydberg wrote: > >> > This callback is called when the parsing of the report has been done >> > by hid-core (so after the calls to .event). The hid drivers can now >> > have access to the whole report by relying on the values stored in >> > the different fields. >> > >> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com> >> > --- >> > drivers/hid/hid-core.c | 4 ++++ >> > include/linux/hid.h | 2 ++ >> > 2 files changed, 6 insertions(+) >> > >> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c >> > index 5ae2cb1..b671e4e 100644 >> > --- a/drivers/hid/hid-core.c >> > +++ b/drivers/hid/hid-core.c >> > @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, >> > { >> > struct hid_report_enum *report_enum = hid->report_enum + type; >> > struct hid_report *report; >> > + struct hid_driver *hdrv; >> > unsigned int a; >> > int rsize, csize = size; >> > u8 *cdata = data; >> > @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, >> > if (hid->claimed != HID_CLAIMED_HIDRAW) { >> > for (a = 0; a < report->maxfield; a++) >> > hid_input_field(hid, report->field[a], cdata, interrupt); >> > + hdrv = hid->driver; >> > + if (hdrv && hdrv->report) >> > + hdrv->report(hid, report); >> >> I think this is more useful if called before the individual fields. In >> fact, it seems raw_event() is already doing exactly that. No need for >> a new callback, in other words. > > raw_event() doesn't provide equivalent functionality though; namely, the > report is not parsed. > > Or have I missed your point? No, you perfectly understood the purpose of the patch. raw_event() and report() are not the same kind of callbacks at all. > > Thanks for the extensive review, Henrik, it's really helpful. Yep, thanks Henrik, and thanks Jiri for having a look at it. Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benjamin, > > I think this is more useful if called before the individual fields. In > > fact, it seems raw_event() is already doing exactly that. No need for > > a new callback, in other words. > > I'm afraid this does not work: > - calling it before the individual fields is not possible unless more > intrusive changes to hid-core: > * "hid_input_field" does the actual parsing of the input report, so > without the call the .value field is still set to the previous one > (not very useful then) > * the "hid_input_field" requires to know the previous report, due to > Array type reports which send the current keys down, and not the > release. > - the raw_event() callback contains the report _unparsed_, which means > that it's up to the hid specific driver to split the fields depending > on the report descriptor (for instance, get the right bit for > tipswitch in a byte containing tipswitch, inrange, confidence, > contactId). > - the raw_event() mechanism was the one in place in the v1 of the > patch series, in which the new callback was asked. Thanks for the compelling argument list, v1 had obviously been thrown out of the cache already. ;-) I agree with your approach. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 5ae2cb1..b671e4e 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, { struct hid_report_enum *report_enum = hid->report_enum + type; struct hid_report *report; + struct hid_driver *hdrv; unsigned int a; int rsize, csize = size; u8 *cdata = data; @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, if (hid->claimed != HID_CLAIMED_HIDRAW) { for (a = 0; a < report->maxfield; a++) hid_input_field(hid, report->field[a], cdata, interrupt); + hdrv = hid->driver; + if (hdrv && hdrv->report) + hdrv->report(hid, report); } if (hid->claimed & HID_CLAIMED_INPUT) diff --git a/include/linux/hid.h b/include/linux/hid.h index 828726c..e14b465 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -589,6 +589,7 @@ struct hid_usage_id { * @raw_event: if report in report_table, this hook is called (NULL means nop) * @usage_table: on which events to call event (NULL means all) * @event: if usage in usage_table, this hook is called (NULL means nop) + * @report: this hook is called after parsing a report (NULL means nop) * @report_fixup: called before report descriptor parsing (NULL means nop) * @input_mapping: invoked on input registering before mapping an usage * @input_mapped: invoked on input registering after mapping an usage @@ -627,6 +628,7 @@ struct hid_driver { const struct hid_usage_id *usage_table; int (*event)(struct hid_device *hdev, struct hid_field *field, struct hid_usage *usage, __s32 value); + void (*report)(struct hid_device *hdev, struct hid_report *report); __u8 *(*report_fixup)(struct hid_device *hdev, __u8 *buf, unsigned int *size);
This callback is called when the parsing of the report has been done by hid-core (so after the calls to .event). The hid drivers can now have access to the whole report by relying on the values stored in the different fields. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com> --- drivers/hid/hid-core.c | 4 ++++ include/linux/hid.h | 2 ++ 2 files changed, 6 insertions(+)