Platform-driver-x86 archive mirror
 help / color / mirror / Atom feed

From: Armin Wolf <W_Armin@gmx.de>
To: Andy Shevchenko <andriy.shevchenko@intel.com>,
	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
Date: Tue, 26 Mar 2024 21:59:58 +0100	[thread overview]
Message-ID: <31bc6c0a-c0e7-4055-82ec-33da51368e0f@gmx.de> (raw)
In-Reply-To: <ZgGmfV9ciPdtbGO1@smile.fi.intel.com>

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.
>

next prev parent reply	other threads:[~2024-03-26 21:00 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-22  6:47 [PATCH v11] platform/x86: add lenovo wmi camera button driver Ai Chao
2024-03-22 14:34 ` Kuppuswamy Sathyanarayanan
2024-03-22 14:39   ` Ilpo Järvinen
2024-03-22 16:47     ` Kuppuswamy Sathyanarayanan
2024-03-22 18:48       ` Armin Wolf
2024-03-25 15:01 ` Hans de Goede
2024-03-25 16:29 ` Andy Shevchenko
2024-03-26 20:59   ` Armin Wolf [this message]
     [not found] <je8phmmtfz-je9zfg1v9s@nsmail7.0.0--kylin--1>
2024-03-27 10:54 ` Hans de Goede
2024-03-27 13:14   ` Andy Shevchenko
2024-03-27 20:03     ` Hans de Goede
2024-03-28 15:18       ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=31bc6c0a-c0e7-4055-82ec-33da51368e0f@gmx.de \
    --to=w_armin@gmx.de \
    --cc=aichao@kylinos.cn \
    --cc=andriy.shevchenko@intel.com \
    --cc=hdegoede@redhat.com \
    --cc=ilpo.jarvinen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).