Hi Andrew,
On 3/8/23 06:14, Andrew Kallmeyer wrote:
> On Mon, Mar 6, 2023 at 12:38 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> Armin and Maximilian, thank you for helping Andrew with this.
>>
>> On 3/6/23 02:41, Armin Wolf wrote:
>>> Am 06.03.23 um 02:26 schrieb Armin Wolf:
>>>
>>>> Am 05.03.23 um 23:59 schrieb Andrew Kallmeyer:
>>>>
>>>>> On Sun, Mar 5, 2023 at 1:40 PM Armin Wolf <W_Armin@xxxxxx> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> could it be that bit 5 is set to disable the touchpad when the
>>>>>> device switches
>>>>>> to tablet mode? I suspect that the query handler does the following:
>>>>>> 1. Notify VPC0 to disable the touchpad.
>>>>>> 2. Notify ACPI WMI, which does submit the necessary scancode for
>>>>>> switching to tablet mode.
>>>>> I think you're right about this notification being for the touchpad,
>>>>> although at least on my machine
>>>>> there is no other touchpad switch. So this is identical for my machine
>>>>> specifically. In this function
>>>>> from the decompiled ACPI dump you can see VCP0 and WM00 notified:
>>>>>
>>>>> Method (_Q44, 0, NotSerialized) // _Qxx: EC Query, xx=0x00-0xFF
>>>>> {
>>>>> P80B = 0x44
>>>>> Notify (VPC0, 0x80) // Status Change
>>>>> WEID = 0xF4
>>>>> Notify (WM00, 0x80) // Status Change
>>>>> }
>>>>>
>>>>> This WM00 device sounds like the WMI you're talking about, however I'm
>>>>> getting those errors
>>>>> about this device not existing in journalctl still. I was asking
>>>>> before about how to create this
>>>>> missing device but it's not clear to me if that is possible.
>>>>>
>>>>> kernel: ACPI BIOS Error (bug): Could not resolve symbol
>>>>> [\_SB.PC00.LPCB.EC0._Q44.WM00], AE_NOT_FOUND
>>>>>
>>>>> I searched in my .dsl files from the acpidump and wasn't able to find
>>>>> any of the 3 ideapad_wmi_ids
>>>>> listed in the driver. Maybe you have an idea of how to interface with
>>>>> this missing WM00 object though.
>>>>
>>>> I was combing through the ACPI DSDT table inside the acpidump you
>>>> provided,
>>>> and i found serveral PNP0C14 devices, which hold WMI methods, events
>>>> and data.
>>>>
>>>> The WMI GUIDs are encoded inside the associated _WDG buffers, you
>>>> should therefore
>>>> only grep for parts of the GUIDs.
>>>> For example: GUID 06129D99-6083-4164-81AD-F092F9D773A6 -> grep "0xF0,
>>>> 0x92"
>>>>
>>>> When feeding the content of the buffers named WQxx to the bmfdec
>>>> utility, i was able
>>>> to extract a description of each WMI object.
>>>>
>>>> On of which (WMIY) is handling the "Lenovo Yoga Mode", which seems to
>>>> handle the tablet
>>>> mode transitions. The MOF data is:
>>>>
>>>> [WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x409"),
>>>> Description("Lenovo Yoga Mode Change Event"),
>>>> guid("{06129D99-6083-4164-81AD-F092F9D773A6}")]
>>>> class LENOVO_GSENSOR_EVENT : WMIEvent {
>>>> [key, read] string InstanceName;
>>>> [read] boolean Active;
>>>> [WmiDataId(1), read, Description("Lenovo Yoga Mode Change Event")]
>>>> uint32 ModeDataVal;
>>>> };
>>>>
>>>> [WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x409"),
>>>> Description("LENOVO_GSENSOR_DATA class"),
>>>> guid("{09B0EE6E-C3FD-4243-8DA1-7911FF80BB8C}")]
>>>> class LENOVO_GSENSOR_DATA {
>>>> [key, read] string InstanceName;
>>>> [read] boolean Active;
>>>>
>>>> [WmiMethodId(1), Implemented, Description("Mode Data")] void
>>>> GetUsageMode([out, Description("Mode Data")] uint32 Data);
>>>> [WmiMethodId(2), Implemented, Description("Get Xaxis Value")] void
>>>> GetXaxisValue([out, Description("Get Xaxis Value")] uint32 Data);
>>>> [WmiMethodId(3), Implemented, Description("Get Yaxis Value")] void
>>>> GetYaxisValue([out, Description("Get Yaxis Value")] uint32 Data);
>>>> [WmiMethodId(4), Implemented, Description("Get Zaxis Value")] void
>>>> GetZaxisValue([out, Description("Get Zaxis Value")] uint32 Data);
>>>> [WmiMethodId(5), Implemented, Description("Base to Ground")] void
>>>> GetAngle4Value([out, Description("Base to Ground")] uint32 Data);
>>>> [WmiMethodId(6), Implemented, Description("Screen to Ground")] void
>>>> GetAngle5Value([out, Description("Screen to Ground")] uint32 Data);
>>>> [WmiMethodId(7), Implemented, Description("Screen to Base")] void
>>>> GetAngle6Value([out, Description("Screen to Base")] uint32 Data);
>>>> };
>>>>
>>>> While looking at _WED (used to get ModeDataVal) and WMAB (handles
>>>> method calls on LENOVO_GSENSOR_DATA),
>>>> i assume that only the first method (GetUsageMode) is implemented,
>>>> and that ModeDataVal is used to tell
>>>> which value of LENOVO_GSENSOR_DATA has changed (hardwired to 1 on your
>>>> device).
>>>>
>>>> Maybe you can write a wmi driver which handles both WMI objects, so
>>>> that you can find out what the values
>>>> returned by GetUsageMode mean. With a bit of luck, you can use this to
>>>> implement tablet mode toggle detection.
>>>>
>>>> BTW, what is the name of your notebook model?
>>>>
>>>> Armin Wolf
>>>>
>>> Well, it turns out i totally forgot that there exists already a patch which adds support for this:
>>> https://patchwork.kernel.org/project/platform-driver-x86/patch/20221004214332.35934-1-soyer@xxxxxx/
>>>
>>> Maybe you can get this patch into shape and submit it again?
>>
>> Yes if you can do that, then that would be great.
>>
>> The end result should really be a patch-series then with the first patch
>> being the prep patch introducing a new ideapad-laptop.h I suggested
>> during my review. This patch can have you as the author + your
>> Signed-off-by.
>>
>> The 2nd patch would then introduce a new version of Gergo's driver using
>> the helper functions introduced in the first patch.
>>
>> For this patch you want to keep Gergo as the author (1), and then add
>> yourself as co-author by using the following tags at the end of
>> the commit message:
>>
>> Co-developed-by: Andrew Kallmeyer <kallmeyeras@xxxxxxxxx>
>> Signed-off-by: Andrew Kallmeyer <kallmeyeras@xxxxxxxxx>
>>
>> Note do NOT add a Signed-off-by for Gergo, he clearly intended to
>> submit his code under the GPL (see the GPL copyright header in
>> the new file) so the Signed-off-by missing is fine. And you must
>> not add Signed-off-by-s for other people. A S-o-b must always
>> be given by the person themselves.
>>
>> Regards,
>>
>> Hans
>>
>> 1) git commit --author="..."
>
> Hi, Hans, thanks for the great advice! I'm excited to see it's already
> so close to being done. I have compiled Gergo's module separately from
> the kernel tree and loaded it on my laptop and it works perfectly.
>
> I will follow the review comments on the patch, getting it integrated
> with the existing module etc. I will also make sure to sign the
> commits as you advise.
Great, thank you for picking op Gergo's work on this!
> It will be good to have it working for
> everyone. This will be my first kernel patch; I have found the docs
> page on submitting a patch but let me know if there's anything I'm
> missing when I do it.
Unfortunately the kernel process is not very newcomer friendly,
so chances are you will make make some mistakes / miss some steps
with your first submission.
Note that that (making some mistakes) is totally fine, the best
advice I can give you is don't be afraid to make mistakes :)
If something needs to be fixed before the patches can be merged
I or another reviewer will gently point that out and you can
prepare a new version addressing any remarks.
The only other advise I have is try to use "git send-email"
to send out the patches, some people copy and paste them
into a regular email client to send them and most email
clients then mangle the patch (change whitespace, wrap
lines, etc.).
Regards,
Hans
