2024-03-05 12:16:06

by Ai Chao

[permalink] [raw]
Subject: [PATCH v4] platform/x86: add lenovo generic wmi driver

Add lenovo generic wmi driver to support camera button.
The Camera button is a GPIO device. This driver receives ACPI notifyi
when the button pressed.

Signed-off-by: Ai Chao <[email protected]>
---
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 | 118 +++++++++++++++++++++++
3 files changed, 131 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..079f5aa5910c 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
+ button pressed.
+
+ 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..77084266829c
--- /dev/null
+++ b/drivers/platform/x86/lenovo-wmi-camera.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lenovo WMI Camera Button Driver
+ *
+ * Author: Ai Chao <[email protected]>
+ * 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;
+ struct device *dev;
+ u8 camera_mode;
+};
+
+enum {
+ CAMERA_BUTTON_PRESSED = 1,
+};
+
+static ssize_t camerabutton_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct lenovo_wmi_priv *priv = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%u\n", priv->camera_mode);
+}
+DEVICE_ATTR_RO(camerabutton);
+
+static struct attribute *lenovo_wmi_attrs[] = {
+ &dev_attr_camerabutton.attr,
+ NULL,
+};
+
+static const struct attribute_group lenovo_wmi_group = {
+ .attrs = lenovo_wmi_attrs,
+};
+
+const struct attribute_group *lenovo_wmi_groups[] = {
+ &lenovo_wmi_group,
+ NULL,
+};
+
+static void lenovo_wmi_notify(struct wmi_device *wdev, union acpi_object *obj)
+{
+ struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
+
+ if (obj->type == ACPI_TYPE_BUFFER &&
+ obj->buffer.pointer[0] <= CAMERA_BUTTON_PRESSED) {
+ /* Camera mode:
+ * 0 camera close
+ * 1 camera open
+ */
+ priv->camera_mode = obj->buffer.pointer[0];
+
+ input_report_key(priv->idev, KEY_CAMERA, 1);
+ input_sync(priv->idev);
+ input_report_key(priv->idev, KEY_CAMERA, 0);
+ input_sync(priv->idev);
+ } else {
+ dev_dbg(&wdev->dev, "Bad response type %d\n", obj->type);
+ }
+}
+
+static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
+{
+ struct lenovo_wmi_priv *priv;
+
+ priv = devm_kzalloc(&wdev->dev, sizeof(*priv),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ dev_set_drvdata(&wdev->dev, priv);
+
+ priv->idev = devm_input_allocate_device(&wdev->dev);
+ if (!priv->idev)
+ return -ENOMEM;
+
+ priv->idev->name = "Lenovo WMI Camera Button";
+ priv->idev->phys = "wmi/input0";
+ priv->idev->id.bustype = BUS_HOST;
+ priv->idev->dev.parent = &wdev->dev;
+ set_bit(EV_KEY, priv->idev->evbit);
+ set_bit(KEY_CAMERA, priv->idev->keybit);
+
+ return input_register_device(priv->idev);
+}
+
+static const struct wmi_device_id lenovo_wmi_id_table[] = {
+ { .guid_string = WMI_LENOVO_CAMERABUTTON_EVENT_GUID },
+ { }
+};
+
+static struct wmi_driver lenovo_wmi_driver = {
+ .driver = {
+ .name = "lenovo-wmi-camera",
+ .dev_groups = lenovo_wmi_groups,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .id_table = lenovo_wmi_id_table,
+ .no_singleton = false,
+ .probe = lenovo_wmi_probe,
+ .notify = lenovo_wmi_notify,
+};
+
+module_wmi_driver(lenovo_wmi_driver);
+
+MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
+MODULE_AUTHOR("Ai Chao <[email protected]>");
+MODULE_DESCRIPTION("Lenovo Generic WMI Driver");
+MODULE_LICENSE("GPL");
--
2.25.1



Subject: Re: [PATCH v4] platform/x86: add lenovo generic wmi driver


On 3/5/24 4:13 AM, Ai Chao wrote:
> Add lenovo generic wmi driver to support camera button.
> The Camera button is a GPIO device. This driver receives ACPI notifyi

/s/notifyi/notification/

> when the button pressed.
>
> Signed-off-by: Ai Chao <[email protected]>
> ---

Other than this, it looks fine to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
> 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 | 118 +++++++++++++++++++++++
> 3 files changed, 131 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..079f5aa5910c 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
> + button pressed.
> +
> + 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..77084266829c
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-wmi-camera.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Lenovo WMI Camera Button Driver
> + *
> + * Author: Ai Chao <[email protected]>
> + * 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;
> + struct device *dev;
> + u8 camera_mode;
> +};
> +
> +enum {
> + CAMERA_BUTTON_PRESSED = 1,
> +};
> +
> +static ssize_t camerabutton_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct lenovo_wmi_priv *priv = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%u\n", priv->camera_mode);
> +}
> +DEVICE_ATTR_RO(camerabutton);
> +
> +static struct attribute *lenovo_wmi_attrs[] = {
> + &dev_attr_camerabutton.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group lenovo_wmi_group = {
> + .attrs = lenovo_wmi_attrs,
> +};
> +
> +const struct attribute_group *lenovo_wmi_groups[] = {
> + &lenovo_wmi_group,
> + NULL,
> +};
> +
> +static void lenovo_wmi_notify(struct wmi_device *wdev, union acpi_object *obj)
> +{
> + struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> +
> + if (obj->type == ACPI_TYPE_BUFFER &&
> + obj->buffer.pointer[0] <= CAMERA_BUTTON_PRESSED) {
> + /* Camera mode:
> + * 0 camera close
> + * 1 camera open
> + */
> + priv->camera_mode = obj->buffer.pointer[0];
> +
> + input_report_key(priv->idev, KEY_CAMERA, 1);
> + input_sync(priv->idev);
> + input_report_key(priv->idev, KEY_CAMERA, 0);
> + input_sync(priv->idev);
> + } else {
> + dev_dbg(&wdev->dev, "Bad response type %d\n", obj->type);
> + }
> +}
> +
> +static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct lenovo_wmi_priv *priv;
> +
> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv),
> + GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&wdev->dev, priv);
> +
> + priv->idev = devm_input_allocate_device(&wdev->dev);
> + if (!priv->idev)
> + return -ENOMEM;
> +
> + priv->idev->name = "Lenovo WMI Camera Button";
> + priv->idev->phys = "wmi/input0";
> + priv->idev->id.bustype = BUS_HOST;
> + priv->idev->dev.parent = &wdev->dev;
> + set_bit(EV_KEY, priv->idev->evbit);
> + set_bit(KEY_CAMERA, priv->idev->keybit);
> +
> + return input_register_device(priv->idev);
> +}
> +
> +static const struct wmi_device_id lenovo_wmi_id_table[] = {
> + { .guid_string = WMI_LENOVO_CAMERABUTTON_EVENT_GUID },
> + { }
> +};
> +
> +static struct wmi_driver lenovo_wmi_driver = {
> + .driver = {
> + .name = "lenovo-wmi-camera",
> + .dev_groups = lenovo_wmi_groups,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = lenovo_wmi_id_table,
> + .no_singleton = false,
> + .probe = lenovo_wmi_probe,
> + .notify = lenovo_wmi_notify,
> +};
> +
> +module_wmi_driver(lenovo_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
> +MODULE_AUTHOR("Ai Chao <[email protected]>");
> +MODULE_DESCRIPTION("Lenovo Generic WMI Driver");
> +MODULE_LICENSE("GPL");

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer


2024-03-05 14:35:22

by Gergo Koteles

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86: add lenovo generic wmi driver

Hi Ai,

On Tue, 2024-03-05 at 20:13 +0800, Ai Chao wrote:
>
> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
> +MODULE_AUTHOR("Ai Chao <[email protected]>");
> +MODULE_DESCRIPTION("Lenovo Generic WMI Driver");

This is forgotten.
It should be "Lenovo WMI Camera Button driver"

> +MODULE_LICENSE("GPL");

Thanks,
Gergo


2024-03-05 14:59:05

by Thomas Weißschuh

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86: add lenovo generic wmi driver

On Tue, Mar 05, 2024 at 08:13:15PM +0800, Ai Chao wrote:
> Add lenovo generic wmi driver to support camera button.
> The Camera button is a GPIO device. This driver receives ACPI notifyi
> when the button pressed.

the button *is* pressed.


The subject does not mention that it is only about te camera button.

I would be interested on which devices this driver was tested and is
expected to work with.

>
> Signed-off-by: Ai Chao <[email protected]>
> ---
> 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 | 118 +++++++++++++++++++++++
> 3 files changed, 131 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..079f5aa5910c 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
> + button pressed.
> +
> + 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..77084266829c
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-wmi-camera.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Lenovo WMI Camera Button Driver
> + *
> + * Author: Ai Chao <[email protected]>
> + * 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;
> + struct device *dev;
> + u8 camera_mode;
> +};
> +
> +enum {
> + CAMERA_BUTTON_PRESSED = 1,
> +};
> +
> +static ssize_t camerabutton_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct lenovo_wmi_priv *priv = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%u\n", priv->camera_mode);
> +}
> +DEVICE_ATTR_RO(camerabutton);
> +
> +static struct attribute *lenovo_wmi_attrs[] = {
> + &dev_attr_camerabutton.attr,
> + NULL,

No trailing comma after sentinel elements.

> +};
> +
> +static const struct attribute_group lenovo_wmi_group = {
> + .attrs = lenovo_wmi_attrs,
> +};
> +
> +const struct attribute_group *lenovo_wmi_groups[] = {
> + &lenovo_wmi_group,
> + NULL,

Also no trailing comma.

> +};
> +
> +static void lenovo_wmi_notify(struct wmi_device *wdev, union acpi_object *obj)
> +{
> + struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> +
> + if (obj->type == ACPI_TYPE_BUFFER &&
> + obj->buffer.pointer[0] <= CAMERA_BUTTON_PRESSED) {
> + /* Camera mode:
> + * 0 camera close
> + * 1 camera open
> + */
> + priv->camera_mode = obj->buffer.pointer[0];

This looks similar to a switch.
Would it be more useful for the user to report a standard switch instead
of a key event which needs to be correlated with the sysfs file?

> +
> + input_report_key(priv->idev, KEY_CAMERA, 1);
> + input_sync(priv->idev);
> + input_report_key(priv->idev, KEY_CAMERA, 0);
> + input_sync(priv->idev);
> + } else {
> + dev_dbg(&wdev->dev, "Bad response type %d\n", obj->type);
> + }
> +}
> +
> +static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct lenovo_wmi_priv *priv;
> +
> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv),
> + GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&wdev->dev, priv);
> +
> + priv->idev = devm_input_allocate_device(&wdev->dev);
> + if (!priv->idev)
> + return -ENOMEM;
> +
> + priv->idev->name = "Lenovo WMI Camera Button";
> + priv->idev->phys = "wmi/input0";
> + priv->idev->id.bustype = BUS_HOST;
> + priv->idev->dev.parent = &wdev->dev;
> + set_bit(EV_KEY, priv->idev->evbit);
> + set_bit(KEY_CAMERA, priv->idev->keybit);

