Hi,
On 2/24/21 10:59 PM, Alexander Kobel wrote:
> 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.
Great, I've submitted a new version of my previous hid-lenovo series upstream now, with the 2 patches to add support for the X1 tablet keyboard added to the series.
Regards,
Hans
