Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

From: Pierre-Louis Bossart
Date: Mon Mar 01 2021 - 10:03:28 EST



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.o
# Fujitsu
obj-$(CONFIG_AMILO_RFKILL) += amilo-rfkill.o
obj-$(CONFIG_FUJITSU_LAPTOP) += fujitsu-laptop.o
diff --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

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?

+ if (!privacy_valid)
+ has_privacy = true;
+ else
+ has_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?

+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%04x and 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:

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 type 0x%04x 0x%04x",
+ type, code);

alignment?

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

+ return ret;
+}
+

MODULE_AUTHOR("Matthew Garrett <mjg@xxxxxxxxxx>");
MODULE_AUTHOR("Pali Rohár <pali@xxxxxxxxxx>");
@@ -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?

/* 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;
default: /* Unknown event */
pr_info("Unknown WMI event type 0x%x\n",
(int)buffer_entry[1]);