Hi,
On 2/24/21 10:00 PM, Hans de Goede wrote:
> Hi,
>
> On 2/22/21 1:31 PM, Alexander Kobel wrote:
>> On 2/21/21 5:30 PM, Hans de Goede wrote:
<snip>
>>> The input report used by the Fn + key "media keys" use a 24 bit report
>>> with 1 bit per key. The standard keyboard interface uses 1 byte per
>>> pressed key (with a maximum of 6 pressed keys) where the full byte
>>> encodes the scancode of the key. Normally SysRq is 0x46 but for some
>>> reason your keyboard is sending 0x9a you can map this by adding the following
>>> to the mapping function:
>>>
>>> if (usage->hid == (HID_UP_KEYBOARD | 0x009a))
>>> map_key_clear(KEY_SYSRQ);
>>
>> And the same here, I think. Works with return 1; after the map_key_clear, see the attached function.
>
> Ah yes, I forgot the return 1, sorry about that.
No problem, nearby pattern matching and copy-paste is a great way to learn sometimes. ;-)
>>> Likewise for the sleep combo:
>>>
>>> if (usage->hid == (HID_UP_KEYBOARD | 0x0072))
>>> map_key_clear(KEY_SLEEP);
>>
>> This seems unnecessary, as the sleep combo already emits KEY_SLEEP. Which I don't quite get, cause - if I learned correctly how to read the rdesc - sleep should be on 0x0072 should emit F23 (and 0x0078 sleep), but the key produces the 0x0072 pattern according to hidraw.
>> By the way, the Fn+4 for sleep also works in hid-generic, and also just once, see below.
>
> Ok.
>
>>> ###
>>>
>>> Note chances are you have more Fn + 'letter' combinations at least on
>>> the thinkpad10 kbd I have:
>>>
>>> Fn + T -> SysRq
>>> Fn + I -> Insert
>>> Fn + P -> Pause
>>> Fn + S -> Sysrq
>>> Fn + K -> ScrollLock
>>> Fn + B -> Pause
>>>
>>> Note these do not need any special mappings on the thinkpad10 kbd and I guess
>>> the doubles may have something to do with some non qwerty keymaps.
>>
>> Not the same on this keyboard. I have Fn + {B,K,P,S,End,4} for {Break,ScrLk,Pause,SysRq,Insert,Sleep}, but only SysRq was missing; the others are available on the "default" device, both with hid-lenovo and hid-generic.
>
> Ok.
>
>>>> The ugly:
>>>>
>>>> Fn+4 ("sleep") triggers the appropriate ACPI event button/sleep and emits something on /dev/hidraw0, too, but *only once*. After resuming, no reaction at all (neither on ACPI nor hidraw) until I unload and reload the hid_lenovo module.
>>>
>>> As for the sleep key working only once, what happens after a suspend/resume ?
>>
>> Nothing. At least nothing I can measure (via evtest, libinput debug-events, cat /dev/hidraw*, dmesg, journal, acpi_listen). The key simply doesn't react anymore.
>> Irrespective on how I wake the device. Even irrespective of whether I actually suspend the device or disable the sleep handler altogether, e.g. by systemd-inhibit. In only see the KEY_SLEEP press (no release!) event once until I reload the module or unplug and reattach the keyboard.
>> Very strange. And, by the way: the same for hid-generic.
>>
>>> I think the key may have some special handling to avoid it sending
>>> a second KEY_SLEEP when the user uses it to wakeup the system, to
>>> avoid the system immediately going to sleep again when the user tries
>>> to wake the system this way.
>>
>> Yes, that'd make sense. So probably the handler should restore something; apparently, that part is initialized when the module is loaded, so it's not just the keyboard firmware.
>
> Maybe echo-ing to the fnlock attribute resets the key ? The driver does always force the fnlock LED on when it is loaded.
Good catch, but that doesn't help, either.
It's only a minor nuisance, though; in any case, it can be worked around by reloading the driver in a resume hook. If one actually wants to use that button; I personally won't.
>>>> Finally, keyboard backlight is handled in firmware, apparently; Fn+Space is visible on /dev/hidraw1 (see attached capture), but it toggles the backlight levels without any userspace code involved, as far as I can see.
>>>
>>> Yes I saw this in your dump, this is really weird because it sets 3 bits at once in the INPUT(3) report.
>>> Does it always set the same 3 bits independent of the brightness level ?
>>
>> Yes; the four key presses are actually one full cycle over off, auto, dim, and bright. hidraw dumps are identical.
>>
>>>> Also, the keyboard backlight doesn't create an entry in /sys/class/backlight or the like, so neither read nor write access. Out-of-the-box, at least. But I'm not even sure if this is possible in Windows.
>>>
>>> Actuallu kbd-backlighting used the /sys/class/leds interface, but yeah that is not support by Linux ATM for this kbd.
>>>
>>> Yeah, adding support for that (assuming the hw can do it) would definitely require making some USB dumps under Windows
>>> (after finding sw which can change it from within the OS under Windows).
>>
>> I double-checked; somewhat unsurprisingly, there is at least a notification client on Windows that displays the new configuration after Fn+Space. The client only reports the setting; not sure if it would be technically feasible to also set the brightness level in software.
>
> We could try asking Nitin if he has any info about this, but I agree that this is a low priority item.
Ack.
>> But anyways: I feel that I exploited your generosity in helping me far enough. I hope I wasn't just fed, but learned a bit how to fish for myself in the future. So, unless you're really committed to walk me through this further, I won't beg any further.
>>
>> And in case you're looking for problems to tackle on the Tablet 2nd Gen, this one about the power button could be way more significant - but not sure if it's in an area that you are familiar in:
>> https://bbs.archlinux.org/viewtopic.php?id=248857
>> https://bugzilla.kernel.org/show_bug.cgi?id=204763
>
> I see that you have already tested the patch which was posted for this, so I assume that this is resolved now ?
Correct. I resurrected the bugzilla task shortly after my last mail, and Alban cranked out a patch with you in CC within few hours. Didn't want to add more noise here.
>>>> Bottom line: this is mostly usable already, modulo the adjustments for the different keys. I'd like to make F10 to F12 work before it hits testing; everything else is icing on the cake, I suppose. Do you have an hint for me how I can approach that?
>>>
>>> See above, I think we are pretty close to solving this.
>>
>> Attached is the modified version of the input mapping for this keyboard, working subject to the above mentioned restrictions. I think this is fine.
>> As you did all the real work, I feel this should be your contribution. But of course I can also prepare a patch on top of yours.
>
> I think you're underestimating your own contribution here...
>
> For cases like this we usually add a co-authored tag. Since this applies on top of another hid-lenovo series which I recently send out it is probably easier if I upstream this, that I agree on.
>
> I would like to attribute your work though, so I would like to suggest adding the following 2 tags to the commit msg for
> the "HID: lenovo: Add support for Thinkpad X1 Tablet Thin keyboard" patch:
>
> Co-authored-by: Alexander Kobel <a-kobel@xxxxxxxxxx>
> Signed-off-by: Alexander Kobel <a-kobel@xxxxxxxxxx>
>
> Alternatively
<snip>
> But I believe that co-authored-by + s-o-b are more appropriate.
Okay, so be it. I trust your opinion here.
> If you can let me know which one you prefer, then I will drop in your fixed lenovo_input_mapping_x1_tab_kbd() function, remove the WIP from the commit subject and submit the last 2 patches of the set which I send you upstream (the rest was already submitted earlier).
co-authored-by + s-o-b it is then. Ack for the rest.
>>> Note in the mean time I've posted a hid-lenovo patch-series with various improvements related to
>>> the LED handling. I'll send you an offlist mail with the latest patches so that you can base any work you do on top of those.
>>
>> Didn't have a look yet, but will do.
>>
>>>> Also, I'd make sure that this is about the "ThinkPad X1 Tablet Thin Keyboard **Gen 2**". The consumer functions are different for the **Gen 1** keyboard, so I would also adjust the function names. I do have an old Gen 1 keyboard lying around, but unfortunately it's either broken (it lights up shortly after attaching in Windows, but doesn't report keypresses at all, and pretends to be completely dead in Linux), or it's incompatible with my X1 Tablet 2nd Gen. So I cannot test how your patch might impact the Gen1, too... :-/
>>>
>>> I would expect the Gen1 to have a different product-id, so my patch shouldn't do anything.
>>
>> Right. In this light, perhaps the function should still be called lenovo_input_mapping_x1_tab2_kbd (note the "2")?
>
> Well drivers/hid/hid-ids.h has this:
>
> #define USB_DEVICE_ID_LENOVO_X1_COVER 0x6085
> #define USB_DEVICE_ID_LENOVO_X1_TAB 0x60a3
> #define USB_DEVICE_ID_LENOVO_X1_TAB3 0x60b5
>
> And I guess that the COVER might be the X1 gen1 product-id ?
AFAICS, yes; confirmed by a quick web search. So it'd be more apt to
#define USB_DEVICE_ID_LENOVO_X1_TAB 0x6085
#define USB_DEVICE_ID_LENOVO_X1_TAB2 0x60a3
#define USB_DEVICE_ID_LENOVO_X1_TAB3 0x60b5
but I have no way to really confirm right now, so I'd also be in favor to leave it as-is.
> Your working kbd is using the USB_DEVICE_ID_LENOVO_X1_TAB id. note not TAB2 just tab.
>
> I don't know, but it is not all that important really, we can always rename both the #define USB_DEVICE_ID_LENOVO_X1_TAB and the function later if there is a reason to do so.
Ack. Better have #defines and function names in sync and think about more interesting stuff. ;-)
>> Thanks a lot,
>
> You're welcome and thank you for helping with improving support for Linux on these devices.
Ditto!
Cheers,
Alex
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
