Hello Pierre:
-----Original Message----- From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Sent: Monday, March 1, 2021 10:28 PM To: Yuan, Perry; pobrn@protonmail.com; oder_chiou@realtek.com; perex@perex.cz; tiwai@suse.com; hdegoede@redhat.com; mgross@linux.intel.com; Limonciello, Mario Cc: lgirdwood@gmail.com; broonie@kernel.org; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org Subject: Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
[EXTERNAL EMAIL]
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 581475f59819..18c430456de7 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -51,7 +51,9 @@ obj-$(CONFIG_DELL_WMI_DESCRIPTOR) +=
dell-wmi-descriptor.o
obj-$(CONFIG_DELL_WMI_AIO) += dell-wmi-aio.o obj-$(CONFIG_DELL_WMI_LED) += dell-wmi-led.o obj-$(CONFIG_DELL_WMI_SYSMAN) += dell-wmi-sysman/
+obj-$(CONFIG_DELL_PRIVACY) += dell-privacy.o +dell-privacy-objs := dell-privacy-wmi.o \
dell-privacy-acpi.odiff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c index 70edc5bb3a14..ec0dcc7fc17c 100644 --- a/drivers/platform/x86/dell-laptop.c +++ b/drivers/platform/x86/dell-laptop.c @@ -31,6 +31,8 @@ #include "dell-rbtn.h" #include "dell-smbios.h"
+#include "dell-privacy-wmi.h"
- struct quirk_entry { bool touchpad_led; bool kbd_led_not_present;
@@ -90,10 +92,12 @@ static struct rfkill *wifi_rfkill; static struct rfkill *bluetooth_rfkill; static struct rfkill *wwan_rfkill; static bool force_rfkill; +static bool has_privacy;
module_param(force_rfkill, bool, 0444); MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
spurious line change
I just want to make them separate with more space . If it cause concern, I will remote the line in V5.
static const struct dmi_system_id dell_device_table[] __initconst = { { .ident = "Dell laptop", @@ -2205,11 +2209,17 @@ static int __init dell_init(void) dell_laptop_register_notifier(&dell_laptop_notifier);
if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {micmute_led_cdev.brightness =ledtrig_audio_get(LED_AUDIO_MICMUTE);
ret = led_classdev_register(&platform_device->dev,&micmute_led_cdev);
if (ret < 0)goto fail_led;
dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
not sure why you changed the alignment?
The previous alignment is a little not correct. So I adjust it If it cause concern, will restore it to original shape.
if (!privacy_valid)has_privacy = true;elsehas_privacy = false;if (!has_privacy) {micmute_led_cdev.brightness =ledtrig_audio_get(LED_AUDIO_MICMUTE);
ret = led_classdev_register(&platform_device->dev,&micmute_led_cdev);
if (ret < 0)goto fail_led;}}
if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
+static struct platform_driver dell_privacy_platform_drv = {
- .driver = {
.name = PRIVACY_PLATFORM_NAME,- },
- .probe = dell_privacy_acpi_probe,
- .remove = dell_privacy_acpi_remove,
+};
+int __init dell_privacy_acpi_init(void) {
- int err;
- struct platform_device *pdev;
- if (!wmi_has_guid(DELL_PRIVACY_GUID))
return -ENODEV;- privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
- if (!privacy_acpi)
return -ENOMEM;- err = platform_driver_register(&dell_privacy_platform_drv);
- if (err)
goto pdrv_err;- pdev = platform_device_register_simple(
PRIVACY_PLATFORM_NAME,PLATFORM_DEVID_NONE, NULL, 0);
- if (IS_ERR(pdev)) {
err = PTR_ERR(pdev);goto pdev_err;- }
- return 0;
+pdev_err:
- platform_device_unregister(pdev);
+pdrv_err:
- kfree(privacy_acpi);
- return err;
+}
don't you need some sort of device_initcall() to load this module on startup?
The driver dell_privacy_acpi_init will be called from dell-privacy-wmi.c , So this driver file will not start to register by itself . The whole driver init entry is controlled by dell-privacy-wmi.c
+void dell_privacy_process_event(int type, int code, int status) {
- struct privacy_wmi_data *priv;
- const struct key_entry *key;
- mutex_lock(&list_mutex);
- priv = list_first_entry_or_null(&wmi_list,
struct privacy_wmi_data,list);- if (!priv) {
pr_err("dell privacy priv is NULL\n");goto error;- }
- key = sparse_keymap_entry_from_scancode(priv->input_dev, (type
<< 16) | code);
- if (!key) {
dev_dbg(&priv->wdev->dev, "Unknown key with type 0x%04xand code 0x%04x pressed\n",
type, code);goto error;- }
- switch (code) {
- case DELL_PRIVACY_TYPE_AUDIO: /* Mic mute */
priv->last_status = status;sparse_keymap_report_entry(priv->input_dev, key, 1, true);break;- case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
priv->last_status = status;sparse_keymap_report_entry(priv->input_dev, key, 1, true);break;You are doing the same things twice, so group the two cases:
In near future, I am going to add some different event handle codes for the Audio and Camera. Currently the Camera is just reporting the same event to user space like audio dose.
case DELL_PRIVACY_TYPE_AUDIO: /* Mic mute */ case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */ priv->last_status = status; sparse_keymap_report_entry(priv->input_dev, key, 1, true); break;
- default:
dev_dbg(&priv->wdev->dev, "unknown event type0x%04x 0x%04x",
type, code);alignment?
Will fix it in V5
- }
+error:
- mutex_unlock(&list_mutex);
+} +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
+static int dell_privacy_wmi_probe(struct wmi_device *wdev, const void +*context) {
- struct privacy_wmi_data *priv;
- struct key_entry *keymap;
- int ret, i;
- priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(&wdev->dev, priv);
- priv->wdev = wdev;
- /* create evdev passing interface */
- priv->input_dev = devm_input_allocate_device(&wdev->dev);
- if (!priv->input_dev)
return -ENOMEM;- /* remap the wmi keymap event to new keymap */
- keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012),
sizeof(struct key_entry), GFP_KERNEL);- if (!keymap) {
ret = -ENOMEM;goto err_free_dev;- }
- /* remap the keymap code with Dell privacy key type 0x12 as prefix
* KEY_MICMUTE scancode will be reported as 0x120001*/- for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
keymap[i] = dell_wmi_keymap_type_0012[i];keymap[i].code |= (0x0012 << 16);- }
- ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
- if (ret)
return ret;- priv->input_dev->dev.parent = &wdev->dev;
- priv->input_dev->name = "Dell Privacy Driver";
- priv->input_dev->id.bustype = BUS_HOST;
- if (input_register_device(priv->input_dev)) {
pr_debug("input_register_device failed to register!\n");goto err_free_keymap;- }
- mutex_lock(&list_mutex);
- list_add_tail(&priv->list, &wmi_list);
- mutex_unlock(&list_mutex);
- if (get_current_status(priv->wdev))
goto err_free_input;- ret = devm_device_add_group(&wdev->dev,
&privacy_attribute_group);
- if (ret)
goto err_free_input;- kfree(keymap);
- return 0;
+err_free_input:
- input_unregister_device(priv->input_dev);
+err_free_keymap:
- privacy_valid = -ENODEV;
- kfree(keymap);
+err_free_dev:
- input_free_device(priv->input_dev);
priv->input_dev is allocated with devm_, so why do you need to do anything with it? that seems like a miss.
YES, It dose not need to free the device memory, will fix this in V5
- return ret;
+}
MODULE_AUTHOR("Matthew Garrett mjg@redhat.com"); MODULE_AUTHOR("Pali Rohár pali@kernel.org"); @@ -381,6 +383,7 @@ static void dell_wmi_notify(struct wmi_device
*wdev,
u16 *buffer_entry, *buffer_end; acpi_size buffer_size; int len, i;
int err;
if (obj->type != ACPI_TYPE_BUFFER) { pr_warn("bad response type %x\n", obj->type);
@@ -427,10 +430,9 @@ static void dell_wmi_notify(struct wmi_device
*wdev,
switch (buffer_entry[1]) { case 0x0000: /* One key pressed or event occurred */
case 0x0012: /* Event with extended data occurred */ if (len > 2) dell_wmi_process_key(wdev, buffer_entry[1],buffer_entry[2]);
buffer_entry[2]);keep the alignment?
Will check the line and fix it in V5
/* Extended data is currently ignored */ break; case 0x0010: /* Sequence of keys pressed */@@ -439,6 +441,17 @@ static void dell_wmi_notify(struct wmi_device
*wdev,
dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[i]); break;
case 0x0012:err = dell_privacy_state();if (err == 0) {dell_privacy_process_event(buffer_entry[1],buffer_entry[3],buffer_entry[4]);
} else {if (len > 2)dell_wmi_process_key(wdev,buffer_entry[1],
buffer_entry[2]);}break;