Received-SPF: pass (google.com: domain of linux-kernel+bounces-117469-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 25 Mar 2024 18:29:49 +0200
From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Ai Chao <aichao@kylinos.cn>
Cc: hdegoede@redhat.com, ilpo.jarvinen@linux.intel.com,
	linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v11] platform/x86: add lenovo wmi camera button driver
Message-ID: <ZgGmfV9ciPdtbGO1@smile.fi.intel.com>
References: <20240322064750.267422-1-aichao@kylinos.cn>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240322064750.267422-1-aichao@kylinos.cn>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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