Re: [PATCH 2/2] platform/x86: Add driver for Yoga Tablet Mode switch

[Armin Wolf <W_Armin@gmx.de>]

Am 16.03.23 um 10:02 schrieb Hans de Goede:

> Hi
>
> On 3/15/23 23:39, Armin Wolf wrote:
>
> <snip>
>
>>> 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@canonical.com>) 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, I'm not sure what you mean with this ?
>
> For me the patches have a good $subject / first line of the commit message ?
>
> Regards,
>
> Hans
>
>
My fault, i messed up. Sorry for the unnecessary noise.

Armin Wolf

>
>
>> 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@vger.kernel.org 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.
>>>