When the multi input quirk is set, there is a new input device created for every feature report. Since the idea is to present features per hid device, not per input device, revert back to the original report loop and change the feature_mapping() callback to not take the input device as argument. Signed-off-by: Henrik Rydberg <rydberg@euromail.se> --- Hi Jiri, Benjamin, It seems the feature report callback was a bit intrusive, after all; for some devices such as ntrig, with multi input, there are additional input devices created. The following patch seems to fix the problem, but it has not been tested on any device using the hid-multitouch driver. Thanks, Henrik drivers/hid/hid-input.c | 30 +++++++++++++++++++++--------- drivers/hid/hid-multitouch.c | 2 +- include/linux/hid.h | 2 +- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 7f552bf..ebcc02a 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -290,14 +290,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel goto ignore; } - if (field->report_type == HID_FEATURE_REPORT) { - if (device->driver->feature_mapping) { - device->driver->feature_mapping(device, hidinput, field, - usage); - } - goto ignore; - } - if (device->driver->input_mapping) { int ret = device->driver->input_mapping(device, hidinput, field, usage, &bit, &max); @@ -835,6 +827,24 @@ static void hidinput_close(struct input_dev *dev) hid_hw_close(hid); } +static void report_features(struct hid_device *hid) +{ + struct hid_driver *drv = hid->driver; + struct hid_report_enum *rep_enum; + struct hid_report *rep; + int i, j; + + if (!drv->feature_mapping) + return; + + rep_enum = &hid->report_enum[HID_FEATURE_REPORT]; + list_for_each_entry(rep, &rep_enum->report_list, list) + for (i = 0; i < rep->maxfield; i++) + for (j = 0; j < rep->field[i]->maxusage; j++) + drv->feature_mapping(hid, rep->field[i], + rep->field[i]->usage + j); +} + /* * Register the input device; print a message. * Configure the input layer interface @@ -863,7 +873,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force) return -1; } - for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) { + report_features(hid); + + for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) { if (k == HID_OUTPUT_REPORT && hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS) continue; diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 07d3183..2bbc954 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -122,7 +122,7 @@ struct mt_class mt_classes[] = { { } }; -static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi, +static void mt_feature_mapping(struct hid_device *hdev, struct hid_field *field, struct hid_usage *usage) { if (usage->hid == HID_DG_INPUTMODE) { diff --git a/include/linux/hid.h b/include/linux/hid.h index d91c25e..fc5faf6 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -638,7 +638,7 @@ struct hid_driver { struct hid_input *hidinput, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max); void (*feature_mapping)(struct hid_device *hdev, - struct hid_input *hidinput, struct hid_field *field, + struct hid_field *field, struct hid_usage *usage); #ifdef CONFIG_PM int (*suspend)(struct hid_device *hdev, pm_message_t message); -- 1.7.4.1
Hi Henrik,
Thanks for spotting this. BTW, I'm not happy with your solution.
You are sending the feature report before creating the struct
hid_input. To be consistent with the rest, we have to keep the same
signature. Today, the code does not make any use of it. But I use it
in my devel branch to auto-detect the maximum contact count of the
device. This was the safest place to call input_mt_init_slots.
How about adding hidinput as an argument to report_features
and calling it after the " for (k = HID_INPUT_REPORT; k <=
HID_OUTPUT_REPORT; k++) {" loop with
list_for_each_entry_safe(hidinput, next, &hid->inputs, list)
report_features(hid, hidinput);
I did not even try to compile it right now (I don't have any
multitouch device right now) but I'll be able to make further tests
tomorrow.
Cheers,
Benjamin
On Thu, Feb 24, 2011 at 19:30, Henrik Rydberg <rydberg@euromail.se> wrote:
> When the multi input quirk is set, there is a new input device
> created for every feature report. Since the idea is to present
> features per hid device, not per input device, revert back to
> the original report loop and change the feature_mapping() callback
> to not take the input device as argument.
>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
> Hi Jiri, Benjamin,
>
> It seems the feature report callback was a bit intrusive, after all;
> for some devices such as ntrig, with multi input, there are additional
> input devices created. The following patch seems to fix the problem,
> but it has not been tested on any device using the hid-multitouch
> driver.
>
> Thanks,
> Henrik
>
> drivers/hid/hid-input.c | 30 +++++++++++++++++++++---------
> drivers/hid/hid-multitouch.c | 2 +-
> include/linux/hid.h | 2 +-
> 3 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 7f552bf..ebcc02a 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -290,14 +290,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
> goto ignore;
> }
>
> - if (field->report_type == HID_FEATURE_REPORT) {
> - if (device->driver->feature_mapping) {
> - device->driver->feature_mapping(device, hidinput, field,
> - usage);
> - }
> - goto ignore;
> - }
> -
> if (device->driver->input_mapping) {
> int ret = device->driver->input_mapping(device, hidinput, field,
> usage, &bit, &max);
> @@ -835,6 +827,24 @@ static void hidinput_close(struct input_dev *dev)
> hid_hw_close(hid);
> }
>
> +static void report_features(struct hid_device *hid)
> +{
> + struct hid_driver *drv = hid->driver;
> + struct hid_report_enum *rep_enum;
> + struct hid_report *rep;
> + int i, j;
> +
> + if (!drv->feature_mapping)
> + return;
> +
> + rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
> + list_for_each_entry(rep, &rep_enum->report_list, list)
> + for (i = 0; i < rep->maxfield; i++)
> + for (j = 0; j < rep->field[i]->maxusage; j++)
> + drv->feature_mapping(hid, rep->field[i],
> + rep->field[i]->usage + j);
> +}
> +
> /*
> * Register the input device; print a message.
> * Configure the input layer interface
> @@ -863,7 +873,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> return -1;
> }
>
> - for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) {
> + report_features(hid);
> +
> + for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {
> if (k == HID_OUTPUT_REPORT &&
> hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
> continue;
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 07d3183..2bbc954 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -122,7 +122,7 @@ struct mt_class mt_classes[] = {
> { }
> };
>
> -static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
> +static void mt_feature_mapping(struct hid_device *hdev,
> struct hid_field *field, struct hid_usage *usage)
> {
> if (usage->hid == HID_DG_INPUTMODE) {
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index d91c25e..fc5faf6 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -638,7 +638,7 @@ struct hid_driver {
> struct hid_input *hidinput, struct hid_field *field,
> struct hid_usage *usage, unsigned long **bit, int *max);
> void (*feature_mapping)(struct hid_device *hdev,
> - struct hid_input *hidinput, struct hid_field *field,
> + struct hid_field *field,
> struct hid_usage *usage);
> #ifdef CONFIG_PM
> int (*suspend)(struct hid_device *hdev, pm_message_t message);
> --
> 1.7.4.1
>
> --
> 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, Thanks for spotting this. BTW, I'm not happy with your solution. You are sending the feature report before creating the struct hid_input. To be consistent with the rest, we have to keep the same signature. Today, the code does not make any use of it. But I use it in my devel branch to auto-detect the maximum contact count of the device. This was the safest place to call input_mt_init_slots. How about adding hidinput as an argument to report_features and calling it after the " for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {" loop with list_for_each_entry_safe(hidinput, next, &hid->inputs, list) report_features(hid, hidinput); I did not even try to compile it right now (I don't have any multitouch device right now) but I'll be able to make further tests tomorrow. Cheers, Benjamin On Thu, Feb 24, 2011 at 19:30, Henrik Rydberg <rydberg@euromail.se> wrote: > When the multi input quirk is set, there is a new input device > created for every feature report. Since the idea is to present > features per hid device, not per input device, revert back to > the original report loop and change the feature_mapping() callback > to not take the input device as argument. > > Signed-off-by: Henrik Rydberg <rydberg@euromail.se> > --- > Hi Jiri, Benjamin, > > It seems the feature report callback was a bit intrusive, after all; > for some devices such as ntrig, with multi input, there are additional > input devices created. The following patch seems to fix the problem, > but it has not been tested on any device using the hid-multitouch > driver. > > Thanks, > Henrik > > drivers/hid/hid-input.c | 30 +++++++++++++++++++++--------- > drivers/hid/hid-multitouch.c | 2 +- > include/linux/hid.h | 2 +- > 3 files changed, 23 insertions(+), 11 deletions(-) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index 7f552bf..ebcc02a 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -290,14 +290,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > goto ignore; > } > > - if (field->report_type == HID_FEATURE_REPORT) { > - if (device->driver->feature_mapping) { > - device->driver->feature_mapping(device, hidinput, field, > - usage); > - } > - goto ignore; > - } > - > if (device->driver->input_mapping) { > int ret = device->driver->input_mapping(device, hidinput, field, > usage, &bit, &max); > @@ -835,6 +827,24 @@ static void hidinput_close(struct input_dev *dev) > hid_hw_close(hid); > } > > +static void report_features(struct hid_device *hid) > +{ > + struct hid_driver *drv = hid->driver; > + struct hid_report_enum *rep_enum; > + struct hid_report *rep; > + int i, j; > + > + if (!drv->feature_mapping) > + return; > + > + rep_enum = &hid->report_enum[HID_FEATURE_REPORT]; > + list_for_each_entry(rep, &rep_enum->report_list, list) > + for (i = 0; i < rep->maxfield; i++) > + for (j = 0; j < rep->field[i]->maxusage; j++) > + drv->feature_mapping(hid, rep->field[i], > + rep->field[i]->usage + j); > +} > + > /* > * Register the input device; print a message. > * Configure the input layer interface > @@ -863,7 +873,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force) > return -1; > } > > - for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) { > + report_features(hid); > + > + for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) { > if (k == HID_OUTPUT_REPORT && > hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS) > continue; > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > index 07d3183..2bbc954 100644 > --- a/drivers/hid/hid-multitouch.c > +++ b/drivers/hid/hid-multitouch.c > @@ -122,7 +122,7 @@ struct mt_class mt_classes[] = { > { } > }; > > -static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi, > +static void mt_feature_mapping(struct hid_device *hdev, > struct hid_field *field, struct hid_usage *usage) > { > if (usage->hid == HID_DG_INPUTMODE) { > diff --git a/include/linux/hid.h b/include/linux/hid.h > index d91c25e..fc5faf6 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -638,7 +638,7 @@ struct hid_driver { > struct hid_input *hidinput, struct hid_field *field, > struct hid_usage *usage, unsigned long **bit, int *max); > void (*feature_mapping)(struct hid_device *hdev, > - struct hid_input *hidinput, struct hid_field *field, > + struct hid_field *field, > struct hid_usage *usage); > #ifdef CONFIG_PM > int (*suspend)(struct hid_device *hdev, pm_message_t message); > -- > 1.7.4.1 > > -- > 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 > -- 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 Thu, Feb 24, 2011 at 08:50:55PM +0100, Benjamin Tissoires wrote: > Hi Henrik, > > Thanks for spotting this. BTW, I'm not happy with your solution. > > You are sending the feature report before creating the struct > hid_input. To be consistent with the rest, we have to keep the same > signature. Today, the code does not make any use of it. But I use it > in my devel branch to auto-detect the maximum contact count of the > device. This was the safest place to call input_mt_init_slots. Well, the whole point is "which input device". It would only be well-defined when the hid device has a single input device. The feature callback could be called last, however, if that helps. > How about adding hidinput as an argument to report_features > > and calling it after the " for (k = HID_INPUT_REPORT; k <= > HID_OUTPUT_REPORT; k++) {" loop with > > list_for_each_entry_safe(hidinput, next, &hid->inputs, list) > report_features(hid, hidinput); > > I did not even try to compile it right now (I don't have any > multitouch device right now) but I'll be able to make further tests > tomorrow. Thanks, Henrik
Hi Henrik,
after some quick tests, I can deal with our two options (changing
feature_mapping signature or not, and so calling feature_mapping
before or after input_mapping).
So, my questions are:
- do we really need to change feature_mapping signature?
- is feature_mapping tied to an input or to a device?
Cheers,
Benjamin
On Thu, Feb 24, 2011 at 21:43, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Thu, Feb 24, 2011 at 08:50:55PM +0100, Benjamin Tissoires wrote:
>> Hi Henrik,
>>
>> Thanks for spotting this. BTW, I'm not happy with your solution.
>>
>> You are sending the feature report before creating the struct
>> hid_input. To be consistent with the rest, we have to keep the same
>> signature. Today, the code does not make any use of it. But I use it
>> in my devel branch to auto-detect the maximum contact count of the
>> device. This was the safest place to call input_mt_init_slots.
>
> Well, the whole point is "which input device". It would only be
> well-defined when the hid device has a single input device. The
> feature callback could be called last, however, if that helps.
>
>> How about adding hidinput as an argument to report_features
>>
>> and calling it after the " for (k = HID_INPUT_REPORT; k <=
>> HID_OUTPUT_REPORT; k++) {" loop with
>>
>> list_for_each_entry_safe(hidinput, next, &hid->inputs, list)
>> report_features(hid, hidinput);
>>
>> I did not even try to compile it right now (I don't have any
>> multitouch device right now) but I'll be able to make further tests
>> tomorrow.
>
> 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, after some quick tests, I can deal with our two options (changing feature_mapping signature or not, and so calling feature_mapping before or after input_mapping). So, my questions are: - do we really need to change feature_mapping signature? - is feature_mapping tied to an input or to a device? Cheers, Benjamin On Thu, Feb 24, 2011 at 21:43, Henrik Rydberg <rydberg@euromail.se> wrote: > On Thu, Feb 24, 2011 at 08:50:55PM +0100, Benjamin Tissoires wrote: >> Hi Henrik, >> >> Thanks for spotting this. BTW, I'm not happy with your solution. >> >> You are sending the feature report before creating the struct >> hid_input. To be consistent with the rest, we have to keep the same >> signature. Today, the code does not make any use of it. But I use it >> in my devel branch to auto-detect the maximum contact count of the >> device. This was the safest place to call input_mt_init_slots. > > Well, the whole point is "which input device". It would only be > well-defined when the hid device has a single input device. The > feature callback could be called last, however, if that helps. > >> How about adding hidinput as an argument to report_features >> >> and calling it after the " for (k = HID_INPUT_REPORT; k <= >> HID_OUTPUT_REPORT; k++) {" loop with >> >> list_for_each_entry_safe(hidinput, next, &hid->inputs, list) >> report_features(hid, hidinput); >> >> I did not even try to compile it right now (I don't have any >> multitouch device right now) but I'll be able to make further tests >> tomorrow. > > 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 > -- 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, > after some quick tests, I can deal with our two options (changing > feature_mapping signature or not, and so calling feature_mapping > before or after input_mapping). Good, good. > So, my questions are: > - do we really need to change feature_mapping signature? > - is feature_mapping tied to an input or to a device? The input, output and feature reports are all found on the same level in the HID protocol, so it makes sense to associate all reports with the device itself, without any assumed association between different reports. From a practical point of view, we may assign different input nodes (input devices) to different input reports, so it is clear that the mapping between hid device and input device is not 1-to-1. For output devices, the only supported case is EV_LED, which passes events to the input device. It is probably assumed that HID_QUIRK_MULTI_INPUT is false for those devices. Jiri? For feature reports, the lack of 1-1 correspondence suggests the input device is ill-defined, and should therefore be left out of the argument list of the callback. However, I will leave that to Jiri or anyone more experienced with the HID layer. Thanks, Henrik
On Thu, 24 Feb 2011, Henrik Rydberg wrote: > When the multi input quirk is set, there is a new input device > created for every feature report. Since the idea is to present > features per hid device, not per input device, revert back to > the original report loop and change the feature_mapping() callback > to not take the input device as argument. > > Signed-off-by: Henrik Rydberg <rydberg@euromail.se> Hi Henrik, I agree with this implementation. Benjamin, did you manage to do the tests with hid-multitouch driver so that I could put your Tested-by: to the patch and apply it? Thanks. > --- > Hi Jiri, Benjamin, > > It seems the feature report callback was a bit intrusive, after all; > for some devices such as ntrig, with multi input, there are additional > input devices created. The following patch seems to fix the problem, > but it has not been tested on any device using the hid-multitouch > driver. > > Thanks, > Henrik > > drivers/hid/hid-input.c | 30 +++++++++++++++++++++--------- > drivers/hid/hid-multitouch.c | 2 +- > include/linux/hid.h | 2 +- > 3 files changed, 23 insertions(+), 11 deletions(-) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index 7f552bf..ebcc02a 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -290,14 +290,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > goto ignore; > } > > - if (field->report_type == HID_FEATURE_REPORT) { > - if (device->driver->feature_mapping) { > - device->driver->feature_mapping(device, hidinput, field, > - usage); > - } > - goto ignore; > - } > - > if (device->driver->input_mapping) { > int ret = device->driver->input_mapping(device, hidinput, field, > usage, &bit, &max); > @@ -835,6 +827,24 @@ static void hidinput_close(struct input_dev *dev) > hid_hw_close(hid); > } > > +static void report_features(struct hid_device *hid) > +{ > + struct hid_driver *drv = hid->driver; > + struct hid_report_enum *rep_enum; > + struct hid_report *rep; > + int i, j; > + > + if (!drv->feature_mapping) > + return; > + > + rep_enum = &hid->report_enum[HID_FEATURE_REPORT]; > + list_for_each_entry(rep, &rep_enum->report_list, list) > + for (i = 0; i < rep->maxfield; i++) > + for (j = 0; j < rep->field[i]->maxusage; j++) > + drv->feature_mapping(hid, rep->field[i], > + rep->field[i]->usage + j); > +} > + > /* > * Register the input device; print a message. > * Configure the input layer interface > @@ -863,7 +873,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force) > return -1; > } > > - for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) { > + report_features(hid); > + > + for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) { > if (k == HID_OUTPUT_REPORT && > hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS) > continue; > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c > index 07d3183..2bbc954 100644 > --- a/drivers/hid/hid-multitouch.c > +++ b/drivers/hid/hid-multitouch.c > @@ -122,7 +122,7 @@ struct mt_class mt_classes[] = { > { } > }; > > -static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi, > +static void mt_feature_mapping(struct hid_device *hdev, > struct hid_field *field, struct hid_usage *usage) > { > if (usage->hid == HID_DG_INPUTMODE) { > diff --git a/include/linux/hid.h b/include/linux/hid.h > index d91c25e..fc5faf6 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -638,7 +638,7 @@ struct hid_driver { > struct hid_input *hidinput, struct hid_field *field, > struct hid_usage *usage, unsigned long **bit, int *max); > void (*feature_mapping)(struct hid_device *hdev, > - struct hid_input *hidinput, struct hid_field *field, > + struct hid_field *field, > struct hid_usage *usage); > #ifdef CONFIG_PM > int (*suspend)(struct hid_device *hdev, pm_message_t message); > -- > 1.7.4.1 > -- Jiri Kosina SUSE Labs, Novell Inc.
Hi Jiri,
Tested-by: Benjamin Tissoires <benjmain.tissoires@gmail.com>
Acked-by: Benjamin Tissoires <benjmain.tissoires@gmail.com>
Cheers,
Benjamin
On Tue, Mar 1, 2011 at 17:03, Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 24 Feb 2011, Henrik Rydberg wrote:
>
>> When the multi input quirk is set, there is a new input device
>> created for every feature report. Since the idea is to present
>> features per hid device, not per input device, revert back to
>> the original report loop and change the feature_mapping() callback
>> to not take the input device as argument.
>>
>> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
>
> Hi Henrik,
>
> I agree with this implementation.
>
> Benjamin, did you manage to do the tests with hid-multitouch driver so
> that I could put your Tested-by: to the patch and apply it?
>
> Thanks.
>
>> ---
>> Hi Jiri, Benjamin,
>>
>> It seems the feature report callback was a bit intrusive, after all;
>> for some devices such as ntrig, with multi input, there are additional
>> input devices created. The following patch seems to fix the problem,
>> but it has not been tested on any device using the hid-multitouch
>> driver.
>>
>> Thanks,
>> Henrik
>>
>> drivers/hid/hid-input.c | 30 +++++++++++++++++++++---------
>> drivers/hid/hid-multitouch.c | 2 +-
>> include/linux/hid.h | 2 +-
>> 3 files changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 7f552bf..ebcc02a 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -290,14 +290,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>> goto ignore;
>> }
>>
>> - if (field->report_type == HID_FEATURE_REPORT) {
>> - if (device->driver->feature_mapping) {
>> - device->driver->feature_mapping(device, hidinput, field,
>> - usage);
>> - }
>> - goto ignore;
>> - }
>> -
>> if (device->driver->input_mapping) {
>> int ret = device->driver->input_mapping(device, hidinput, field,
>> usage, &bit, &max);
>> @@ -835,6 +827,24 @@ static void hidinput_close(struct input_dev *dev)
>> hid_hw_close(hid);
>> }
>>
>> +static void report_features(struct hid_device *hid)
>> +{
>> + struct hid_driver *drv = hid->driver;
>> + struct hid_report_enum *rep_enum;
>> + struct hid_report *rep;
>> + int i, j;
>> +
>> + if (!drv->feature_mapping)
>> + return;
>> +
>> + rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
>> + list_for_each_entry(rep, &rep_enum->report_list, list)
>> + for (i = 0; i < rep->maxfield; i++)
>> + for (j = 0; j < rep->field[i]->maxusage; j++)
>> + drv->feature_mapping(hid, rep->field[i],
>> + rep->field[i]->usage + j);
>> +}
>> +
>> /*
>> * Register the input device; print a message.
>> * Configure the input layer interface
>> @@ -863,7 +873,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>> return -1;
>> }
>>
>> - for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) {
>> + report_features(hid);
>> +
>> + for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) {
>> if (k == HID_OUTPUT_REPORT &&
>> hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS)
>> continue;
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 07d3183..2bbc954 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -122,7 +122,7 @@ struct mt_class mt_classes[] = {
>> { }
>> };
>>
>> -static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi,
>> +static void mt_feature_mapping(struct hid_device *hdev,
>> struct hid_field *field, struct hid_usage *usage)
>> {
>> if (usage->hid == HID_DG_INPUTMODE) {
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index d91c25e..fc5faf6 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -638,7 +638,7 @@ struct hid_driver {
>> struct hid_input *hidinput, struct hid_field *field,
>> struct hid_usage *usage, unsigned long **bit, int *max);
>> void (*feature_mapping)(struct hid_device *hdev,
>> - struct hid_input *hidinput, struct hid_field *field,
>> + struct hid_field *field,
>> struct hid_usage *usage);
>> #ifdef CONFIG_PM
>> int (*suspend)(struct hid_device *hdev, pm_message_t message);
>> --
>> 1.7.4.1
>>
>
> --
> Jiri Kosina
> SUSE Labs, Novell Inc.
> --
> 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 Jiri, Tested-by: Benjamin Tissoires <benjmain.tissoires@gmail.com> Acked-by: Benjamin Tissoires <benjmain.tissoires@gmail.com> Cheers, Benjamin On Tue, Mar 1, 2011 at 17:03, Jiri Kosina <jkosina@suse.cz> wrote: > On Thu, 24 Feb 2011, Henrik Rydberg wrote: > >> When the multi input quirk is set, there is a new input device >> created for every feature report. Since the idea is to present >> features per hid device, not per input device, revert back to >> the original report loop and change the feature_mapping() callback >> to not take the input device as argument. >> >> Signed-off-by: Henrik Rydberg <rydberg@euromail.se> > > Hi Henrik, > > I agree with this implementation. > > Benjamin, did you manage to do the tests with hid-multitouch driver so > that I could put your Tested-by: to the patch and apply it? > > Thanks. > >> --- >> Hi Jiri, Benjamin, >> >> It seems the feature report callback was a bit intrusive, after all; >> for some devices such as ntrig, with multi input, there are additional >> input devices created. The following patch seems to fix the problem, >> but it has not been tested on any device using the hid-multitouch >> driver. >> >> Thanks, >> Henrik >> >> drivers/hid/hid-input.c | 30 +++++++++++++++++++++--------- >> drivers/hid/hid-multitouch.c | 2 +- >> include/linux/hid.h | 2 +- >> 3 files changed, 23 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c >> index 7f552bf..ebcc02a 100644 >> --- a/drivers/hid/hid-input.c >> +++ b/drivers/hid/hid-input.c >> @@ -290,14 +290,6 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel >> goto ignore; >> } >> >> - if (field->report_type == HID_FEATURE_REPORT) { >> - if (device->driver->feature_mapping) { >> - device->driver->feature_mapping(device, hidinput, field, >> - usage); >> - } >> - goto ignore; >> - } >> - >> if (device->driver->input_mapping) { >> int ret = device->driver->input_mapping(device, hidinput, field, >> usage, &bit, &max); >> @@ -835,6 +827,24 @@ static void hidinput_close(struct input_dev *dev) >> hid_hw_close(hid); >> } >> >> +static void report_features(struct hid_device *hid) >> +{ >> + struct hid_driver *drv = hid->driver; >> + struct hid_report_enum *rep_enum; >> + struct hid_report *rep; >> + int i, j; >> + >> + if (!drv->feature_mapping) >> + return; >> + >> + rep_enum = &hid->report_enum[HID_FEATURE_REPORT]; >> + list_for_each_entry(rep, &rep_enum->report_list, list) >> + for (i = 0; i < rep->maxfield; i++) >> + for (j = 0; j < rep->field[i]->maxusage; j++) >> + drv->feature_mapping(hid, rep->field[i], >> + rep->field[i]->usage + j); >> +} >> + >> /* >> * Register the input device; print a message. >> * Configure the input layer interface >> @@ -863,7 +873,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force) >> return -1; >> } >> >> - for (k = HID_INPUT_REPORT; k <= HID_FEATURE_REPORT; k++) { >> + report_features(hid); >> + >> + for (k = HID_INPUT_REPORT; k <= HID_OUTPUT_REPORT; k++) { >> if (k == HID_OUTPUT_REPORT && >> hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORTS) >> continue; >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c >> index 07d3183..2bbc954 100644 >> --- a/drivers/hid/hid-multitouch.c >> +++ b/drivers/hid/hid-multitouch.c >> @@ -122,7 +122,7 @@ struct mt_class mt_classes[] = { >> { } >> }; >> >> -static void mt_feature_mapping(struct hid_device *hdev, struct hid_input *hi, >> +static void mt_feature_mapping(struct hid_device *hdev, >> struct hid_field *field, struct hid_usage *usage) >> { >> if (usage->hid == HID_DG_INPUTMODE) { >> diff --git a/include/linux/hid.h b/include/linux/hid.h >> index d91c25e..fc5faf6 100644 >> --- a/include/linux/hid.h >> +++ b/include/linux/hid.h >> @@ -638,7 +638,7 @@ struct hid_driver { >> struct hid_input *hidinput, struct hid_field *field, >> struct hid_usage *usage, unsigned long **bit, int *max); >> void (*feature_mapping)(struct hid_device *hdev, >> - struct hid_input *hidinput, struct hid_field *field, >> + struct hid_field *field, >> struct hid_usage *usage); >> #ifdef CONFIG_PM >> int (*suspend)(struct hid_device *hdev, pm_message_t message); >> -- >> 1.7.4.1 >> > > -- > Jiri Kosina > SUSE Labs, Novell Inc. > -- > 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 > -- 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 Tue, 1 Mar 2011, Benjamin Tissoires wrote:
> Tested-by: Benjamin Tissoires <benjmain.tissoires@gmail.com>
> Acked-by: Benjamin Tissoires <benjmain.tissoires@gmail.com>
Applied, thanks.
--
Jiri Kosina
SUSE Labs, Novell Inc.
On Fri, Feb 25, 2011 at 06:19:11PM +0100, Henrik Rydberg wrote:
> Hi Benjamin,
>
> > after some quick tests, I can deal with our two options (changing
> > feature_mapping signature or not, and so calling feature_mapping
> > before or after input_mapping).
>
> Good, good.
>
> > So, my questions are:
> > - do we really need to change feature_mapping signature?
> > - is feature_mapping tied to an input or to a device?
>
> The input, output and feature reports are all found on the same level
> in the HID protocol, so it makes sense to associate all reports with
> the device itself, without any assumed association between different
> reports. From a practical point of view, we may assign different input
> nodes (input devices) to different input reports, so it is clear that
> the mapping between hid device and input device is not 1-to-1.
>
> For output devices, the only supported case is EV_LED, which passes
> events to the input device. It is probably assumed that
> HID_QUIRK_MULTI_INPUT is false for those devices. Jiri?
>
I am probably late to the party fut the above is not true. Here is an
example of an USB keyboard (wih LEDs) that is split into two:
I: Bus=0003 Vendor=046d Product=c30e Version=0110
N: Name="Logitech HID compliant keyboard"
P: Phys=usb-0000:00:1d.0-1.2/input0
S:
Sysfs=/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/input/input3
U: Uniq=
H: Handlers=sysrq kbd event3
B: PROP=0
B: EV=120013
B: KEY=1000000000007 ff800000000007ff febeffdff3cfffff fffffffffffffffe
B: MSC=10
B: LED=7
I: Bus=0003 Vendor=046d Product=c30e Version=0110
N: Name="Logitech HID compliant keyboard"
P: Phys=usb-0000:00:1d.0-1.2/input1
S:
Sysfs=/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.1/input/input4
U: Uniq=
H: Handlers=kbd event4
B: PROP=0
B: EV=13
B: KEY=fff ffffffffffffffff 2000000 387ad800d001 1e000000000000 0
B: MSC=10
This was done, most likely, because Logitech decided to reuse usage codes
for different keys.
Thanks.
--
Dmitry
Hi Dmitry,
> > For output devices, the only supported case is EV_LED, which passes
> > events to the input device. It is probably assumed that
> > HID_QUIRK_MULTI_INPUT is false for those devices. Jiri?
> >
>
> I am probably late to the party fut the above is not true. Here is an
> example of an USB keyboard (wih LEDs) that is split into two:
>
> I: Bus=0003 Vendor=046d Product=c30e Version=0110
> N: Name="Logitech HID compliant keyboard"
> P: Phys=usb-0000:00:1d.0-1.2/input0
> S:
> Sysfs=/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/input/input3
> U: Uniq=
> H: Handlers=sysrq kbd event3
> B: PROP=0
> B: EV=120013
> B: KEY=1000000000007 ff800000000007ff febeffdff3cfffff fffffffffffffffe
> B: MSC=10
> B: LED=7
>
> I: Bus=0003 Vendor=046d Product=c30e Version=0110
> N: Name="Logitech HID compliant keyboard"
> P: Phys=usb-0000:00:1d.0-1.2/input1
> S:
> Sysfs=/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.1/input/input4
> U: Uniq=
> H: Handlers=kbd event4
> B: PROP=0
> B: EV=13
> B: KEY=fff ffffffffffffffff 2000000 387ad800d001 1e000000000000 0
> B: MSC=10
>
> This was done, most likely, because Logitech decided to reuse usage codes
> for different keys.
This looks like different interfaces though, which should be fine. It
is only in the odd case of mixed input and output reports on the same
interface that the MULTI_INPUT quirk would ever have any strange
effect.
Cheers,
Henrik
On Wed, Mar 02, 2011 at 04:02:27PM +0100, Henrik Rydberg wrote:
> Hi Dmitry,
>
> > > For output devices, the only supported case is EV_LED, which passes
> > > events to the input device. It is probably assumed that
> > > HID_QUIRK_MULTI_INPUT is false for those devices. Jiri?
> > >
> >
> > I am probably late to the party fut the above is not true. Here is an
> > example of an USB keyboard (wih LEDs) that is split into two:
> >
> > I: Bus=0003 Vendor=046d Product=c30e Version=0110
> > N: Name="Logitech HID compliant keyboard"
> > P: Phys=usb-0000:00:1d.0-1.2/input0
> > S:
> > Sysfs=/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.0/input/input3
> > U: Uniq=
> > H: Handlers=sysrq kbd event3
> > B: PROP=0
> > B: EV=120013
> > B: KEY=1000000000007 ff800000000007ff febeffdff3cfffff fffffffffffffffe
> > B: MSC=10
> > B: LED=7
> >
> > I: Bus=0003 Vendor=046d Product=c30e Version=0110
> > N: Name="Logitech HID compliant keyboard"
> > P: Phys=usb-0000:00:1d.0-1.2/input1
> > S:
> > Sysfs=/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.2/2-1.2:1.1/input/input4
> > U: Uniq=
> > H: Handlers=kbd event4
> > B: PROP=0
> > B: EV=13
> > B: KEY=fff ffffffffffffffff 2000000 387ad800d001 1e000000000000 0
> > B: MSC=10
> >
> > This was done, most likely, because Logitech decided to reuse usage codes
> > for different keys.
>
> This looks like different interfaces though, which should be fine. It
> is only in the odd case of mixed input and output reports on the same
> interface that the MULTI_INPUT quirk would ever have any strange
> effect.
>
Gah, when I looked at it before posting I could swore they were on the
same interface. OK, just ignore me...
Thanks.
--
Dmitry
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 A bit on the late side, I can confirm this reverts the annoying proliferation of input nodes mapped for ntrig hardware. The new firmwares seem to really enjoy exposing bugs, more on that later. Rafi On 03/01/11 11:26, Jiri Kosina wrote: > On Tue, 1 Mar 2011, Benjamin Tissoires wrote: > >> Tested-by: Benjamin Tissoires <benjmain.tissoires@gmail.com> >> Acked-by: Benjamin Tissoires <benjmain.tissoires@gmail.com> > > Applied, thanks. > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQIcBAEBAgAGBQJNdaYCAAoJEPILXytRLnK214MP/RC61mIowTq/2o4oWp8JQvy3 l7zZumg3v/tInK7OS1XciWzMQl+zH09FfzutgAEULO7hGhN1RFiP8qiBHtuMrMVg 80FWXIl24SykGe/MwgQPi9esipTx6ee8W7hmIpFOfyVIZIl/F4afdPe3a9fFhuH+ 0H5yn7+tUw5n7wglD57xkeDW4a3RD3ehf++ehZhmmwWoPKhr9119XN9UDAmHRBX4 1DfdtOTtsVA161DJkiMipEO5KoeD6dH1VMiCBhxkn+WLpVPHP6zhV/3KUhIDEFDd oWMvldltbpe8UHQmh2KwiwrtJib0bLIrrmzS5EZkVm8bT2EFlM6ZsYt5tkp6NDwl /bD1gBrMd14KGwVjb7XflfprlUHbbgpvjDUF7L5B3I955LWsowFoFqjquoV6+Phj KOJRbUABvDa6esRpg79aAjD59Uh3adnmFydFFcHBHNc3Kb6ROJ4LVGlR1Zxt+QoT rEXEYuxPZwxJ0zZ08yO2Si+FaoPVOrZC7S2XNhv64MMpISGBWEIvRWowDPjpgutY v2aVdGiWskPsjqxjVXyR9MTqOgdXrp2JoZy2U2Jq7AV3vzwQfoJ+BJjgEAwkVmO3 XpKuxJ2s5vYPTlxPga3PqUPT2r7PzfjakTcXM4UQ3f7iQy4RZ4qr4q6+4aI3saVy kvTrsCQV6SmmbbZuaFLh =h0h3 -----END PGP SIGNATURE-----
On Tue, Mar 01, 2011 at 05:26:41PM +0100, Jiri Kosina wrote:
> On Tue, 1 Mar 2011, Benjamin Tissoires wrote:
>
> > Tested-by: Benjamin Tissoires <benjmain.tissoires@gmail.com>
> > Acked-by: Benjamin Tissoires <benjmain.tissoires@gmail.com>
>
> Applied, thanks.
Hi Jiri,
I was wondering if this patch will appear in the final 2.6.38? It
seems the regression it fixes is pretty severe for ntrig devices, and
once out there, the callback argument change will create problems for
add-on modules such as dkms modules.
Thanks,
Henrik
On Thu, 10 Mar 2011, Henrik Rydberg wrote:
> > > Tested-by: Benjamin Tissoires <benjmain.tissoires@gmail.com>
> > > Acked-by: Benjamin Tissoires <benjmain.tissoires@gmail.com>
> >
> > Applied, thanks.
>
> Hi Jiri,
>
> I was wondering if this patch will appear in the final 2.6.38? It
> seems the regression it fixes is pretty severe for ntrig devices, and
> once out there, the callback argument change will create problems for
> add-on modules such as dkms modules.
Hi Henrik,
I am not sure Linus will be taking it for .38 still, as he is now offline
and will be releasing final .38 after he appears, so it's unlikely that
he'll take pull requests unless it's something super-severe.
I'll put it on my list of commits to be pushed out to 38-stable right
away. Does that make sense?
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
> > I was wondering if this patch will appear in the final 2.6.38? It
> > seems the regression it fixes is pretty severe for ntrig devices, and
> > once out there, the callback argument change will create problems for
> > add-on modules such as dkms modules.
>
> Hi Henrik,
>
> I am not sure Linus will be taking it for .38 still, as he is now offline
> and will be releasing final .38 after he appears, so it's unlikely that
> he'll take pull requests unless it's something super-severe.
>
> I'll put it on my list of commits to be pushed out to 38-stable right
> away. Does that make sense?
Yes, that will be great. Thanks!
Henrik