Am 15.03.23 um 23:33 schrieb Armin Wolf:
Am 15.03.23 um 04:37 schrieb Andrew Kallmeyer:
On Fri, Mar 10, 2023 at 2:28 AM Armin Wolf <W_Armin@xxxxxx> wrote:
Hi,
according to the embedded MOF data, the method id for retrieving the
mode data
is 0x1, not 0xAB.
Thanks! I can see from your earlier email in the other thread that
this is right. Strangely, when I tested 0xAB had almost exactly the
same behavior as 0x01. I think you're right though, I have updated my
local copy to 0x01.
Hi,
the reason for this is that most hardware vendors will omit any input
checks
if they think they are unnecessary. The WMI interface on my Dell notebook
does the same, sadly.
I have also fixed the missing cleanup calls and
Using wmi_evaluate_method() is deprecated as WMI GUIDs are not
guaranteed to be
unique for a given machine. I think both WMI GUIDs should be handled
by separate
drivers, and the driver for the event GUID uses a notifier call
chain to inform
the driver for the data GUID that the usage mode changed, which then
handles the
input device stuff.
This way the driver does not rely on deprecated WMI APIs, and
everything would
still work should Lenovo decide to duplicate some WMI GUIDs.
After reading many times, I believe I understand this and can figure
out the implementation. How should I attribute the commits? Should
this be a 3rd patch in a v2 patch series with myself as the author? Or
should these drivers be introduced in one commit without the
intermediate single driver that uses the deprecated API?
I thing the new driver(s) should be introduced in one commit. I am not
sure about the
new author of the commit. If the original driver was significantly
altered, then AFAIK
you should be the new author. Still, the original author should at
least be mentioned
with a "Co-developed by" tag. You can then omit your "Co-developed by"
tag.
As a site note, i recommend to use the get_maintainer.pl scripts under
scripts/ to find
any additional maintainers which should be CC-ed. Since your patch
series touches the
ideapad-laptop driver, its maintainer (Ike Panhc
<ike.pan@xxxxxxxxxxxxx>) should also be
notified about this patch series.
I forgot to mention that your patches have to title, please add one for the next revision.
Armin Wolf
Also have I correctly set Gergo as the commit author for this PATCH
2/2 email like Hans said to? I wasn't sure if I had it right and I
could fix it in the v2 series.
You are still the author of the second patch. Also you should not send
a patch series as
a reply to any previous emails.
acpi_dev_put() is missing.
Thanks! Not sure why I thought it was okay to delete lenovo_ymc_remove
but I have added that back in with the input_unregister_device call.
Maybe it would be beneficial to allow userspace to get the current
usage mode without having
to wait for an WMI event. This way, userspace could still react to
situations like the device
already being in tablet mode when this driver is loaded.
I'm not following how to implement this, not familiar enough with WMI.
Could you explain more?
I meant that your driver should, after (!) setting up the input device
and event handling, call
sparse_keymap_report_event() with the code obtained from
wmidev_evaluate_method() so that userspace knows about the initial state
of the input device. You might also CC linux-input@xxxxxxxxxxxxxxx for
the next patch series, so that the input driver maintainers can also
review your patch series.
If the drivers handling the event and data GUIDs are fine with being
instantiated multiple
times, then adding the WMI GUIDs to the allow_duplicates[] list in
drivers/platform/x86/wmi.c
will allow the WMI driver core to handle duplicated event and data
GUIDs.
Is there an easy way to test if it's fine to run multiple copies?
Currently testing by compiling the module with an inlined
ideapad-laptop.h out of the kernel tree and using the insmod command
to load it.
Drivers can be instantiated multiple times, and each time their probe
callback is invoked,
and many older WMI drivers cannot do this, so the allowlist exists.
The section "State Container" in
Documentation/driver-api/driver-model/design-patterns.rst
explains how to write drivers which can be instantiated multiple times.
If your driver is not a singleton, i.e. it can safely be instantiated
multiple times, then
you can add its WMI GUID to the allowlist.