input_set_capability()?

> +
> + return input_register_device(priv->idev);
> +}
> +
> +static const struct wmi_device_id lenovo_wmi_id_table[] = {
> + { .guid_string = WMI_LENOVO_CAMERABUTTON_EVENT_GUID },
> + { }
> +};
> +
> +static struct wmi_driver lenovo_wmi_driver = {
> + .driver = {
> + .name = "lenovo-wmi-camera",
> + .dev_groups = lenovo_wmi_groups,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = lenovo_wmi_id_table,
> + .no_singleton = false,
> + .probe = lenovo_wmi_probe,
> + .notify = lenovo_wmi_notify,
> +};
> +
> +module_wmi_driver(lenovo_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
> +MODULE_AUTHOR("Ai Chao <[email protected]>");
> +MODULE_DESCRIPTION("Lenovo Generic WMI Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>

2024-03-05 15:35:59

by Armin Wolf

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86: add lenovo generic wmi driver

Am 05.03.24 um 15:48 schrieb Thomas Weißschuh:

> On Tue, Mar 05, 2024 at 08:13:15PM +0800, Ai Chao wrote:
>> Add lenovo generic wmi driver to support camera button.
>> The Camera button is a GPIO device. This driver receives ACPI notifyi
>> when the button pressed.
> the button *is* pressed.
>
>
> The subject does not mention that it is only about te camera button.
>
> I would be interested on which devices this driver was tested and is
> expected to work with.
>
>> Signed-off-by: Ai Chao <[email protected]>
>> ---
>> 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 | 118 +++++++++++++++++++++++
>> 3 files changed, 131 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..079f5aa5910c 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
>> + button pressed.
>> +
>> + 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..77084266829c
>> --- /dev/null
>> +++ b/drivers/platform/x86/lenovo-wmi-camera.c
>> @@ -0,0 +1,118 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Lenovo WMI Camera Button Driver
>> + *
>> + * Author: Ai Chao <[email protected]>
>> + * 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;
>> + struct device *dev;
>> + u8 camera_mode;
>> +};
>> +
>> +enum {
>> + CAMERA_BUTTON_PRESSED = 1,
>> +};
>> +
>> +static ssize_t camerabutton_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct lenovo_wmi_priv *priv = dev_get_drvdata(dev);
>> +
>> + return sysfs_emit(buf, "%u\n", priv->camera_mode);
>> +}
>> +DEVICE_ATTR_RO(camerabutton);
>> +
>> +static struct attribute *lenovo_wmi_attrs[] = {
>> + &dev_attr_camerabutton.attr,
>> + NULL,
> No trailing comma after sentinel elements.
>
>> +};
>> +
>> +static const struct attribute_group lenovo_wmi_group = {
>> + .attrs = lenovo_wmi_attrs,
>> +};
>> +
>> +const struct attribute_group *lenovo_wmi_groups[] = {
>> + &lenovo_wmi_group,
>> + NULL,
> Also no trailing comma.
>
>> +};
>> +
>> +static void lenovo_wmi_notify(struct wmi_device *wdev, union acpi_object *obj)
>> +{
>> + struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
>> +
>> + if (obj->type == ACPI_TYPE_BUFFER &&
>> + obj->buffer.pointer[0] <= CAMERA_BUTTON_PRESSED) {
>> + /* Camera mode:
>> + * 0 camera close
>> + * 1 camera open
>> + */
>> + priv->camera_mode = obj->buffer.pointer[0];
> This looks similar to a switch.
> Would it be more useful for the user to report a standard switch instead
> of a key event which needs to be correlated with the sysfs file?

I agree, maybe SW_CAMERA_LENS_COVER might be the right thing to use here,
if those camera states (open/closed) are meant to symbolize camera shutter states.

In such a case the initial switch state has to be retrieved, or else the input device
cannot be registered until the first event is received (similar how the hp-wmi driver
handles SW_CAMERA_LENS_COVER events).

Ai Chao, can you tell us if those two camera states are meant to act like a switch (camera switched off,
camera switched on) or meant to act like a key (camera button pressed, camera button released)?

>> +
>> + input_report_key(priv->idev, KEY_CAMERA, 1);
>> + input_sync(priv->idev);
>> + input_report_key(priv->idev, KEY_CAMERA, 0);
>> + input_sync(priv->idev);
>> + } else {
>> + dev_dbg(&wdev->dev, "Bad response type %d\n", obj->type);
>> + }
>> +}
>> +
>> +static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
>> +{
>> + struct lenovo_wmi_priv *priv;
>> +
>> + priv = devm_kzalloc(&wdev->dev, sizeof(*priv),
>> + GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + dev_set_drvdata(&wdev->dev, priv);
>> +
>> + priv->idev = devm_input_allocate_device(&wdev->dev);
>> + if (!priv->idev)
>> + return -ENOMEM;
>> +
>> + priv->idev->name = "Lenovo WMI Camera Button";
>> + priv->idev->phys = "wmi/input0";
>> + priv->idev->id.bustype = BUS_HOST;
>> + priv->idev->dev.parent = &wdev->dev;
>> + set_bit(EV_KEY, priv->idev->evbit);
>> + set_bit(KEY_CAMERA, priv->idev->keybit);
> input_set_capability()?
>
>> +
>> + return input_register_device(priv->idev);
>> +}
>> +
>> +static const struct wmi_device_id lenovo_wmi_id_table[] = {
>> + { .guid_string = WMI_LENOVO_CAMERABUTTON_EVENT_GUID },
>> + { }
>> +};
>> +
>> +static struct wmi_driver lenovo_wmi_driver = {
>> + .driver = {
>> + .name = "lenovo-wmi-camera",
>> + .dev_groups = lenovo_wmi_groups,
>> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> + },
>> + .id_table = lenovo_wmi_id_table,
>> + .no_singleton = false,

The correct setting in this case would be ".no_singleton = true",
since camera_mode now lives inside struct lenovo_wmi_priv.

Thanks,
Armin Wolf

>> + .probe = lenovo_wmi_probe,
>> + .notify = lenovo_wmi_notify,
>> +};
>> +
>> +module_wmi_driver(lenovo_wmi_driver);
>> +
>> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
>> +MODULE_AUTHOR("Ai Chao <[email protected]>");
>> +MODULE_DESCRIPTION("Lenovo Generic WMI Driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.25.1
>>

2024-03-06 08:37:05

by Thomas Weißschuh

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86: add lenovo generic wmi driver

On Wed, Mar 06, 2024 at 04:17:43PM +0800, 艾超 wrote:
> > I would be interested on which devices this driver was tested and is
> > expected to work with.
>
> Lenovo A70,it is a Computer integrated machine.

Could you mention this in your next commit message?

Do you have any idea on which other devices this should work?

> >> This looks similar to a switch.
> >> Would it be more useful for the user to report a standard switch
> instead
> >> of a key event which needs to be correlated with the sysfs file?
> > I agree, maybe SW_CAMERA_LENS_COVER might be the right thing to use
> here,
> > if those camera states (open/closed) are meant to symbolize camera
> shutter states.
> > In such a case the initial switch state has to be retrieved, or else
> the input device
> > cannot be registered until the first event is received (similar how
> the hp-wmi driver
> > handles SW_CAMERA_LENS_COVER events).
> > Ai Chao, can you tell us if those two camera states are meant to act
> like a switch (camera switched off,
> > camera switched on) or meant to act like a key (camera button pressed,
> camera button released)?
>
> The camera button is like a switch.  I can used SW_CAMERA_LENS_COVER to
> report input event , but the OS
>
>  can't to show camera OSD. OS need a key value  map to the camera OSD.

Why can't the OS do this?

The switch should report its value.

2024-03-06 11:26:59

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86: add lenovo generic wmi driver

Hi,

On 3/6/24 09:17, 艾超 wrote:
> Hi
>
>  
>
>> I would be interested on which devices this driver was tested and is
>> expected to work with.
>
>  
>
> Lenovo A70,it is a Computer integrated machine.
>
>
>>> This looks similar to a switch.
>>> Would it be more useful for the user to report a standard switch instead
>>> of a key event which needs to be correlated with the sysfs file?
>
>> I agree, maybe SW_CAMERA_LENS_COVER might be the right thing to use here,
>> if those camera states (open/closed) are meant to symbolize camera shutter states.
>
>> In such a case the initial switch state has to be retrieved, or else the input device
>> cannot be registered until the first event is received (similar how the hp-wmi driver
>> handles SW_CAMERA_LENS_COVER events).
>
>> Ai Chao, can you tell us if those two camera states are meant to act like a switch (camera switched off,
>> camera switched on) or meant to act like a key (camera button pressed, camera button released)?
>
>  
>
> The camera button is like a switch.  I can used SW_CAMERA_LENS_COVER to report input event , but the OS
>
>  can't to show camera OSD. OS need a key value  map to the camera OSD.

Please use SW_CAMERA_LENS_COVER in the next version, that really is the correct
thing to do here, so that you not only report the event of the camera state
changing, but also the actual camera state (on/off) to userspace.

This will also allow you to remove the custom sysfs atribute since you
are now correctly reporting the value to userspace through the evdev event.

As for userspace not responding to SW_CAMERA_LENS_COVER, this is because
SW_CAMERA_LENS_COVER is relatively new and we need to add support to
userspace for this. SW_CAMERA_LENS_COVER is already used on whole a bunch of
Dell laptops for this, so we need to do this anyways.

Please file an issue against GNOME, lets say against gnome-settings-daemon
for this.

Regards,

Hans