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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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