On Sun, Sep 14, 2014 at 12:45 PM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
> Hi
>
> On Sun, Aug 10, 2014 at 11:56 AM, Fredrik Hallenberg
> <megahallon@xxxxxxxxx> wrote:
>> Here is my attempt on a fix for bug 70181, please review it. It is
>> tested on my nordic Corsair K70, if this solution is deemed acceptable
>> I can ask some people with other problematic keyboards to test it.
>>
>> Signed-off-by: Fredrik Hallenberg <megahallon@xxxxxxxxx>
>> ---
>> drivers/hid/hid-input.c | 14 ++++++++++++++
>> include/linux/hid.h | 1 +
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 2619f7f..56429c0 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1085,6 +1085,20 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>> return;
>> }
>>
>> + /*
>> + * Some keyboards will report both HID keys 0x31 (\ and |) and
>> + * 0x32 (Non-US # and ~) which are both mapped to
>> + * KEY_BACKSLASH. This will cause spurious events causing
>> + * repeated backslash key presses. Handle this by tracking the
>> + * active HID code and ignoring the other one.
>> + */
>> + if (usage->type == EV_KEY && usage->code == KEY_BACKSLASH) {
>> + if (value)
>> + field->hidinput->backslash_usage = usage->hid;
>> + else if (field->hidinput->backslash_usage != usage->hid)
>> + return;
>> + }
>> +
>
> Ok, I have looked through some more reports and at a bunch of AT
> keyboards and how the HID standard maps them. The backslash/pipe key
> is the only key affected by this. There _might_ be some similar
> overlap with Korean 106 and Japanese 109 layouts, but as there haven't
> been any reports, we probably don't care right now.
>
> And indeed, I'm kinda reluctant to add an hwdb entry for the reported
> keyboards as I couldn't find anyone with non-105 keyboards to compare
> VID/PID. Furthermore, similar issues will probably show up again in
> the future. However, I still dislike the quick hack in this patch. I
> especially dislike matching on KEY_BACKSLASH while we really want to
> match on 0x31 and 0x32. If users use setkeycode() to change a mapping,
> this should not trigger our quirk. We could easily change the patch to
> match on usage, but I think there's a better way:
>
> hid-core.c: hid_input_field()
> This is the main entry point for data that is matched on HID fields.
> We already have a RollOver quirk in there, which is extremely bad
> documented and I have no idea what it does.. However, this function
> saves the reported values so we can retrieve them on a following
> report. Why don't we skip events that have the same value reported as
> before? Obviously, we should do this only for absolute data, not
> relative. So I think something like this should work (sorry for
> line-wraps):
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 12b6e67..000a768 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1220,7 +1220,10 @@
> for (n = 0; n < count; n++) {
>
> if (HID_MAIN_ITEM_VARIABLE & field->flags) {
> - hid_process_event(hid, field,
> &field->usage[n], value[n], interrupt);
> + /* ignore absolute values if they didn't change */
> + if (HID_MAIN_ITEM_RELATIVE & field->flags ||
> + value[n] != field->value[n])
> + hid_process_event(hid, field,
> &field->usage[n], value[n], interrupt);
> continue;
> }
>
>
> @Jiri: Any comments on this? I now agree with Fredrik that this is
> better solved in HID core. It is a generic problem..
Jumping in the conversation here, but this is a strong NACK.
The multitouch handling sometimes suppose that there is a keep alive
on each touch, and when the keep alive stops in the report, then the
mt library releases the touch. If you stop sending these events, the
tip switch will not be sent twice, and then the slot will be
considered as released, and you broke many touchscreens.
Cheers,
Benjamin
>
> In HID, we save values per scancode / HID-field, additionally to
> input-core saving them per keycode / input-code. This patch basically
> makes use of this and avoids sending events for scan-codes that didn't
> change, but accidentally map to the same keycode as another key (which
> might have a different state than this one).
>
> If this looks reasonable, I can prepare a patch with a proper
> commit-log and give it a spin here.
>
> Thanks
> David
>
>> /* report the usage code as scancode if the key status has changed */
>> if (usage->type == EV_KEY && !!test_bit(usage->code, input->key) != value)
>> input_event(input, EV_MSC, MSC_SCAN, usage->hid);
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index f53c4a9..1c59f8f 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -448,6 +448,7 @@ struct hid_input {
>> struct list_head list;
>> struct hid_report *report;
>> struct input_dev *input;
>> + unsigned backslash_usage;
>> };
>>
>> enum hid_type {
>> --
>> 2.1.0.rc1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
