RE: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Pierre：
> -Original Message-
> From: Pierre-Louis Bossart
> Sent: Monday, March 22, 2021 10:50 PM
> To: Perry Yuan; Yuan, Perry; po...@protonmail.com;
> oder_ch...@realtek.com; pe...@perex.cz; ti...@suse.com;
> hdego...@redhat.com; mgr...@linux.intel.com; Limonciello, Mario
> Cc: linux-kernel@vger.kernel.org; alsa-de...@alsa-project.org;
> broo...@kernel.org; lgirdw...@gmail.com; platform-driver-
> x...@vger.kernel.org
> Subject: Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
>
>
> [EXTERNAL EMAIL]
>
>
> > As you suggested,I should add the alignment change in another patch.
> > But if i keep the old alignment, the code will be very odd.
> > Seems like that I have to change the below code to new alignment in
> > this patch.
> >
> > if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> > dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) { <<--- changed
> > back
> > if (!privacy_valid)
> > has_privacy = true;
> > else
> > has_privacy = false;
> > if (!has_privacy) {
> > micmute_led_cdev.brightness <<--- new alignment
> > ...
> > }
> > ...
> > }
>
> I don't get the point, sorry. The code above doesn't seem properly indented
> or there were spurious tab/spaces conversions?
Could you help to take a look the V5 patch ?
I recovery some part of original code alignment and add my new codes with new
Tabs added
Thank you !
Perry
Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
As you suggested,I should add the alignment change in another patch.
But if i keep the old alignment, the code will be very odd.
Seems like that I have to change the below code to new alignment in this
patch.
if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) { <<--- changed back
if (!privacy_valid)
has_privacy = true;
else
has_privacy = false;
if (!has_privacy) {
micmute_led_cdev.brightness <<--- new alignment
...
}
...
}
I don't get the point, sorry. The code above doesn't seem properly
indented or there were spurious tab/spaces conversions?
Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hi Pierre:
Thanks for your review!
On 2021/3/8 23:59, Pierre-Louis Bossart wrote:
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.
it's fine to improve spaces/alignment, just do it in a separate patch.
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(_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(_device->dev,
_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.
same here, use a different patch.
As you suggested,I should add the alignment change in another patch.
But if i keep the old alignment, the code will be very odd.
Seems like that I have to change the below code to new alignment in this
patch.
if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) { <<--- changed back
if (!privacy_valid)
has_privacy = true;
else
has_privacy = false;
if (!has_privacy) {
micmute_led_cdev.brightness <<--- new alignment
...
}
...
}
Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware 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.
it's fine to improve spaces/alignment, just do it in a separate patch.
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(_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(_device->dev,
_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.
same here, use a different patch.
RE: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
Hello Pierre:
> -Original Message-
> From: Pierre-Louis Bossart
> Sent: Monday, March 1, 2021 10:28 PM
> To: Yuan, Perry; po...@protonmail.com; oder_ch...@realtek.com;
> pe...@perex.cz; ti...@suse.com; hdego...@redhat.com;
> mgr...@linux.intel.com; Limonciello, Mario
> Cc: lgirdw...@gmail.com; broo...@kernel.org; alsa-de...@alsa-project.org;
> linux-kernel@vger.kernel.org; platform-driver-...@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.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
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(_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(_device->dev,
> _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;
> > + else
> > + has_privacy = false;
> > + if (!has_privacy) {
> > + micmute_led_cdev.brightness =
> ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > + ret = led_classdev_register(_device->dev,
> _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(_privacy_platform_drv);
> &g
Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
On 3/1/21 1:37 AM, Perry Yuan wrote: > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 91e6176cdfbd..e20f79389a39 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -491,6 +491,23 @@ config DELL_WMI_LED > This adds support for the Latitude 2100 and similar > notebooks that have an external LED. > > +config DELL_PRIVACY > + tristate "Dell Hardware Privacy Support" > + depends on ACPI > + depends on ACPI_WMI > + depends on INPUT > + depends on DELL_LAPTOP > + depends on LEDS_TRIGGER_AUDIO > + select DELL_WMI > + help The "help" keyword should be indented only with one tab. Drop the 2 additional spaces for it. > + This driver provides support for the "Dell Hardware Privacy" feature > + of Dell laptops. > + Support for a micmute and camera mute privacy will be provided as > + hardware button Ctrl+F4 and Ctrl+F9 hotkey. > + > + To compile this driver as a module, choose M here: the module will > + be called dell_privacy. -- ~Randy
Re: [PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
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(_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(_device->dev,
_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(_device->dev,
_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(_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(_mutex);
+ priv = list_first_entry_or_null(_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(>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
[PATCH v4 1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy
From: Perry Yuan 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 activated, 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 emmits SW_CAMERA_LENS_COVER event while the camera lens shutter will be changed by EC & hw(hadware) control *The flow is like this: 1) User presses key. HW does stuff with this key (timeout timer is started) 2) WMI event is emitted from BIOS to kernel 3) WMI event is received by dell-privacy 4) KEY_MICMUTE emitted from dell-privacy 5) Userland picks up key and modifies kcontrol for SW mute 6) Codec kernel driver catches and calls ledtrig_audio_set, like this: ledtrig_audio_set(LED_AUDIO_MICMUTE, rt715->micmute_led ? LED_ON :LED_OFF); 7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled, HW mic mute activated. Signed-off-by: Perry Yuan --- v3 -> v4: * fix format for Kconfig * add sysfs document * add flow comments to the privacy wmi/acpi driver * addressed feedback from Barnabás Pőcze[Thanks very much] * export privacy_valid to make the global state simpler to query * fix one issue which will block the dell-laptop driver to load when privacy driver invalid * addressed feedback from Pierre-Louis Bossart,remove the EC ID match v2 -> v3: * add sysfs attributes doc v1 -> v2: * query EC handle from EC driver directly. * fix some code style. * add KEY_END to keymap array. * clean platform device when cleanup called * use hexadecimal format for log print in dev_dbg * remove __set_bit for the report keys from probe. * fix keymap leak * add err_free_keymap in dell_privacy_wmi_probe * wmi driver will be unregistered if privacy_acpi_init() fails * add sysfs attribute files for user space query. * add leds micmute driver to privacy acpi * add more design info the commit info --- --- .../testing/sysfs-platform-dell-privacy-wmi | 32 ++ drivers/platform/x86/Kconfig | 17 + drivers/platform/x86/Makefile | 4 +- drivers/platform/x86/dell-laptop.c| 26 +- drivers/platform/x86/dell-privacy-acpi.c | 164 + drivers/platform/x86/dell-privacy-wmi.c | 340 ++ drivers/platform/x86/dell-privacy-wmi.h | 35 ++ drivers/platform/x86/dell-wmi.c | 29 +- 8 files changed, 631 insertions(+), 16 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi create mode 100644 drivers/platform/x86/dell-privacy-acpi.c create mode 100644 drivers/platform/x86/dell-privacy-wmi.c create mode 100644 drivers/platform/x86/dell-privacy-wmi.h diff --git a/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi new file mode 100644 index ..20c15a51ec38 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi @@ -0,0 +1,32 @@ +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported +Date: Feb 2021 +KernelVersion: 5.12 +Contact: "perry.y...@dell.com>" +Description: + Display which dell hardware level privacy devices are supported + “Dell Privacy” is a set of HW, FW, and SW features to enhance + Dell’s commitment to platform privacy for MIC, Camera, and + ePrivacy screens. + The supported hardware privacy devices are: + - 0 = Not Supported + - 1 = Supported + - Bit0 -> Microphone + - Bit1 -> Camera + - Bit2 -> ePrivacy Screen + +What: /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state +Date: Feb 2021 +KernelVersion: 5.12 +Contact: "perry.y...@dell.com>" +Description: + Allow user space to check current dell privacy device state. + Describes the Device State class exposed by BIOS which can be + consumed by various applications interested in knowing the Privacy + feature capabilities + There are three Bits for available states: + - 0 = Enabled + - 1 = Disabled + - Bit0 -> Microphone + - Bit1 -> Camera + - Bit2 -> ePrivacyScreen + diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 91e6176cdfbd..e20f79389a39 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -491,6 +491,23 @@ config DELL_WMI_LED This adds support for the Latitude 2100 and similar notebooks that have an external LED. +config
