Re: [PATCH v8] platform/x86: add lenovo wmi camera button driver

[Armin Wolf <W_Armin@gmx.de>]

Am 15.03.24 um 18:54 schrieb Ilpo Järvinen:

> On Fri, 15 Mar 2024, Armin Wolf wrote:
>> Am 15.03.24 um 12:51 schrieb Ilpo Järvinen:
>>> On Thu, 14 Mar 2024, Armin Wolf wrote:
>>>> Am 14.03.24 um 06:03 schrieb Ai Chao:
>>>>
>>>>> Add lenovo generic wmi driver to support camera button.
>>>>> The Camera button is a GPIO device. This driver receives ACPI notifyi
>>>>> when the camera button is switched on/off. This driver is used in
>>>>> Lenovo A70, it is a Computer integrated machine.
>>>>>
>>>>> Signed-off-by: Ai Chao <aichao@kylinos.cn>
>>>>> ---
>>>>> v8: Dev_deb convert to dev_err.
>>>>> v7: Add dev_dbg and remove unused dev in struct.
>>>>> v6: Modify SW_CAMERA_LENS_COVER to
>>>>> KEY_CAMERA_ACCESS_ENABLE/KEY_CAMERA_ACCESS_DISABLE.
>>>>> v5: Remove camera button groups, modify KEY_CAMERA to
>>>>> SW_CAMERA_LENS_COVER.
>>>>> v4: Remove lenovo_wmi_input_setup, move camera_mode into struct
>>>>> lenovo_wmi_priv.
>>>>> v3: Remove lenovo_wmi_remove function.
>>>>> v2: Adjust GPL v2 to GPL, adjust sprintf to sysfs_emit.
>>>>>
>>>>>     drivers/platform/x86/Kconfig             |  12 +++
>>>>>     drivers/platform/x86/Makefile            |   1 +
>>>>>     drivers/platform/x86/lenovo-wmi-camera.c | 108
>>>>> +++++++++++++++++++++++
>>>>>     3 files changed, 121 insertions(+)
>>>>>     create mode 100644 drivers/platform/x86/lenovo-wmi-camera.c
>>>>>
>>>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>>>> index bdd302274b9a..9506a455b547 100644
>>>>> --- a/drivers/platform/x86/Kconfig
>>>>> +++ b/drivers/platform/x86/Kconfig
>>>>> @@ -1001,6 +1001,18 @@ config INSPUR_PLATFORM_PROFILE
>>>>>     	To compile this driver as a module, choose M here: the module
>>>>>     	will be called inspur-platform-profile.
>>>>>
>>>>> +config LENOVO_WMI_CAMERA
>>>>> +	tristate "Lenovo WMI Camera Button driver"
>>>>> +	depends on ACPI_WMI
>>>>> +	depends on INPUT
>>>>> +	help
>>>>> +	  This driver provides support for Lenovo camera button. The Camera
>>>>> +	  button is a GPIO device. This driver receives ACPI notify when the
>>>>> +	  camera button is switched on/off.
>>>>> +
>>>>> +	  To compile this driver as a module, choose M here: the module
>>>>> +	  will be called lenovo-wmi-camera.
>>>>> +
>>>>>     source "drivers/platform/x86/x86-android-tablets/Kconfig"
>>>>>
>>>>>     config FW_ATTR_CLASS
>>>>> diff --git a/drivers/platform/x86/Makefile
>>>>> b/drivers/platform/x86/Makefile
>>>>> index 1de432e8861e..217e94d7c877 100644
>>>>> --- a/drivers/platform/x86/Makefile
>>>>> +++ b/drivers/platform/x86/Makefile
>>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_SENSORS_HDAPS)	+= hdaps.o
>>>>>     obj-$(CONFIG_THINKPAD_ACPI)	+= thinkpad_acpi.o
>>>>>     obj-$(CONFIG_THINKPAD_LMI)	+= think-lmi.o
>>>>>     obj-$(CONFIG_YOGABOOK)		+= lenovo-yogabook.o
>>>>> +obj-$(CONFIG_LENOVO_WMI_CAMERA)	+= lenovo-wmi-camera.o
>>>>>
>>>>>     # Intel
>>>>>     obj-y				+= intel/
>>>>> diff --git a/drivers/platform/x86/lenovo-wmi-camera.c
>>>>> b/drivers/platform/x86/lenovo-wmi-camera.c
>>>>> new file mode 100644
>>>>> index 000000000000..f83e3ccd9189
>>>>> --- /dev/null
>>>>> +++ b/drivers/platform/x86/lenovo-wmi-camera.c
>>>>> @@ -0,0 +1,108 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Lenovo WMI Camera Button Driver
>>>>> + *
>>>>> + * Author: Ai Chao <aichao@kylinos.cn>
>>>>> + * Copyright (C) 2024 KylinSoft Corporation.
>>>>> + */
>>>>> +
>>>>> +#include <linux/acpi.h>
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/input.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/wmi.h>
>>>>> +
>>>>> +#define WMI_LENOVO_CAMERABUTTON_EVENT_GUID
>>>>> "50C76F1F-D8E4-D895-0A3D-62F4EA400013"
>>>>> +
>>>>> +struct lenovo_wmi_priv {
>>>>> +	struct input_dev *idev;
>>>>> +};
>>>>> +
>>>>> +enum {
>>>>> +	SW_CAMERA_OFF	= 0,
>>>>> +	SW_CAMERA_ON	= 1,
>>>>> +};
>>>>> +
>>>>> +static void lenovo_wmi_notify(struct wmi_device *wdev, union
>>>>> acpi_object
>>>>> *obj)
>>>>> +{
>>>>> +	struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
>>>>> +	u8 camera_mode;
>>>>> +
>>>>> +	if (obj->type != ACPI_TYPE_BUFFER) {
>>>>> +		dev_err(&wdev->dev, "Bad response type %u\n", obj->type);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	if (obj->buffer.length != 1) {
>>>>> +		dev_err(&wdev->dev, "Invalid buffer length %u\n",
>>>>> obj->buffer.length);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	/* obj->buffer.pointer[0] is camera mode:
>>>>> +	 *      0 camera close
>>>>> +	 *      1 camera open
>>>>> +	 */
>>>>> +	camera_mode = obj->buffer.pointer[0];
>>>>> +	if (camera_mode > SW_CAMERA_ON) {
>>>>> +		dev_err(&wdev->dev, "Unknown camera mode %u\n", camera_mode);
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	if (camera_mode == SW_CAMERA_ON) {
>>>>> +		input_report_key(priv->idev, KEY_CAMERA_ACCESS_ENABLE, 1);
>>>>> +		input_sync(priv->idev);
>>>>> +		input_report_key(priv->idev, KEY_CAMERA_ACCESS_ENABLE, 0);
>>>>> +	} else {
>>>>> +		input_report_key(priv->idev, KEY_CAMERA_ACCESS_DISABLE, 1);
>>>>> +		input_sync(priv->idev);
>>>>> +		input_report_key(priv->idev, KEY_CAMERA_ACCESS_DISABLE, 0);
>>>>> +	}
>>> While not exactly wrong the if seems unnecessary, you could do:
>>>
>>> 	unsigned int keycode;
>>>
>>> 	...
>>>
>>> 	keycode = camera_mode == SW_CAMERA_ON ? KEY_CAMERA_ACCESS_ENABLE :
>>> 						KEY_CAMERA_ACCESS_DISABLE;
>>>
>>> 	input_report_key(priv->idev, keycode, 1);
>>> 	input_sync(priv->idev);
>>> 	input_report_key(priv->idev, keycode, 0);
>>>>> +	input_sync(priv->idev);
>>>>> +}
>>> Armin,
>>>
>>> I tried to figure out the concurrency rules for the WMI notify handler but
>>> came up basically nothing. I suppose it boils down on ACPI notify handling
>>> and I couldn't find useful documentation about that either. :-/
>>>
>>> Could you perhaps add this information into WMI documentation?
>> As far as i know, the ACPI notify handlers can be scheduled concurrently on
>> all CPUs,
>> see https://lore.kernel.org/all/7617703.EvYhyI6sBW@kreacher/ for details.
> Hi,
>
> I meant this from the point of view whether the same notify handler can
> only have one instance active at a time because otherwise one would
> need locking to protect e.g. that input injection sequence.

The same WMI notify handler can have multiple instances active at the
same time, so the input sequence indeed needs some locking.

Chao, can you please add a mutex to the struct lenovo_wmi_priv and use this to
protect the input sequence?

Thanks,
Armin Wolf

>
>> I will add a short note about this to the WMI driver guide which i plan to
>> upstream soon (after the EC handler stuff is finished).
> Thanks.
>