* Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
2024-03-22 6:47 Ai Chao
@ 2024-03-22 14:34 ` Kuppuswamy Sathyanarayanan
2024-03-22 14:39 ` Ilpo Järvinen
2024-03-25 15:01 ` Hans de Goede
2024-03-25 16:29 ` Andy Shevchenko
2 siblings, 1 reply; 12+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-22 14:34 UTC (permalink / raw)
To: Ai Chao, hdegoede, ilpo.jarvinen, linux-kernel, platform-driver-x86
On 3/21/24 11:47 PM, 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 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>
> ---
> v11: remove input_unregister_device.
> v10: Add lenovo_wmi_remove and mutex_destroy.
> v9: Add mutex for wmi notify.
> 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 | 126 +++++++++++++++++++++++
> 3 files changed, 139 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..fda936e2f37c
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-wmi-camera.c
> @@ -0,0 +1,126 @@
> +// 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/mutex.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 mutex notify_lock; /* lenovo wmi camera button notify lock */
> +};
> +
> +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);
> + unsigned int keycode;
> + 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;
> + }
> +
> + mutex_lock(&priv->notify_lock);
> +
> + 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);
> +
> + mutex_unlock(&priv->notify_lock);
> +}
> +
> +static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct lenovo_wmi_priv *priv;
> + int ret;
> +
> + 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;
> + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_ENABLE);
> + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_DISABLE);
> +
> + ret = input_register_device(priv->idev);
> + if (ret)
> + return ret;
> +
> + mutex_init(&priv->notify_lock);
> +
> + return 0;
> +}
> +
> +static void lenovo_wmi_remove(struct wmi_device *wdev)
> +{
> + struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> +
> + mutex_destroy(&priv->notify_lock);
Do you really need to call mutex_destroy() during the module unload?
I don't think kernel allocates any memory during mutex_init() that needs
be freed.
> +}
> +
> +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",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = lenovo_wmi_id_table,
> + .no_singleton = true,
> + .probe = lenovo_wmi_probe,
> + .notify = lenovo_wmi_notify,
> + .remove = lenovo_wmi_remove,
> +};
> +
> +module_wmi_driver(lenovo_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
> +MODULE_AUTHOR("Ai Chao <aichao@kylinos.cn>");
> +MODULE_DESCRIPTION("Lenovo WMI Camera Button Driver");
> +MODULE_LICENSE("GPL");
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
2024-03-22 14:34 ` Kuppuswamy Sathyanarayanan
@ 2024-03-22 14:39 ` Ilpo Järvinen
2024-03-22 16:47 ` Kuppuswamy Sathyanarayanan
0 siblings, 1 reply; 12+ messages in thread
From: Ilpo Järvinen @ 2024-03-22 14:39 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Ai Chao, Hans de Goede, LKML, platform-driver-x86
On Fri, 22 Mar 2024, Kuppuswamy Sathyanarayanan wrote:
> On 3/21/24 11:47 PM, 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 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>
> > ---
> > v11: remove input_unregister_device.
> > v10: Add lenovo_wmi_remove and mutex_destroy.
> > v9: Add mutex for wmi notify.
> > 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 | 126 +++++++++++++++++++++++
> > 3 files changed, 139 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..fda936e2f37c
> > --- /dev/null
> > +++ b/drivers/platform/x86/lenovo-wmi-camera.c
> > @@ -0,0 +1,126 @@
> > +// 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/mutex.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 mutex notify_lock; /* lenovo wmi camera button notify lock */
> > +};
> > +
> > +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);
> > + unsigned int keycode;
> > + 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;
> > + }
> > +
> > + mutex_lock(&priv->notify_lock);
> > +
> > + 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);
> > +
> > + mutex_unlock(&priv->notify_lock);
> > +}
> > +
> > +static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
> > +{
> > + struct lenovo_wmi_priv *priv;
> > + int ret;
> > +
> > + 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;
> > + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_ENABLE);
> > + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_DISABLE);
> > +
> > + ret = input_register_device(priv->idev);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_init(&priv->notify_lock);
> > +
> > + return 0;
> > +}
> > +
> > +static void lenovo_wmi_remove(struct wmi_device *wdev)
> > +{
> > + struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> > +
> > + mutex_destroy(&priv->notify_lock);
>
> Do you really need to call mutex_destroy() during the module unload?
>
> I don't think kernel allocates any memory during mutex_init() that needs
> be freed.
Is all debug code going to be happy if it's not called?
--
i.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
2024-03-22 14:39 ` Ilpo Järvinen
@ 2024-03-22 16:47 ` Kuppuswamy Sathyanarayanan
2024-03-22 18:48 ` Armin Wolf
0 siblings, 1 reply; 12+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-22 16:47 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Ai Chao, Hans de Goede, LKML, platform-driver-x86
On 3/22/24 7:39 AM, Ilpo Järvinen wrote:
> On Fri, 22 Mar 2024, Kuppuswamy Sathyanarayanan wrote:
>> On 3/21/24 11:47 PM, 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 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>
>>> ---
>>> v11: remove input_unregister_device.
>>> v10: Add lenovo_wmi_remove and mutex_destroy.
>>> v9: Add mutex for wmi notify.
>>> 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 | 126 +++++++++++++++++++++++
>>> 3 files changed, 139 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..fda936e2f37c
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/lenovo-wmi-camera.c
>>> @@ -0,0 +1,126 @@
>>> +// 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/mutex.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 mutex notify_lock; /* lenovo wmi camera button notify lock */
>>> +};
>>> +
>>> +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);
>>> + unsigned int keycode;
>>> + 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;
>>> + }
>>> +
>>> + mutex_lock(&priv->notify_lock);
>>> +
>>> + 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);
>>> +
>>> + mutex_unlock(&priv->notify_lock);
>>> +}
>>> +
>>> +static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
>>> +{
>>> + struct lenovo_wmi_priv *priv;
>>> + int ret;
>>> +
>>> + 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;
>>> + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_ENABLE);
>>> + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_DISABLE);
>>> +
>>> + ret = input_register_device(priv->idev);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + mutex_init(&priv->notify_lock);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void lenovo_wmi_remove(struct wmi_device *wdev)
>>> +{
>>> + struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
>>> +
>>> + mutex_destroy(&priv->notify_lock);
>> Do you really need to call mutex_destroy() during the module unload?
>>
>> I don't think kernel allocates any memory during mutex_init() that needs
>> be freed.
> Is all debug code going to be happy if it's not called?
>
I am not aware of any issue. Do you have any details about it?
From the comments, it looks like mutex_destroy() is used to mark a
mutex unusable. Not sure why we need to mark a device priv lock
unusable during the unload process.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
2024-03-22 16:47 ` Kuppuswamy Sathyanarayanan
@ 2024-03-22 18:48 ` Armin Wolf
0 siblings, 0 replies; 12+ messages in thread
From: Armin Wolf @ 2024-03-22 18:48 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, Ilpo Järvinen
Cc: Ai Chao, Hans de Goede, LKML, platform-driver-x86
Am 22.03.24 um 17:47 schrieb Kuppuswamy Sathyanarayanan:
> On 3/22/24 7:39 AM, Ilpo Järvinen wrote:
>> On Fri, 22 Mar 2024, Kuppuswamy Sathyanarayanan wrote:
>>> On 3/21/24 11:47 PM, 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 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>
>>>> ---
>>>> v11: remove input_unregister_device.
>>>> v10: Add lenovo_wmi_remove and mutex_destroy.
>>>> v9: Add mutex for wmi notify.
>>>> 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 | 126 +++++++++++++++++++++++
>>>> 3 files changed, 139 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..fda936e2f37c
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/lenovo-wmi-camera.c
>>>> @@ -0,0 +1,126 @@
>>>> +// 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/mutex.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 mutex notify_lock; /* lenovo wmi camera button notify lock */
>>>> +};
>>>> +
>>>> +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);
>>>> + unsigned int keycode;
>>>> + 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;
>>>> + }
>>>> +
>>>> + mutex_lock(&priv->notify_lock);
>>>> +
>>>> + 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);
>>>> +
>>>> + mutex_unlock(&priv->notify_lock);
>>>> +}
>>>> +
>>>> +static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
>>>> +{
>>>> + struct lenovo_wmi_priv *priv;
>>>> + int ret;
>>>> +
>>>> + 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;
>>>> + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_ENABLE);
>>>> + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_DISABLE);
>>>> +
>>>> + ret = input_register_device(priv->idev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + mutex_init(&priv->notify_lock);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void lenovo_wmi_remove(struct wmi_device *wdev)
>>>> +{
>>>> + struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
>>>> +
>>>> + mutex_destroy(&priv->notify_lock);
>>> Do you really need to call mutex_destroy() during the module unload?
>>>
>>> I don't think kernel allocates any memory during mutex_init() that needs
>>> be freed.
>> Is all debug code going to be happy if it's not called?
>>
> I am not aware of any issue. Do you have any details about it?
>
> From the comments, it looks like mutex_destroy() is used to mark a
> mutex unusable. Not sure why we need to mark a device priv lock
> unusable during the unload process.
Hi,
AFAIK calling mutex_destroy() allows the lock debugging code to catch
attempts to access the mutex afterwards, so this would allow us to spot
such issues when enabling lock debugging.
For the whole driver:
Reviewed-by: Armin Wolf <W_Armin@gmx.de>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
2024-03-22 6:47 Ai Chao
2024-03-22 14:34 ` Kuppuswamy Sathyanarayanan
@ 2024-03-25 15:01 ` Hans de Goede
2024-03-25 16:29 ` Andy Shevchenko
2 siblings, 0 replies; 12+ messages in thread
From: Hans de Goede @ 2024-03-25 15:01 UTC (permalink / raw)
To: Ai Chao, ilpo.jarvinen, linux-kernel, platform-driver-x86
Hi,
On 3/22/24 7:47 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
> 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>
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> ---
> v11: remove input_unregister_device.
> v10: Add lenovo_wmi_remove and mutex_destroy.
> v9: Add mutex for wmi notify.
> 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 | 126 +++++++++++++++++++++++
> 3 files changed, 139 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..fda936e2f37c
> --- /dev/null
> +++ b/drivers/platform/x86/lenovo-wmi-camera.c
> @@ -0,0 +1,126 @@
> +// 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/mutex.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 mutex notify_lock; /* lenovo wmi camera button notify lock */
> +};
> +
> +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);
> + unsigned int keycode;
> + 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;
> + }
> +
> + mutex_lock(&priv->notify_lock);
> +
> + 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);
> +
> + mutex_unlock(&priv->notify_lock);
> +}
> +
> +static int lenovo_wmi_probe(struct wmi_device *wdev, const void *context)
> +{
> + struct lenovo_wmi_priv *priv;
> + int ret;
> +
> + 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;
> + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_ENABLE);
> + input_set_capability(priv->idev, EV_KEY, KEY_CAMERA_ACCESS_DISABLE);
> +
> + ret = input_register_device(priv->idev);
> + if (ret)
> + return ret;
> +
> + mutex_init(&priv->notify_lock);
> +
> + return 0;
> +}
> +
> +static void lenovo_wmi_remove(struct wmi_device *wdev)
> +{
> + struct lenovo_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> +
> + mutex_destroy(&priv->notify_lock);
> +}
> +
> +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",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = lenovo_wmi_id_table,
> + .no_singleton = true,
> + .probe = lenovo_wmi_probe,
> + .notify = lenovo_wmi_notify,
> + .remove = lenovo_wmi_remove,
> +};
> +
> +module_wmi_driver(lenovo_wmi_driver);
> +
> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
> +MODULE_AUTHOR("Ai Chao <aichao@kylinos.cn>");
> +MODULE_DESCRIPTION("Lenovo WMI Camera Button Driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
2024-03-22 6:47 Ai Chao
2024-03-22 14:34 ` Kuppuswamy Sathyanarayanan
2024-03-25 15:01 ` Hans de Goede
@ 2024-03-25 16:29 ` Andy Shevchenko
2024-03-26 20:59 ` Armin Wolf
2 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2024-03-25 16:29 UTC (permalink / raw)
To: Ai Chao; +Cc: hdegoede, ilpo.jarvinen, linux-kernel, platform-driver-x86
On Fri, Mar 22, 2024 at 02:47:50PM +0800, Ai Chao wrote:
> Add lenovo generic wmi driver to support camera button.
WMI
> 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.
> +config LENOVO_WMI_CAMERA
> + tristate "Lenovo WMI Camera Button driver"
> + depends on ACPI_WMI
> + depends on INPUT
No COMPILE_TEST?
> + 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.
...
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
+ types.h
> +#include <linux/wmi.h>
...
> +struct lenovo_wmi_priv {
> + struct input_dev *idev;
> + struct mutex notify_lock; /* lenovo wmi camera button notify lock */
WMI
> +};
...
> + /* obj->buffer.pointer[0] is camera mode:
> + * 0 camera close
> + * 1 camera open
> + */
/*
* The correct multi-line comment style
* is depicted here.
*/
...
> + keycode = (camera_mode == SW_CAMERA_ON ?
> + KEY_CAMERA_ACCESS_ENABLE : KEY_CAMERA_ACCESS_DISABLE);
Useless parentheses.
...
> + ret = input_register_device(priv->idev);
> + if (ret)
> + return ret;
> + mutex_init(&priv->notify_lock);
Your mutex should be initialized before use. Have you tested that?
...
> +static struct wmi_driver lenovo_wmi_driver = {
> + .driver = {
> + .name = "lenovo-wmi-camera",
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> + .id_table = lenovo_wmi_id_table,
> + .no_singleton = true,
> + .probe = lenovo_wmi_probe,
> + .notify = lenovo_wmi_notify,
> + .remove = lenovo_wmi_remove,
> +};
> +
Unneeded blank line.
> +module_wmi_driver(lenovo_wmi_driver);
...
> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
Please, move it closer to the respective table.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
2024-03-25 16:29 ` Andy Shevchenko
@ 2024-03-26 20:59 ` Armin Wolf
0 siblings, 0 replies; 12+ messages in thread
From: Armin Wolf @ 2024-03-26 20:59 UTC (permalink / raw)
To: Andy Shevchenko, Ai Chao
Cc: hdegoede, ilpo.jarvinen, linux-kernel, platform-driver-x86
Am 25.03.24 um 17:29 schrieb Andy Shevchenko:
> On Fri, Mar 22, 2024 at 02:47:50PM +0800, Ai Chao wrote:
>> Add lenovo generic wmi driver to support camera button.
> WMI
>
>> 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.
>> +config LENOVO_WMI_CAMERA
>> + tristate "Lenovo WMI Camera Button driver"
>> + depends on ACPI_WMI
>> + depends on INPUT
> No COMPILE_TEST?
>
>> + 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.
> ...
>
>> +#include <linux/acpi.h>
>> +#include <linux/device.h>
>> +#include <linux/input.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
> + types.h
>
>> +#include <linux/wmi.h>
> ...
>
>> +struct lenovo_wmi_priv {
>> + struct input_dev *idev;
>> + struct mutex notify_lock; /* lenovo wmi camera button notify lock */
> WMI
>
>> +};
> ...
>
>> + /* obj->buffer.pointer[0] is camera mode:
>> + * 0 camera close
>> + * 1 camera open
>> + */
> /*
> * The correct multi-line comment style
> * is depicted here.
> */
>
> ...
>
>> + keycode = (camera_mode == SW_CAMERA_ON ?
>> + KEY_CAMERA_ACCESS_ENABLE : KEY_CAMERA_ACCESS_DISABLE);
> Useless parentheses.
>
> ...
>
>> + ret = input_register_device(priv->idev);
>> + if (ret)
>> + return ret;
>> + mutex_init(&priv->notify_lock);
> Your mutex should be initialized before use. Have you tested that?
Hi,
i suggested that the mutex be initialized after calling input_register_device().
The reason for this is that the mutex is only used inside the WMI notify callback,
and the WMI driver core will only call it after probe() has returned.
So imho it should be safe.
Thanks,
Armin Wolf
>
> ...
>
>> +static struct wmi_driver lenovo_wmi_driver = {
>> + .driver = {
>> + .name = "lenovo-wmi-camera",
>> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> + },
>> + .id_table = lenovo_wmi_id_table,
>> + .no_singleton = true,
>> + .probe = lenovo_wmi_probe,
>> + .notify = lenovo_wmi_notify,
>> + .remove = lenovo_wmi_remove,
>> +};
>> +
> Unneeded blank line.
>
>> +module_wmi_driver(lenovo_wmi_driver);
> ...
>
>> +MODULE_DEVICE_TABLE(wmi, lenovo_wmi_id_table);
> Please, move it closer to the respective table.
>
^ permalink raw reply [flat|nested] 12+ messages in thread