Hi Alexander, On 2/10/21 6:51 PM, Alexander Kobel wrote: > Hi Hans, Nitin, > > thanks a lot for your help. > > On 2/8/21 10:04 AM, Hans de Goede wrote: >> <snip> >> Alexander, so it looks like we need to do the following to support this properly: > > Spot on. Thanks for the detailed guide; I wouldn't have been able to get there in due time without your advise. > I will send the revised patch in a separate mail, but want to highlight few points. > >> 1. At a new TP_HOTKEY_TABLET_USES_GTOP to the hotkey_tablet enum >> 2. At probe time in hotkey_init_tablet_mode add a new if / case with I guess >> the highest prio (so try before GMMS) which does: >> 1. Call GTOP with parameter 0x0000, check return equals 0x0103 (or newer?) >> Nitin, how should the version check look like here, check that the >> upper byte == 0x01 and the lower byte >= 0x03 ? > > Exactly. > >> 2. Call GTOP with parameter 0x0100, check return value equals 4 > > I also included the simpler "any type" interface which does not report folding to the back. I confirmed that it works on my X1 with limited functionality, but don't have a "any type" device to test available. (In fact, I don't know whether such a device even exists.) IIUC it can also easily cover type 2 "Helix" and type 3 "Thinkpad 10" with the functionality available on those types. Since I cannot test, I didn't enable that in my patch; but it should be a matter of checking for return values 2 and 3 and adding an appropriate case in hotkey_get_tablet_mode. > By the way, Nitin kindly confirmed that I can translate the GTOP reference sheet to source code comments, and provided the mapping between type 2, 3, 4 and series "Helix", "Thinkpad 10" and "X1 Tablet". > >> 3. Call GTOP with parameter 0x0200, set in_tablet_mode based on bit 0 >> and bit 16. I think we should report SW_TABLET_MODE=1 when the thin-kbd >> is attached, but folded to the back > > Right. > >> 3. Make hotkey_get_tablet_mode support the new TP_HOTKEY_TABLET_USES_GTOP case and >> make it call GTOP with parameter 0x0200 and check bit 0 + bit 16 >> 4. On the events which you identified call tpacpi_input_send_tabletsw() > > Works like a charm. > I realized that the device also emits 0x60c0 (TP_HKEY_EV_TABLET_CHANGED) when the keyboard cover is attached or detached, yet *not* when it's folded. I don't quite get why I nevertheless receive only one notification to userspace according to acpi_listen, despite the fact that the 0x60c0 handler also calls tpacpi_input_send_tabletsw and hotkey_tablet_mode_notify_change. Is there a deduplication behind the scenes? Yes the input subsystem layer will not send events when nothing has changed. > I also realized that intel_vbtn reports the change, too. Would it be in order to modify intel_vbtn in a next step and blacklist this device to avoid duplicates? Hmm, that is a bit of a problem I would prefer to avoid having to deny-list things in intel_vbtn. So do the 2 behave exactly the same? Also wrt when the kbd is folded behind the kbd. IOW are the 2 SW_TABLET_MODE reports fully in sync in all possible states: 1. Just the tablet 2. Tablet + keyboard attached, keyboard in use 3. Tablet + keyboard attached, keyboard folded away behind tablet ? > On the other hand, userspace should expect duplicate messages to some degree and use a hysteresis approach anyway. Every now and then, the contact of the magnetic plug is not established perfectly on the first attempt. So perhaps not really an issue. The only userspace consumer of this which I know is mutter (part of gnome-shell) and it will just take the value from the last event. So if the 2 are always in sync then the event send by the second input-dev will essentially be a no-op since the value will be the same as the other event. We do need to resolve the question of how to handle this before I can merge the patch, atm I think that just having it reported twice is fine (as long as both reports are in sync). Regards, Hans
