Date:   Tue, 12 Jan 2021 13:00:55 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Perry Yuan <Perry.Yuan@...l.com>, oder_chiou@...ltek.com,
        perex@...ex.cz, tiwai@...e.com, hdegoede@...hat.com,
        mgross@...ux.intel.com
Cc:     alsa-devel@...a-project.org, Mario.Limonciello@...l.com,
        linux-kernel@...r.kernel.org, lgirdwood@...il.com,
        platform-driver-x86@...r.kernel.org, broonie@...nel.org
Subject: Re: [PATCH v3 1/3] platform/x86: dell-privacy: Add support for Dell
 hardware privacy



On 1/12/21 11:17 AM, Perry Yuan wrote:
> From: Perry Yuan <perry_yuan@...l.com>
> 
> add support for dell privacy driver for the dell units equipped
> hardware privacy design, which protect users privacy of audio and
> camera from hardware level. once the audio or camera privacy mode
> enabled, any applications will not get any audio or video stream
> when user pressed ctrl+F4 hotkey, audio privacy mode will be
> enabled,Micmute led will be also changed accordingly
> The micmute led is fully controlled by hardware & EC(embedded controller)
> and camera mute hotkey is ctrl+f9. currently design only emmit
> SW_CAMERA_LENS_COVER event while the camera lens shutter will be
> changed by EC & hw(hadware) control

It wouldn't hurt to use capital letters and punctuation, it helps with 
readility..

[...]

> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 70edc5bb3a14..2fea1f34fcf9 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -30,6 +30,7 @@
>   #include <acpi/video.h>
>   #include "dell-rbtn.h"
>   #include "dell-smbios.h"
> +#include "dell-privacy-wmi.h"
>   
>   struct quirk_entry {
>   	bool touchpad_led;
> @@ -90,6 +91,7 @@ static struct rfkill *wifi_rfkill;
>   static struct rfkill *bluetooth_rfkill;
>   static struct rfkill *wwan_rfkill;
>   static bool force_rfkill;
> +static bool privacy_valid;

why is this variable needed? Was the intent to have a kernel parameter here?

>   module_param(force_rfkill, bool, 0444);
>   MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> @@ -2205,11 +2207,18 @@ 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)) {
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> +		ret = dell_privacy_valid();
> +		if (!ret)
> +			privacy_valid = true;
> +#endif
> +		if (!privacy_valid) {

if it was intended to be used as a kernel parameter it's not done the 
right way: the value set by the user would be ignored...

> +			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)
> @@ -2257,7 +2266,8 @@ static int __init dell_init(void)
>   fail_get_brightness:
>   	backlight_device_unregister(dell_backlight_device);
>   fail_backlight:
> -	led_classdev_unregister(&micmute_led_cdev);
> +	if (!privacy_valid)
> +		led_classdev_unregister(&micmute_led_cdev);
>   fail_led:
>   	dell_cleanup_rfkill();
>   fail_rfkill:
> @@ -2278,7 +2288,8 @@ static void __exit dell_exit(void)
>   		touchpad_led_exit();
>   	kbd_led_exit();
>   	backlight_device_unregister(dell_backlight_device);
> -	led_classdev_unregister(&micmute_led_cdev);
> +	if (!privacy_valid)
> +		led_classdev_unregister(&micmute_led_cdev);
>   	dell_cleanup_rfkill();
>   	if (platform_device) {
>   		platform_device_unregister(platform_device);
> diff --git a/drivers/platform/x86/dell-privacy-acpi.c b/drivers/platform/x86/dell-privacy-acpi.c
> new file mode 100644
> index 000000000000..df6a86e1345c
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-acpi.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/wmi.h>
> +
> +#include "dell-privacy-wmi.h"
> +
> +#define PRIVACY_PLATFORM_NAME	"dell-privacy-acpi"
> +#define DELL_PRIVACY_GUID	"6932965F-1671-4CEB-B988-D3AB0A901919"
> +
> +struct privacy_acpi_priv {
> +	struct device *dev;
> +	struct platform_device *platform_device;
> +	struct led_classdev cdev;
> +};
> +static struct privacy_acpi_priv *privacy_acpi;
> +
> +static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
> +		enum led_brightness brightness)
> +{
> +	struct privacy_acpi_priv *priv = privacy_acpi;
> +	acpi_status status;
> +	acpi_handle handle;
> +	char *acpi_method;
> +
> +	handle = ec_get_handle();
> +	if (!handle)
> +		return -EIO;
> +	acpi_method = "ECAK";
> +	status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(priv->dev, "Error setting privacy EC ack value: %s\n",
> +				acpi_format_exception(status));
> +		return -EIO;
> +	}
> +	return 0;
> +}
> +
> +static int dell_privacy_acpi_remove(struct platform_device *pdev)
> +{
> +	struct privacy_acpi_priv *priv = dev_get_drvdata(privacy_acpi->dev);
> +
> +	led_classdev_unregister(&priv->cdev);
> +
> +	return 0;
> +}
> +/*
> + * Pressing the mute key activates a time delayed circuit to physically cut
> + * off the mute. The LED is in the same circuit, so it reflects the true
> + * state of the HW mute.  The reason for the EC "ack" is so that software
> + * can first invoke a SW mute before the HW circuit is cut off.  Without SW
> + * cutting this off first does not affect the time delayed muting or status
> + * of the LED but there is a possibility of a "popping" noise.
> + *
> + * If the EC receives the SW ack, the circuit will be activated before the
> + * delay completed.
> + *
> + * Exposing as an LED device allows the codec drivers notification path to
> + * EC ACK to work
> + */
> +static int dell_privacy_leds_setup(struct device *dev)
> +{
> +	struct privacy_acpi_priv *priv = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	priv->cdev.name = "dell-privacy::micmute";
> +	priv->cdev.max_brightness = 1;
> +	priv->cdev.brightness_set_blocking = dell_privacy_micmute_led_set;
> +	priv->cdev.default_trigger = "audio-micmute";
> +	priv->cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> +	ret = devm_led_classdev_register(dev, &priv->cdev);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}
> +
> +static int dell_privacy_acpi_probe(struct platform_device *pdev)
> +{
> +	platform_set_drvdata(pdev, privacy_acpi);
> +	privacy_acpi->dev = &pdev->dev;
> +	privacy_acpi->platform_device = pdev;
> +	return 0;
> +}
> +
> +static const struct acpi_device_id privacy_acpi_device_ids[] = {
> +	{"PNP0C09", 0},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
> +
> +static struct platform_driver dell_privacy_platform_drv = {
> +	.driver = {
> +		.name = PRIVACY_PLATFORM_NAME,
> +		.acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
> +	},

no .probe?

> +	.remove = dell_privacy_acpi_remove,
> +};
> +
> +int __init dell_privacy_acpi_init(void)
> +{
> +	int err;
> +	struct platform_device *pdev;
> +	int privacy_capable = wmi_has_guid(DELL_PRIVACY_GUID);
> +
> +	if (!wmi_has_guid(DELL_PRIVACY_GUID))
> +		return -ENODEV;
> +
> +	privacy_acpi = kzalloc(sizeof(*privacy_acpi), GFP_KERNEL);
> +	if (!privacy_acpi)
> +		return -ENOMEM;
> +
> +	pdev = platform_device_register_simple(
> +			PRIVACY_PLATFORM_NAME, PLATFORM_DEVID_NONE, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		err = PTR_ERR(pdev);
> +		goto pdev_err;
> +	}
> +	err = platform_driver_probe(&dell_privacy_platform_drv,
> +			dell_privacy_acpi_probe);
> +	if (err)
> +		goto pdrv_err;

why is the probe done here? Put differently, what prevents you from 
using a 'normal' platform driver, and do the leds_setup in the .probe()?

> +
> +	err = dell_privacy_leds_setup(&pdev->dev);
> +	if (err < 0)
> +		goto pdrv_err;
> +
> +	return 0;
> +
> +pdrv_err:
> +	platform_device_unregister(pdev);
> +pdev_err:
> +	kfree(privacy_acpi);
> +	return err;
> +}
> +
> +void __exit dell_privacy_acpi_exit(void)
> +{
> +	struct platform_device *pdev = to_platform_device(privacy_acpi->dev);
> +
> +	platform_device_unregister(pdev);
> +	platform_driver_unregister(&dell_privacy_platform_drv);
> +	kfree(privacy_acpi);
> +}
> +
> +MODULE_AUTHOR("Perry Yuan <perry_yuan@...l.com>");
> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell-privacy-wmi.c b/drivers/platform/x86/dell-privacy-wmi.c
> new file mode 100644
> index 000000000000..432a3f4ed226
> --- /dev/null
> +++ b/drivers/platform/x86/dell-privacy-wmi.c
> @@ -0,0 +1,320 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/input.h>
> +#include <linux/input/sparse-keymap.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/wmi.h>
> +#include "dell-privacy-wmi.h"
> +
> +#define DELL_PRIVACY_GUID "6932965F-1671-4CEB-B988-D3AB0A901919"
> +#define MICROPHONE_STATUS		    BIT(0)
> +#define CAMERA_STATUS		        BIT(1)
> +#define PRIVACY_SCREEN_STATUS		BIT(2)
> +
> +static int privacy_valid = -EPROBE_DEFER;

this is set to -ENODEV on remove. that looks odd to me.

> +static LIST_HEAD(wmi_list);
> +static DEFINE_MUTEX(list_mutex);
> +
> +struct privacy_wmi_data {
> +	struct input_dev *input_dev;
> +	struct wmi_device *wdev;
> +	struct list_head list;
> +	u32 features_present;
> +	u32 last_status;
> +};
> +
> +/*
> + * Keymap for WMI privacy events of type 0x0012
> + */
> +static const struct key_entry dell_wmi_keymap_type_0012[] = {
> +	/* privacy mic mute */
> +	{ KE_KEY, 0x0001, { KEY_MICMUTE } },
> +	/* privacy camera mute */
> +	{ KE_SW,  0x0002, { SW_CAMERA_LENS_COVER } },
> +	{ KE_END, 0},
> +};
> +
> +int dell_privacy_valid(void)
> +{
> +	int ret;
> +
> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);
> +	if (!ret)
> +		return -ENODEV;
> +	ret = privacy_valid;
> +	return ret;

return privacy_valid?

> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_valid);
> +
> +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);

