FYI I just got a "lsusb -v" dump from a US version of my keyboard, and
it is exactly the same as the one from my nordic version.
On Mon, Sep 15, 2014 at 12:53 AM, Fredrik Hallenberg
<megahallon@xxxxxxxxx> wrote:
> Thank you for this, I felt that it would be difficult to do a good
> solution with hwdb but could not really back this up. It is also nice
> to see a much better patch than mine, I did not have enough knowledge
> on the input system to do a proper one.
>
> On Sun, Sep 14, 2014 at 6: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..
>>
>> 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