missing spaces, does this even pass with checkpatch.pl?

> +	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 */

Never seen anyone 'mute' their camera? 'switch off' or 'disable'?

> +		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);
> +	}
> +error:
> +	mutex_unlock(&list_mutex);
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);

[...]

> +/*
> + * Describes the Device State class exposed by BIOS which can be consumed by
> + * various applications interested in knowing the Privacy feature capabilities.
> + * class DeviceState
> + * {
> + *  [key, read] string InstanceName;
> + *  [read] boolean ReadOnly;
> + *  [WmiDataId(1), read] uint32 DevicesSupported;
> + *   0 – None, 0x1 – Microphone, 0x2 – Camera, 0x4 -ePrivacy  Screen
> + *  [WmiDataId(2), read] uint32 CurrentState;
> + *   0:Off; 1:On. Bit0 – Microphone, Bit1 – Camera, Bit2 - ePrivacyScreen
> + * };
> + */
> +
> +static int get_current_status(struct wmi_device *wdev)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> +	union acpi_object *obj_present;
> +	u32 *buffer;
> +	int ret = 0;
> +
> +	if (!priv) {
> +		pr_err("dell privacy priv is NULL\n");
> +		return -EINVAL;
> +	}

can this happen? when would this pointer be NULL?

> +	/* check privacy support features and device states */
> +	obj_present = wmidev_block_query(wdev, 0);
> +	if (!obj_present) {
> +		dev_err(&wdev->dev, "failed to read Binary MOF\n");
> +		ret = -EIO;
> +		privacy_valid = ret;
> +		return ret;
> +	}
> +
> +	if (obj_present->type != ACPI_TYPE_BUFFER) {
> +		dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
> +		ret = -EIO;
> +		privacy_valid = ret;
> +		goto obj_free;
> +	}
> +	/*  Although it's not technically a failure, this would lead to
> +	 *  unexpected behavior
> +	 */
> +	if (obj_present->buffer.length != 8) {
> +		dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
> +				obj_present->buffer.length);
> +		ret = -EINVAL;
> +		privacy_valid = ret;
> +		goto obj_free;
> +	}
> +	buffer = (u32 *)obj_present->buffer.pointer;
> +	priv->features_present = buffer[0];
> +	priv->last_status = buffer[1];
> +	privacy_valid = 0;
> +
> +obj_free:
> +	kfree(obj_present);
> +	return ret;
> +}
> +
> +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, pos = 0;
> +
> +	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) +
> +			1,

same line?

> +			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[pos] = dell_wmi_keymap_type_0012[i];
> +		keymap[pos].code |= (0x0012 << 16);
> +		pos++;
> +	}
> +	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;

having a set of newlines wouldn't hurt, thsi is not very easy to read 
and split in logical sections...

> +
> +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);
> +	return ret;
> +}
> +
> +static int dell_privacy_wmi_remove(struct wmi_device *wdev)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
> +
> +	mutex_lock(&list_mutex);
> +	list_del(&priv->list);
> +	mutex_unlock(&list_mutex);
> +	privacy_valid = -ENODEV;
> +	input_unregister_device(priv->input_dev);
> +
> +	return 0;
> +}

[...]

> +		case 0x0012:
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> +			err = dell_privacy_valid();
> +			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]);
> +				/* Extended data is currently ignored */
> +			}
> +#else
> +			if (len > 2)
> +				dell_wmi_process_key(wdev, buffer_entry[1],
> +						buffer_entry[2]);
> +			/* Extended data is currently ignored */
> +#endif

this doesn't look very nice, can we avoid the duplication?

> +			break;
>   		default: /* Unknown event */
>   			pr_info("Unknown WMI event type 0x%x\n",
>   				(int)buffer_entry[1]);
> 

Powered by blists - more mailing lists