[v5,1/2] platform/x86: dell-privacy: Add support for Dell hardware privacy

Commit Message

Yuan, Perry March 22, 2021, 9:38 a.m. UTC

From: Perry Yuan <[e-mail]>

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 <[e-mail]>
---
v4 -> v5:
* addressed feedback from Randy Dunlap
* addressed feedback from Pierre-Louis Bossart
* rebase to latest 5.12 rc4 upstream kernel
* fix some space alignment problem
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/dell/Kconfig             |  16 +
 drivers/platform/x86/dell/Makefile            |   3 +
 drivers/platform/x86/dell/dell-laptop.c       |  23 +-
 drivers/platform/x86/dell/dell-privacy-acpi.c | 164 +++++++++
 drivers/platform/x86/dell/dell-privacy-wmi.c  | 340 ++++++++++++++++++
 drivers/platform/x86/dell/dell-privacy-wmi.h  |  35 ++
 drivers/platform/x86/dell/dell-wmi.c          |  27 +-
 8 files changed, 627 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
 create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c
 create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
 create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.h

Comments

Randy Dunlap March 22, 2021, 5:15 p.m. UTC | #1

On 3/22/21 2:38 AM, Perry Yuan wrote:
> From: Perry Yuan <[e-mail]>
> 

> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index e0a55337f51a..0e0f1eb35bd6 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -204,4 +204,20 @@ config DELL_WMI_SYSMAN
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called dell-wmi-sysman.
>  
> +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
> +	  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

better:
	                                                are provided as

> +	  hardware button Ctrl+F4 and Ctrl+F9 hotkey.

Does that say that Ctrl+F4 is a hardware button
and that Ctrl+F9 is a hotkey?
If so, what's the difference? and why?  Are they different
hardware implementations?  Does the user care?

> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called dell_privacy.
>  endif # X86_PLATFORM_DRIVERS_DELL
thanks.

Yuan, Perry March 23, 2021, 6:19 a.m. UTC | #2

Hi Randy.

> -----Original Message-----
> From: Randy Dunlap <[e-mail]>
> Sent: Tuesday, March 23, 2021 1:15 AM
> To: Yuan, Perry; [e-mail]; pierre-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Limonciello, Mario
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> On 3/22/21 2:38 AM, Perry Yuan wrote:
> > From: Perry Yuan <[e-mail]>
> >
> 
> > diff --git a/drivers/platform/x86/dell/Kconfig
> > b/drivers/platform/x86/dell/Kconfig
> > index e0a55337f51a..0e0f1eb35bd6 100644
> > --- a/drivers/platform/x86/dell/Kconfig
> > +++ b/drivers/platform/x86/dell/Kconfig
> > @@ -204,4 +204,20 @@ config DELL_WMI_SYSMAN
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called dell-wmi-sysman.
> >
> > +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
> > +	  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
> 
> better:
> 	                                                are provided as
> 
> > +	  hardware button Ctrl+F4 and Ctrl+F9 hotkey.
> 
> Does that say that Ctrl+F4 is a hardware button and that Ctrl+F9 is a hotkey?
> If so, what's the difference? and why?  Are they different hardware
> implementations?  Does the user care?

They all use the Ctrl button,
Ctrl +F4 requires that  the Ctrl button and F4 button are pressed at the same time. F9+ctrl also have the same meaning.
They are belonging to same hardware privacy solution, just use different keys combination to activate different privacy function .

> 
> > +
> > +	  To compile this driver as a module, choose M here: the module will
> > +	  be called dell_privacy.
> >  endif # X86_PLATFORM_DRIVERS_DELL
> thanks.
> --
> ~Randy

Pierre-Louis Bossart March 23, 2021, 6:57 p.m. UTC | #3

Minor comments below.

On 3/22/21 4:38 AM, Perry Yuan wrote:
> From: Perry Yuan <[e-mail]>
> 
> 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

typo: emits

> SW_CAMERA_LENS_COVER event while the camera lens shutter will be
> changed by EC & hw(hadware) control

typo: hardware
> 
> *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.

what happens if there is timeout? You have an explicit description of 
the timer handling in the success case, but not what happens on a timeout...

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

5.13 now?

> +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;
> +	static char *acpi_method = (char *)"ECAK";
> +
> +	handle = ec_get_handle();
> +	if (!handle)
> +		return -EIO;
> +	if (!acpi_has_method(handle, acpi_method))
> +		return -EIO;
> +	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;
> +	}
> +	pr_debug("set dell privacy micmute ec ack event done\n");

can we use dev_dbg() here? Same for all occurrences of pr_debug and 
pr_err below, it's cleaner and easier to filter.

> +	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;

useless init

> +
> +	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)
> +		return ret;
> +	return 0;
> +}
> +
> +static int dell_privacy_acpi_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	platform_set_drvdata(pdev, privacy_acpi);
> +	privacy_acpi->dev = &pdev->dev;
> +	privacy_acpi->platform_device = pdev;
> +
> +	ret = dell_privacy_leds_setup(&pdev->dev);
> +	if (ret)
> +		return -EIO;

any reason to hard-code -EIO, woud 'return ret' be enough?


> +
> +	return 0;
> +}
> +
> +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)

is the __init necessary? You call this routine from another which 
already has this qualifier.

> +{
> +	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;
> +}
> +
> +void __exit dell_privacy_acpi_exit(void)

is the __exit required here?

> +{
> +	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 <[e-mail]>");
> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> +MODULE_LICENSE("GPL");

Hans de Goede March 23, 2021, 7:40 p.m. UTC | #4

Hi,

On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
> Minor comments below.

<snip<

>> +int __init dell_privacy_acpi_init(void)
> 
> is the __init necessary? You call this routine from another which already has this qualifier.

Yes this is necessary, all functions which are only used during module_load / from the
module's init function must be marked as __init so that the kernel can unload them
after module loading is done.

I do wonder if this one should not be static though (I've not looked at this patch in
detail yet).

> 
>> +{
>> +    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;
>> +}
>> +
>> +void __exit dell_privacy_acpi_exit(void)
> 
> is the __exit required here?

Idem. Also static ?

Regards,

Hans



> 
>> +{
>> +    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 <[e-mail]>");
>> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
>> +MODULE_LICENSE("GPL");
> 
>

Hans de Goede March 24, 2021, 11:19 a.m. UTC | #5

Hi,

On 3/22/21 10:38 AM, Perry Yuan wrote:
> From: Perry Yuan <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> v4 -> v5:
> * addressed feedback from Randy Dunlap
> * addressed feedback from Pierre-Louis Bossart
> * rebase to latest 5.12 rc4 upstream kernel
> * fix some space alignment problem
> 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/dell/Kconfig             |  16 +
>  drivers/platform/x86/dell/Makefile            |   3 +
>  drivers/platform/x86/dell/dell-laptop.c       |  23 +-
>  drivers/platform/x86/dell/dell-privacy-acpi.c | 164 +++++++++
>  drivers/platform/x86/dell/dell-privacy-wmi.c  | 340 ++++++++++++++++++
>  drivers/platform/x86/dell/dell-privacy-wmi.h  |  35 ++
>  drivers/platform/x86/dell/dell-wmi.c          |  27 +-
>  8 files changed, 627 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
>  create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c
>  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
>  create mode 100644 drivers/platform/x86/dell/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 000000000000..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:	"[e-mail]>"
> +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:	"[e-mail]>"
> +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

What is the definition of enabled/disabled ? Does enabled mean that the device
is enabled, or that the device is in privacy mode, so in case of the microphone
it is actually disabled (muted) ?

> +		- Bit0 -> Microphone
> +		- Bit1 -> Camera
> +		- Bit2 -> ePrivacyScreen
> +

Normally we would use strings for something like this and not direclty export a bitfield,
this would also allow avoiding the enabled / disabled ambiguity

E.g. you could have the following:

# supported devices
$ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported
Microphone Mute
Camera Shutter
ePrivacyScreen

# All devices in privacy mode:
$ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state
Microphone muted
Camera Shutter closed
ePrivacyScreen privacy-filter enabled

# All devices in non-privacy mode:

$ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state
Microphone unmuted
Camera Shutter open
ePrivacyScreen privacy-filter disabled

You can easily do the bitfield -> string mapping in the kernel by having 3
string arrays (support, privacy_on, privacy_off) indexed by the bitfield indices.


> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index e0a55337f51a..0e0f1eb35bd6 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -204,4 +204,20 @@ config DELL_WMI_SYSMAN
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called dell-wmi-sysman.
>  
> +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
> +	  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.

This suggests that that the driver implements the buttons, which is not the
case. How about:

	  This driver provides integration with the "Dell Hardware Privacy" feature
	  of Dell laptops.


	  

> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called dell_privacy.
>  endif # X86_PLATFORM_DRIVERS_DELL
> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
> index d720a3e42ae3..8b0883ee9412 100644
> --- a/drivers/platform/x86/dell/Makefile
> +++ b/drivers/platform/x86/dell/Makefile
> @@ -19,3 +19,6 @@ obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
>  obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.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
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 70edc5bb3a14..4c840ab6cb59 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/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,6 +92,7 @@ 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");
> @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
>  
>  	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;
> +		if (!privacy_valid)

This privacy_valid variable is coming from another module, so you're adding
a "privacy_valid" symbol to the global kernel symbol namespace, even though this is
dell specific. Please use the dell_privacy_present() function (see below) instead, so you get:

		if (dell_privacy_present())
			has_privacy = true;
		else
			has_privacy = false;

Which makes much more sense then "if (!privacy_valid) has_privacy = true;" which
feels as if it is inverted to what it should be.

> +			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)
> @@ -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 (!has_privacy)
> +		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 (!has_privacy)
> +		led_classdev_unregister(&micmute_led_cdev);
>  	dell_cleanup_rfkill();
>  	if (platform_device) {
>  		platform_device_unregister(platform_device);
> diff --git a/drivers/platform/x86/dell/dell-privacy-acpi.c b/drivers/platform/x86/dell/dell-privacy-acpi.c
> new file mode 100644
> index 000000000000..4c785b62c7c4
> --- /dev/null
> +++ b/drivers/platform/x86/dell/dell-privacy-acpi.c
> @@ -0,0 +1,164 @@
> +// 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;

Please use container_of on the passed in led_cdev here instead
of relying on a global variable.

> +	acpi_status status;
> +	acpi_handle handle;
> +	static char *acpi_method = (char *)"ECAK";
> +
> +	handle = ec_get_handle();
> +	if (!handle)
> +		return -EIO;
> +	if (!acpi_has_method(handle, acpi_method))
> +		return -EIO;
> +	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;
> +	}
> +	pr_debug("set dell privacy micmute ec ack event done\n");
> +	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)
> +		return ret;
> +	return 0;
> +}
> +
> +static int dell_privacy_acpi_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	platform_set_drvdata(pdev, privacy_acpi);
> +	privacy_acpi->dev = &pdev->dev;
> +	privacy_acpi->platform_device = pdev;
> +
> +	ret = dell_privacy_leds_setup(&pdev->dev);
> +	if (ret)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +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;
> +	}

So you're creating a platform_device here, from code called from the
init_dell_privacy() function in dell-privacy-wmi.c, just so that
the probe() function for that platform-device can register the
led_classdev for the (alternative) mute LED.

Please just put the cdev struct inside struct privacy_wmi_data
and call dell_privacy_leds_setup() with the wmi-dev as parameter
from dell_privacy_wmi_probe and have the wmi-dev as the parent
of the led_classdev.

This allows removing both the platform-driver and the instantiation
of the platform-device.

After removing the unnecessary platfrom-dev/driver there will be
so little left of dell-privacy-acpi.c, that what remains can and
should just be folded into dell-privacy-wmi.c. At which point
you're down to one file for the module which also simplifies the
Makefile.

> +
> +	return 0;
> +
> +pdev_err:
> +	platform_device_unregister(pdev);
> +pdrv_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 <[e-mail]>");
> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
> new file mode 100644
> index 000000000000..8bde97613477
> --- /dev/null
> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
> @@ -0,0 +1,340 @@
> +// 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)
> +
> +/*
> + * The wmi_list is used to store the privacy_priv struct with mutex protecting
> + */
> +static LIST_HEAD(wmi_list);
> +static DEFINE_MUTEX(list_mutex);
> +/*
> + * global privacy state for other modules to query if the privacy driver loaded successfully
> + * or no privacy WMI device was detected at initial probe phase
> + */
> +int privacy_valid = -EPROBE_DEFER;
> +EXPORT_SYMBOL_GPL(privacy_valid);

None of the callers are interpreting the error-codes stored here.
So this is really treated as a bool, and the info stored in the bool
is already available inside the wmi_list-head. So just drop this.

> +
> +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_state(void)

Please rename this to dell_privacy_present(), "state" is very generic / not descriptive
and use list_first_entry_or_null(&wmi_list) as done in dell_privacy_process_event(), e.g.:

bool dell_privacy_present(void)
{
	struct privacy_wmi_data *priv;

	mutex_lock(&list_mutex);
	priv = list_first_entry_or_null(&wmi_list,
			struct privacy_wmi_data,
			list);
	mutex_unlock(&list_mutex);

	return priv;
}



> +{
> +	return privacy_valid;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_state);
> +/*
> + * The flow of privacy event:
> + * 1) User presses key. HW does stuff with this key (timeout is started)
> + * 2) WMI event is emitted from BIOS
> + * 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 defined by
> + *    dell-privacy-acpi driver.
> + *    codec driver will call like this to switch micmute led state.
> + *	ledtrig_audio_set(LED_AUDIO_MICMUTE, 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.
> + */
> +void dell_privacy_process_event(int type, int code, int status)

Please make this return a bool, returning true when it has consumed the event
and false when it did not consume the event. This will allow other 0x0012 0xXXXX
codes to still be handled by the existing dell-wmi.c code for this.

> +{
> +	struct privacy_wmi_data *priv;
> +	const struct key_entry *key;

	bool ret = false;

> +
> +	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);

		ret = true;

> +		break;
> +	case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
> +		priv->last_status = status;
> +		sparse_keymap_report_entry(priv->input_dev, key, 1, true);

		ret = true;

> +		break;

These 2 cases are exactly the same, please merge them into a single case.

> +	default:
> +		dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x",
> +				type, code);
> +	}
> +
> +error:
> +	mutex_unlock(&list_mutex);

	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> +
> +static ssize_t devices_supported_show(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%x\n", priv->features_present);
> +}
> +
> +static ssize_t current_state_show(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%x\n", priv->last_status);
> +}

Please change these to emit strings as discussed above
(assuming you agree with my above proposal for this).

> +
> +static DEVICE_ATTR_RO(devices_supported);
> +static DEVICE_ATTR_RO(current_state);
> +
> +static struct attribute *privacy_attributes[] = {
> +	&dev_attr_devices_supported.attr,
> +	&dev_attr_current_state.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group privacy_attribute_group = {
> +	.attrs = privacy_attributes
> +};
> +
> +/*
> + * 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;
> +	}
> +	/* 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;

As discussed, there is no need for the privacy_valid variable, remove all
references to it here.

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

No need for the goto here just simplify this to:

	if (!keymap)
		return -ENOMEM;

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

You're done with the keymap here, please put a single:

	kfree(keymap);

Here before the if (ret), then you won't need any goto style
cleanup anywhere in this function.

This also fixes not free-ing the keymap when sparse_keymap_setup() fails.

> +	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");

You're not setting ret here.

> +		goto err_free_keymap;
> +	}

No need for the pr_debug, the kernel will already complain if this fails.
Also since all your resources are devm managed and we've already taken
care of the keymap this can be simplified to:

	ret = input_register_device(priv->input_dev);
	if (ret)
		returm ret;

> +	mutex_lock(&list_mutex);
> +	list_add_tail(&priv->list, &wmi_list);
> +	mutex_unlock(&list_mutex);

Move this to directly above the return 0; below, see below.

> +
> +	if (get_current_status(priv->wdev))
> +		goto err_free_input;

You're again not setting ret here before the goto.

And you are returning with an error here after adding the device to the wmi_list,
the list_add_tail call should be done last, directly above the return 0 call.

Also since all your resources are devm managed and we've already taken
care of the keymap this can be simplified to:

	ret = get_current_status(priv->wdev);
	if (ret)
		return ret;

> +
> +	ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
> +	if (ret)
> +		goto err_free_input;

Replace the goto err_free_input() with return ret;

> +
> +	kfree(keymap);

Drop this kfree() (already done above now).

Move the list_add_tail() here.

> +	return 0;
> +
> +err_free_input:
> +	input_unregister_device(priv->input_dev);

This is not necessary for an input_device allocated with devm_input_allocate_device(),
which means that none of these error handling labels are necessary now, so this can
all be removed.



> +err_free_keymap:
> +	privacy_valid = -ENODEV;
> +	kfree(keymap);
> +err_free_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);

The input_unregister is not necessary (and privacy_valid is gone too).

> +
> +	return 0;
> +}
> +
> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> +	{ .guid_string = DELL_PRIVACY_GUID },
> +	{ },
> +};
> +
> +static struct wmi_driver dell_privacy_wmi_driver = {
> +	.driver = {
> +		.name = "dell-privacy",
> +	},
> +	.probe = dell_privacy_wmi_probe,
> +	.remove = dell_privacy_wmi_remove,
> +	.id_table = dell_wmi_privacy_wmi_id_table,
> +};
> +
> +static int __init init_dell_privacy(void)
> +{
> +	int ret;
> +
> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);> +	if (!ret) {
> +		privacy_valid = -ENODEV;
> +		pr_debug("Unable to detect available Dell privacy devices: %d\n", ret);
> +		return privacy_valid;
> +	}

This driver will only get modprobed if the GUID is present and when it is
not present then loading the driver is a no-op so this can be checked.

> +
> +	ret = wmi_driver_register(&dell_privacy_wmi_driver);
> +	if (ret) {
> +		pr_err("failed to initialize privacy wmi driver: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = dell_privacy_acpi_init();
> +	if (ret) {
> +		pr_err("failed to initialize privacy acpi driver: %d\n", ret);
> +		goto err_init;
> +	}

And with the platform-device/driver removed this is not necessary too,
which means ...

> +
> +	return 0;
> +
> +err_init:
> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
> +	return ret;
> +}
> +
> +static void dell_privacy_wmi_exit(void)
> +{
> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
> +}
> +
> +static void __exit exit_dell_privacy(void)
> +{
> +	dell_privacy_wmi_exit();
> +	dell_privacy_acpi_exit();
> +}
> +
> +module_init(init_dell_privacy);
> +module_exit(exit_dell_privacy);

That all of the init_dell_privacy / exit_dell_privacy stuff can be replaced with
a single line:

module_wmi_driver(&dell_privacy_wmi_driver);





> +
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> +MODULE_AUTHOR("Perry Yuan <[e-mail]>");
> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
> new file mode 100644
> index 000000000000..6dda2990df5d
> --- /dev/null
> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Dell privacy notification driver
> + *
> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> + */
> +
> +#ifndef _DELL_PRIVACY_WMI_H_
> +#define _DELL_PRIVACY_WMI_H_
> +
> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> +extern int privacy_valid;

Please remove this from the global kernel namespace
(make it a static global inside the dell-privacy-wmi.c code).

> +int  dell_privacy_state(void);
> +void dell_privacy_process_event(int type, int code, int status);
> +#else /* CONFIG_DELL_PRIVACY */
> +int privacy_valid = -ENODEV;

Drop this.

> +static inline int dell_privacy_state(void)
> +{
> +	return privacy_valid;

And directly return -ENODEV here.

> +}
> +
> +static inline void dell_privacy_process_event(int type, int code, int status)
> +{}
> +#endif /* CONFIG_DELL_PRIVACY */
> +
> +int  dell_privacy_acpi_init(void);
> +void dell_privacy_acpi_exit(void);
> +
> +/* DELL Privacy Type */
> +enum {
> +	DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
> +	DELL_PRIVACY_TYPE_AUDIO,
> +	DELL_PRIVACY_TYPE_CAMERA,
> +};

This enum is only used inside dell-privacy-wmi.c, please move it there.

> +#endif

With the platform-device/driver removal discussed above all this header file
needs to do is defines the prototypes / stubs for 2 functions:

int  dell_privacy_state(void);
void dell_privacy_process_event(int type, int code, int status);

I think it would be best to just add these to dell-smbios.h, rather then
adding a new header just for these 2 prototypes / stubs.


> diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
> index bbdb3e860892..389e86de24b0 100644
> --- a/drivers/platform/x86/dell/dell-wmi.c
> +++ b/drivers/platform/x86/dell/dell-wmi.c
> @@ -13,20 +13,22 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> -#include <linux/kernel.h>
> -#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/dmi.h>
>  #include <linux/init.h>
> -#include <linux/slab.h>
> -#include <linux/types.h>
>  #include <linux/input.h>
>  #include <linux/input/sparse-keymap.h>
> -#include <linux/acpi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
>  #include <linux/string.h>
> -#include <linux/dmi.h>
> +#include <linux/types.h>
>  #include <linux/wmi.h>
>  #include <acpi/video.h>
> +

Please split the changes sorting these includes alphabetically out
into a separate cleanup patch.

>  #include "dell-smbios.h"
>  #include "dell-wmi-descriptor.h"
> +#include "dell-privacy-wmi.h"
>  
>  MODULE_AUTHOR("Matthew Garrett <[e-mail]>");
>  MODULE_AUTHOR("Pali Rohár <[e-mail]>");
> @@ -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;

Drop this.

>  
>  	if (obj->type != ACPI_TYPE_BUFFER) {
>  		pr_warn("bad response type %x\n", obj->type);
> @@ -427,7 +430,6 @@ 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]);
> @@ -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]);

This needs a len > 4 check !

> +			} else {
> +				if (len > 2)
> +					dell_wmi_process_key(wdev, buffer_entry[1],
> +							     buffer_entry[2]);
> +			}

Combined with the change to have dell_privacy_process_event() return a bool, this should
look something like this:

			if (len > 4 && dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
				; /* Handled by dell_privacy_process_event() */
			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]);
> 

Regards,

Hans

Hans de Goede March 24, 2021, 11:39 a.m. UTC | #6

Hi,

On 3/24/21 12:19 PM, Hans de Goede wrote:
> Hi,
> 
> On 3/22/21 10:38 AM, Perry Yuan wrote:
>> From: Perry Yuan <[e-mail]>
>>
>> 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 <[e-mail]>

One last remark, I think that the dell_privacy_present() function which
I suggested in my review should be renamed to:

dell_privacy_has_micmute() and then its return should check the micmute
feature bit in priv->features_present and this new function should then
be used in dell-laptop.c to determine it dell-laptop.c should register
its own mic-mute led_classdev.

This way if a theoretical future laptop shows up where the micmute feature
is not implemented by the privacy interface dell-laptop.c will still
register the non-privacy mic-mute led_classdev.

Regards,

Hans




>> ---
>> v4 -> v5:
>> * addressed feedback from Randy Dunlap
>> * addressed feedback from Pierre-Louis Bossart
>> * rebase to latest 5.12 rc4 upstream kernel
>> * fix some space alignment problem
>> 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/dell/Kconfig             |  16 +
>>  drivers/platform/x86/dell/Makefile            |   3 +
>>  drivers/platform/x86/dell/dell-laptop.c       |  23 +-
>>  drivers/platform/x86/dell/dell-privacy-acpi.c | 164 +++++++++
>>  drivers/platform/x86/dell/dell-privacy-wmi.c  | 340 ++++++++++++++++++
>>  drivers/platform/x86/dell/dell-privacy-wmi.h  |  35 ++
>>  drivers/platform/x86/dell/dell-wmi.c          |  27 +-
>>  8 files changed, 627 insertions(+), 13 deletions(-)
>>  create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
>>  create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c
>>  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
>>  create mode 100644 drivers/platform/x86/dell/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 000000000000..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:	"[e-mail]>"
>> +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:	"[e-mail]>"
>> +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
> 
> What is the definition of enabled/disabled ? Does enabled mean that the device
> is enabled, or that the device is in privacy mode, so in case of the microphone
> it is actually disabled (muted) ?
> 
>> +		- Bit0 -> Microphone
>> +		- Bit1 -> Camera
>> +		- Bit2 -> ePrivacyScreen
>> +
> 
> Normally we would use strings for something like this and not direclty export a bitfield,
> this would also allow avoiding the enabled / disabled ambiguity
> 
> E.g. you could have the following:
> 
> # supported devices
> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported
> Microphone Mute
> Camera Shutter
> ePrivacyScreen
> 
> # All devices in privacy mode:
> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state
> Microphone muted
> Camera Shutter closed
> ePrivacyScreen privacy-filter enabled
> 
> # All devices in non-privacy mode:
> 
> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state
> Microphone unmuted
> Camera Shutter open
> ePrivacyScreen privacy-filter disabled
> 
> You can easily do the bitfield -> string mapping in the kernel by having 3
> string arrays (support, privacy_on, privacy_off) indexed by the bitfield indices.
> 
> 
>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>> index e0a55337f51a..0e0f1eb35bd6 100644
>> --- a/drivers/platform/x86/dell/Kconfig
>> +++ b/drivers/platform/x86/dell/Kconfig
>> @@ -204,4 +204,20 @@ config DELL_WMI_SYSMAN
>>  	  To compile this driver as a module, choose M here: the module will
>>  	  be called dell-wmi-sysman.
>>  
>> +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
>> +	  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.
> 
> This suggests that that the driver implements the buttons, which is not the
> case. How about:
> 
> 	  This driver provides integration with the "Dell Hardware Privacy" feature
> 	  of Dell laptops.
> 
> 
> 	  
> 
>> +
>> +	  To compile this driver as a module, choose M here: the module will
>> +	  be called dell_privacy.
>>  endif # X86_PLATFORM_DRIVERS_DELL
>> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
>> index d720a3e42ae3..8b0883ee9412 100644
>> --- a/drivers/platform/x86/dell/Makefile
>> +++ b/drivers/platform/x86/dell/Makefile
>> @@ -19,3 +19,6 @@ obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
>>  obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.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
>> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
>> index 70edc5bb3a14..4c840ab6cb59 100644
>> --- a/drivers/platform/x86/dell/dell-laptop.c
>> +++ b/drivers/platform/x86/dell/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,6 +92,7 @@ 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");
>> @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
>>  
>>  	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;
>> +		if (!privacy_valid)
> 
> This privacy_valid variable is coming from another module, so you're adding
> a "privacy_valid" symbol to the global kernel symbol namespace, even though this is
> dell specific. Please use the dell_privacy_present() function (see below) instead, so you get:
> 
> 		if (dell_privacy_present())
> 			has_privacy = true;
> 		else
> 			has_privacy = false;
> 
> Which makes much more sense then "if (!privacy_valid) has_privacy = true;" which
> feels as if it is inverted to what it should be.
> 
>> +			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)
>> @@ -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 (!has_privacy)
>> +		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 (!has_privacy)
>> +		led_classdev_unregister(&micmute_led_cdev);
>>  	dell_cleanup_rfkill();
>>  	if (platform_device) {
>>  		platform_device_unregister(platform_device);
>> diff --git a/drivers/platform/x86/dell/dell-privacy-acpi.c b/drivers/platform/x86/dell/dell-privacy-acpi.c
>> new file mode 100644
>> index 000000000000..4c785b62c7c4
>> --- /dev/null
>> +++ b/drivers/platform/x86/dell/dell-privacy-acpi.c
>> @@ -0,0 +1,164 @@
>> +// 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;
> 
> Please use container_of on the passed in led_cdev here instead
> of relying on a global variable.
> 
>> +	acpi_status status;
>> +	acpi_handle handle;
>> +	static char *acpi_method = (char *)"ECAK";
>> +
>> +	handle = ec_get_handle();
>> +	if (!handle)
>> +		return -EIO;
>> +	if (!acpi_has_method(handle, acpi_method))
>> +		return -EIO;
>> +	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;
>> +	}
>> +	pr_debug("set dell privacy micmute ec ack event done\n");
>> +	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)
>> +		return ret;
>> +	return 0;
>> +}
>> +
>> +static int dell_privacy_acpi_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +
>> +	platform_set_drvdata(pdev, privacy_acpi);
>> +	privacy_acpi->dev = &pdev->dev;
>> +	privacy_acpi->platform_device = pdev;
>> +
>> +	ret = dell_privacy_leds_setup(&pdev->dev);
>> +	if (ret)
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>> +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;
>> +	}
> 
> So you're creating a platform_device here, from code called from the
> init_dell_privacy() function in dell-privacy-wmi.c, just so that
> the probe() function for that platform-device can register the
> led_classdev for the (alternative) mute LED.
> 
> Please just put the cdev struct inside struct privacy_wmi_data
> and call dell_privacy_leds_setup() with the wmi-dev as parameter
> from dell_privacy_wmi_probe and have the wmi-dev as the parent
> of the led_classdev.
> 
> This allows removing both the platform-driver and the instantiation
> of the platform-device.
> 
> After removing the unnecessary platfrom-dev/driver there will be
> so little left of dell-privacy-acpi.c, that what remains can and
> should just be folded into dell-privacy-wmi.c. At which point
> you're down to one file for the module which also simplifies the
> Makefile.
> 
>> +
>> +	return 0;
>> +
>> +pdev_err:
>> +	platform_device_unregister(pdev);
>> +pdrv_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 <[e-mail]>");
>> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
>> new file mode 100644
>> index 000000000000..8bde97613477
>> --- /dev/null
>> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
>> @@ -0,0 +1,340 @@
>> +// 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)
>> +
>> +/*
>> + * The wmi_list is used to store the privacy_priv struct with mutex protecting
>> + */
>> +static LIST_HEAD(wmi_list);
>> +static DEFINE_MUTEX(list_mutex);
>> +/*
>> + * global privacy state for other modules to query if the privacy driver loaded successfully
>> + * or no privacy WMI device was detected at initial probe phase
>> + */
>> +int privacy_valid = -EPROBE_DEFER;
>> +EXPORT_SYMBOL_GPL(privacy_valid);
> 
> None of the callers are interpreting the error-codes stored here.
> So this is really treated as a bool, and the info stored in the bool
> is already available inside the wmi_list-head. So just drop this.
> 
>> +
>> +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_state(void)
> 
> Please rename this to dell_privacy_present(), "state" is very generic / not descriptive
> and use list_first_entry_or_null(&wmi_list) as done in dell_privacy_process_event(), e.g.:
> 
> bool dell_privacy_present(void)
> {
> 	struct privacy_wmi_data *priv;
> 
> 	mutex_lock(&list_mutex);
> 	priv = list_first_entry_or_null(&wmi_list,
> 			struct privacy_wmi_data,
> 			list);
> 	mutex_unlock(&list_mutex);
> 
> 	return priv;
> }
> 
> 
> 
>> +{
>> +	return privacy_valid;
>> +}
>> +EXPORT_SYMBOL_GPL(dell_privacy_state);
>> +/*
>> + * The flow of privacy event:
>> + * 1) User presses key. HW does stuff with this key (timeout is started)
>> + * 2) WMI event is emitted from BIOS
>> + * 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 defined by
>> + *    dell-privacy-acpi driver.
>> + *    codec driver will call like this to switch micmute led state.
>> + *	ledtrig_audio_set(LED_AUDIO_MICMUTE, 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.
>> + */
>> +void dell_privacy_process_event(int type, int code, int status)
> 
> Please make this return a bool, returning true when it has consumed the event
> and false when it did not consume the event. This will allow other 0x0012 0xXXXX
> codes to still be handled by the existing dell-wmi.c code for this.
> 
>> +{
>> +	struct privacy_wmi_data *priv;
>> +	const struct key_entry *key;
> 
> 	bool ret = false;
> 
>> +
>> +	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);
> 
> 		ret = true;
> 
>> +		break;
>> +	case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
>> +		priv->last_status = status;
>> +		sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> 
> 		ret = true;
> 
>> +		break;
> 
> These 2 cases are exactly the same, please merge them into a single case.
> 
>> +	default:
>> +		dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x",
>> +				type, code);
>> +	}
>> +
>> +error:
>> +	mutex_unlock(&list_mutex);
> 
> 	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
>> +
>> +static ssize_t devices_supported_show(struct device *dev,
>> +		struct device_attribute *attr,
>> +		char *buf)
>> +{
>> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
>> +
>> +	return sysfs_emit(buf, "%x\n", priv->features_present);
>> +}
>> +
>> +static ssize_t current_state_show(struct device *dev,
>> +		struct device_attribute *attr,
>> +		char *buf)
>> +{
>> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
>> +
>> +	return sysfs_emit(buf, "%x\n", priv->last_status);
>> +}
> 
> Please change these to emit strings as discussed above
> (assuming you agree with my above proposal for this).
> 
>> +
>> +static DEVICE_ATTR_RO(devices_supported);
>> +static DEVICE_ATTR_RO(current_state);
>> +
>> +static struct attribute *privacy_attributes[] = {
>> +	&dev_attr_devices_supported.attr,
>> +	&dev_attr_current_state.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group privacy_attribute_group = {
>> +	.attrs = privacy_attributes
>> +};
>> +
>> +/*
>> + * 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;
>> +	}
>> +	/* 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;
> 
> As discussed, there is no need for the privacy_valid variable, remove all
> references to it here.
> 
>> +
>> +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;
>> +
>> +	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;
>> +	}
> 
> No need for the goto here just simplify this to:
> 
> 	if (!keymap)
> 		return -ENOMEM;
> 
>> +	/* 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);
> 
> You're done with the keymap here, please put a single:
> 
> 	kfree(keymap);
> 
> Here before the if (ret), then you won't need any goto style
> cleanup anywhere in this function.
> 
> This also fixes not free-ing the keymap when sparse_keymap_setup() fails.
> 
>> +	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");
> 
> You're not setting ret here.
> 
>> +		goto err_free_keymap;
>> +	}
> 
> No need for the pr_debug, the kernel will already complain if this fails.
> Also since all your resources are devm managed and we've already taken
> care of the keymap this can be simplified to:
> 
> 	ret = input_register_device(priv->input_dev);
> 	if (ret)
> 		returm ret;
> 
>> +	mutex_lock(&list_mutex);
>> +	list_add_tail(&priv->list, &wmi_list);
>> +	mutex_unlock(&list_mutex);
> 
> Move this to directly above the return 0; below, see below.
> 
>> +
>> +	if (get_current_status(priv->wdev))
>> +		goto err_free_input;
> 
> You're again not setting ret here before the goto.
> 
> And you are returning with an error here after adding the device to the wmi_list,
> the list_add_tail call should be done last, directly above the return 0 call.
> 
> Also since all your resources are devm managed and we've already taken
> care of the keymap this can be simplified to:
> 
> 	ret = get_current_status(priv->wdev);
> 	if (ret)
> 		return ret;
> 
>> +
>> +	ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
>> +	if (ret)
>> +		goto err_free_input;
> 
> Replace the goto err_free_input() with return ret;
> 
>> +
>> +	kfree(keymap);
> 
> Drop this kfree() (already done above now).
> 
> Move the list_add_tail() here.
> 
>> +	return 0;
>> +
>> +err_free_input:
>> +	input_unregister_device(priv->input_dev);
> 
> This is not necessary for an input_device allocated with devm_input_allocate_device(),
> which means that none of these error handling labels are necessary now, so this can
> all be removed.
> 
> 
> 
>> +err_free_keymap:
>> +	privacy_valid = -ENODEV;
>> +	kfree(keymap);
>> +err_free_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);
> 
> The input_unregister is not necessary (and privacy_valid is gone too).
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
>> +	{ .guid_string = DELL_PRIVACY_GUID },
>> +	{ },
>> +};
>> +
>> +static struct wmi_driver dell_privacy_wmi_driver = {
>> +	.driver = {
>> +		.name = "dell-privacy",
>> +	},
>> +	.probe = dell_privacy_wmi_probe,
>> +	.remove = dell_privacy_wmi_remove,
>> +	.id_table = dell_wmi_privacy_wmi_id_table,
>> +};
>> +
>> +static int __init init_dell_privacy(void)
>> +{
>> +	int ret;
>> +
>> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);> +	if (!ret) {
>> +		privacy_valid = -ENODEV;
>> +		pr_debug("Unable to detect available Dell privacy devices: %d\n", ret);
>> +		return privacy_valid;
>> +	}
> 
> This driver will only get modprobed if the GUID is present and when it is
> not present then loading the driver is a no-op so this can be checked.
> 
>> +
>> +	ret = wmi_driver_register(&dell_privacy_wmi_driver);
>> +	if (ret) {
>> +		pr_err("failed to initialize privacy wmi driver: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = dell_privacy_acpi_init();
>> +	if (ret) {
>> +		pr_err("failed to initialize privacy acpi driver: %d\n", ret);
>> +		goto err_init;
>> +	}
> 
> And with the platform-device/driver removed this is not necessary too,
> which means ...
> 
>> +
>> +	return 0;
>> +
>> +err_init:
>> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
>> +	return ret;
>> +}
>> +
>> +static void dell_privacy_wmi_exit(void)
>> +{
>> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
>> +}
>> +
>> +static void __exit exit_dell_privacy(void)
>> +{
>> +	dell_privacy_wmi_exit();
>> +	dell_privacy_acpi_exit();
>> +}
>> +
>> +module_init(init_dell_privacy);
>> +module_exit(exit_dell_privacy);
> 
> That all of the init_dell_privacy / exit_dell_privacy stuff can be replaced with
> a single line:
> 
> module_wmi_driver(&dell_privacy_wmi_driver);
> 
> 
> 
> 
> 
>> +
>> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
>> +MODULE_AUTHOR("Perry Yuan <[e-mail]>");
>> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
>> new file mode 100644
>> index 000000000000..6dda2990df5d
>> --- /dev/null
>> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Dell privacy notification driver
>> + *
>> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
>> + */
>> +
>> +#ifndef _DELL_PRIVACY_WMI_H_
>> +#define _DELL_PRIVACY_WMI_H_
>> +
>> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
>> +extern int privacy_valid;
> 
> Please remove this from the global kernel namespace
> (make it a static global inside the dell-privacy-wmi.c code).
> 
>> +int  dell_privacy_state(void);
>> +void dell_privacy_process_event(int type, int code, int status);
>> +#else /* CONFIG_DELL_PRIVACY */
>> +int privacy_valid = -ENODEV;
> 
> Drop this.
> 
>> +static inline int dell_privacy_state(void)
>> +{
>> +	return privacy_valid;
> 
> And directly return -ENODEV here.
> 
>> +}
>> +
>> +static inline void dell_privacy_process_event(int type, int code, int status)
>> +{}
>> +#endif /* CONFIG_DELL_PRIVACY */
>> +
>> +int  dell_privacy_acpi_init(void);
>> +void dell_privacy_acpi_exit(void);
>> +
>> +/* DELL Privacy Type */
>> +enum {
>> +	DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
>> +	DELL_PRIVACY_TYPE_AUDIO,
>> +	DELL_PRIVACY_TYPE_CAMERA,
>> +};
> 
> This enum is only used inside dell-privacy-wmi.c, please move it there.
> 
>> +#endif
> 
> With the platform-device/driver removal discussed above all this header file
> needs to do is defines the prototypes / stubs for 2 functions:
> 
> int  dell_privacy_state(void);
> void dell_privacy_process_event(int type, int code, int status);
> 
> I think it would be best to just add these to dell-smbios.h, rather then
> adding a new header just for these 2 prototypes / stubs.
> 
> 
>> diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
>> index bbdb3e860892..389e86de24b0 100644
>> --- a/drivers/platform/x86/dell/dell-wmi.c
>> +++ b/drivers/platform/x86/dell/dell-wmi.c
>> @@ -13,20 +13,22 @@
>>  
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>  
>> -#include <linux/kernel.h>
>> -#include <linux/module.h>
>> +#include <linux/acpi.h>
>> +#include <linux/dmi.h>
>>  #include <linux/init.h>
>> -#include <linux/slab.h>
>> -#include <linux/types.h>
>>  #include <linux/input.h>
>>  #include <linux/input/sparse-keymap.h>
>> -#include <linux/acpi.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>>  #include <linux/string.h>
>> -#include <linux/dmi.h>
>> +#include <linux/types.h>
>>  #include <linux/wmi.h>
>>  #include <acpi/video.h>
>> +
> 
> Please split the changes sorting these includes alphabetically out
> into a separate cleanup patch.
> 
>>  #include "dell-smbios.h"
>>  #include "dell-wmi-descriptor.h"
>> +#include "dell-privacy-wmi.h"
>>  
>>  MODULE_AUTHOR("Matthew Garrett <[e-mail]>");
>>  MODULE_AUTHOR("Pali Rohár <[e-mail]>");
>> @@ -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;
> 
> Drop this.
> 
>>  
>>  	if (obj->type != ACPI_TYPE_BUFFER) {
>>  		pr_warn("bad response type %x\n", obj->type);
>> @@ -427,7 +430,6 @@ 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]);
>> @@ -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]);
> 
> This needs a len > 4 check !
> 
>> +			} else {
>> +				if (len > 2)
>> +					dell_wmi_process_key(wdev, buffer_entry[1],
>> +							     buffer_entry[2]);
>> +			}
> 
> Combined with the change to have dell_privacy_process_event() return a bool, this should
> look something like this:
> 
> 			if (len > 4 && dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
> 				; /* Handled by dell_privacy_process_event() */
> 			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]);
>>
> 
> Regards,
> 
> Hans
>

Yuan, Perry March 25, 2021, 7:52 a.m. UTC | #7

Hi Hans.

> -----Original Message-----
> From: Hans de Goede <[e-mail]>
> Sent: Wednesday, March 24, 2021 3:40 AM
> To: Pierre-Louis Bossart; Yuan, Perry; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Limonciello, Mario
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
> > Minor comments below.
> 
> <snip<
> 
> >> +int __init dell_privacy_acpi_init(void)
> >
> > is the __init necessary? You call this routine from another which already has
> this qualifier.
> 
> Yes this is necessary, all functions which are only used during module_load /
> from the module's init function must be marked as __init so that the kernel
> can unload them after module loading is done.
> 
> I do wonder if this one should not be static though (I've not looked at this
> patch in detail yet).
> 
> >
> >> +{
> >> +    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;
> >> +}
> >> +
> >> +void __exit dell_privacy_acpi_exit(void)
> >
> > is the __exit required here?
> 
> Idem. Also static ?
> 
> Regards,
> 
> Hans
> 

If adding static to the function,  I will be worried about that the init and exit cannot be called by another file .



> 
> 
> >
> >> +{
> >> +    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 <[e-mail]>");
> >> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> >> +MODULE_LICENSE("GPL");
> >
> >

Hans de Goede March 25, 2021, 8:23 a.m. UTC | #8

Hi,

On 3/25/21 8:52 AM, Yuan, Perry wrote:
> Hi Hans.
> 
>> -----Original Message-----
>> From: Hans de Goede <[e-mail]>
>> Sent: Wednesday, March 24, 2021 3:40 AM
>> To: Pierre-Louis Bossart; Yuan, Perry; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; Limonciello, Mario
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]
>> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
>> hardware privacy
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Hi,
>>
>> On 3/23/21 7:57 PM, Pierre-Louis Bossart wrote:
>>> Minor comments below.
>>
>> <snip<
>>
>>>> +int __init dell_privacy_acpi_init(void)
>>>
>>> is the __init necessary? You call this routine from another which already has
>> this qualifier.
>>
>> Yes this is necessary, all functions which are only used during module_load /
>> from the module's init function must be marked as __init so that the kernel
>> can unload them after module loading is done.
>>
>> I do wonder if this one should not be static though (I've not looked at this
>> patch in detail yet).
>>
>>>
>>>> +{
>>>> +    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;
>>>> +}
>>>> +
>>>> +void __exit dell_privacy_acpi_exit(void)
>>>
>>> is the __exit required here?
>>
>> Idem. Also static ?
>>
>> Regards,
>>
>> Hans
>>
> 
> If adding static to the function,  I will be worried about that the init and exit cannot be called by another file .

That is right, which is why I added the "?".

But this is no longer relevant after my detailed review of the patch,
so lets discuss things further in the detailed review email-thread.

Regards,

Hans

Yuan, Perry March 25, 2021, 2:53 p.m. UTC | #9

Hi Hans.
Reviewing your feedback and working on next version below
 

> -----Original Message-----
> From: Hans de Goede <[e-mail]>
> Sent: Wednesday, March 24, 2021 7:19 PM
> To: Yuan, Perry; [e-mail]; pierre-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Limonciello, Mario
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v5 1/2] platform/x86: dell-privacy: Add support for Dell
> hardware privacy
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 3/22/21 10:38 AM, Perry Yuan wrote:
> > From: Perry Yuan <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> > v4 -> v5:
> > * addressed feedback from Randy Dunlap
> > * addressed feedback from Pierre-Louis Bossart
> > * rebase to latest 5.12 rc4 upstream kernel
> > * fix some space alignment problem
> > 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/dell/Kconfig             |  16 +
> >  drivers/platform/x86/dell/Makefile            |   3 +
> >  drivers/platform/x86/dell/dell-laptop.c       |  23 +-
> >  drivers/platform/x86/dell/dell-privacy-acpi.c | 164 +++++++++
> > drivers/platform/x86/dell/dell-privacy-wmi.c  | 340 ++++++++++++++++++
> > drivers/platform/x86/dell/dell-privacy-wmi.h  |  35 ++
> >  drivers/platform/x86/dell/dell-wmi.c          |  27 +-
> >  8 files changed, 627 insertions(+), 13 deletions(-)  create mode
> > 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
> >  create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c
> >  create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
> >  create mode 100644 drivers/platform/x86/dell/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 000000000000..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:	"[e-mail]>"
> > +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:	"[e-mail]>"
> > +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
> 
> What is the definition of enabled/disabled ? Does enabled mean that the
> device is enabled, or that the device is in privacy mode, so in case of the
> microphone it is actually disabled (muted) ?
> 
> > +		- Bit0 -> Microphone
> > +		- Bit1 -> Camera
> > +		- Bit2 -> ePrivacyScreen
> > +
> 
> Normally we would use strings for something like this and not direclty export
> a bitfield, this would also allow avoiding the enabled / disabled ambiguity
> 
> E.g. you could have the following:
> 
> # supported devices
> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/devices_supported
> Microphone Mute
> Camera Shutter
> ePrivacyScreen
> 
> # All devices in privacy mode:
> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/current_state
> Microphone muted
> Camera Shutter closed
> ePrivacyScreen privacy-filter enabled
> 
> # All devices in non-privacy mode:
> 
> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-
> D3AB0A901919/current_state
> Microphone unmuted
> Camera Shutter open
> ePrivacyScreen privacy-filter disabled
> 
> You can easily do the bitfield -> string mapping in the kernel by having 3
> string arrays (support, privacy_on, privacy_off) indexed by the bitfield indices.
> 
> 
> > diff --git a/drivers/platform/x86/dell/Kconfig
> > b/drivers/platform/x86/dell/Kconfig
> > index e0a55337f51a..0e0f1eb35bd6 100644
> > --- a/drivers/platform/x86/dell/Kconfig
> > +++ b/drivers/platform/x86/dell/Kconfig
> > @@ -204,4 +204,20 @@ config DELL_WMI_SYSMAN
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called dell-wmi-sysman.
> >
> > +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
> > +	  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.
> 
> This suggests that that the driver implements the buttons, which is not the
> case. How about:
> 
> 	  This driver provides integration with the "Dell Hardware Privacy"
> feature
> 	  of Dell laptops.
> 
> 
> 
> 
> > +
> > +	  To compile this driver as a module, choose M here: the module will
> > +	  be called dell_privacy.
> >  endif # X86_PLATFORM_DRIVERS_DELL
> > diff --git a/drivers/platform/x86/dell/Makefile
> > b/drivers/platform/x86/dell/Makefile
> > index d720a3e42ae3..8b0883ee9412 100644
> > --- a/drivers/platform/x86/dell/Makefile
> > +++ b/drivers/platform/x86/dell/Makefile
> > @@ -19,3 +19,6 @@ obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-
> aio.o
> >  obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.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
> > diff --git a/drivers/platform/x86/dell/dell-laptop.c
> > b/drivers/platform/x86/dell/dell-laptop.c
> > index 70edc5bb3a14..4c840ab6cb59 100644
> > --- a/drivers/platform/x86/dell/dell-laptop.c
> > +++ b/drivers/platform/x86/dell/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,6 +92,7 @@ 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"); @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
> >
> >  	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;
> > +		if (!privacy_valid)
> 
> This privacy_valid variable is coming from another module, so you're adding a
> "privacy_valid" symbol to the global kernel symbol namespace, even though
> this is dell specific. Please use the dell_privacy_present() function (see below)
> instead, so you get:
> 
> 		if (dell_privacy_present())
> 			has_privacy = true;
> 		else
> 			has_privacy = false;
> 
> Which makes much more sense then "if (!privacy_valid) has_privacy = true;"
> which feels as if it is inverted to what it should be.
> 
> > +			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) @@
> > -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 (!has_privacy)
> > +		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 (!has_privacy)
> > +		led_classdev_unregister(&micmute_led_cdev);
> >  	dell_cleanup_rfkill();
> >  	if (platform_device) {
> >  		platform_device_unregister(platform_device);
> > diff --git a/drivers/platform/x86/dell/dell-privacy-acpi.c
> > b/drivers/platform/x86/dell/dell-privacy-acpi.c
> > new file mode 100644
> > index 000000000000..4c785b62c7c4
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell/dell-privacy-acpi.c
> > @@ -0,0 +1,164 @@
> > +// 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;
> 
> Please use container_of on the passed in led_cdev here instead of relying on
> a global variable.
> 
> > +	acpi_status status;
> > +	acpi_handle handle;
> > +	static char *acpi_method = (char *)"ECAK";
> > +
> > +	handle = ec_get_handle();
> > +	if (!handle)
> > +		return -EIO;
> > +	if (!acpi_has_method(handle, acpi_method))
> > +		return -EIO;
> > +	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;
> > +	}
> > +	pr_debug("set dell privacy micmute ec ack event done\n");
> > +	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)
> > +		return ret;
> > +	return 0;
> > +}
> > +
> > +static int dell_privacy_acpi_probe(struct platform_device *pdev) {
> > +	int ret;
> > +
> > +	platform_set_drvdata(pdev, privacy_acpi);
> > +	privacy_acpi->dev = &pdev->dev;
> > +	privacy_acpi->platform_device = pdev;
> > +
> > +	ret = dell_privacy_leds_setup(&pdev->dev);
> > +	if (ret)
> > +		return -EIO;
> > +
> > +	return 0;
> > +}
> > +
> > +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;
> > +	}
> 
> So you're creating a platform_device here, from code called from the
> init_dell_privacy() function in dell-privacy-wmi.c, just so that the probe()
> function for that platform-device can register the led_classdev for the
> (alternative) mute LED.
> 
> Please just put the cdev struct inside struct privacy_wmi_data and call
> dell_privacy_leds_setup() with the wmi-dev as parameter from
> dell_privacy_wmi_probe and have the wmi-dev as the parent of the
> led_classdev.
> 
> This allows removing both the platform-driver and the instantiation of the
> platform-device.
> 
> After removing the unnecessary platfrom-dev/driver there will be so little left
> of dell-privacy-acpi.c, that what remains can and should just be folded into
> dell-privacy-wmi.c. At which point you're down to one file for the module
> which also simplifies the Makefile.
> 
> > +
> > +	return 0;
> > +
> > +pdev_err:
> > +	platform_device_unregister(pdev);
> > +pdrv_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 <[e-mail]>");
> > +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c
> > b/drivers/platform/x86/dell/dell-privacy-wmi.c
> > new file mode 100644
> > index 000000000000..8bde97613477
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
> > @@ -0,0 +1,340 @@
> > +// 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)
> > +
> > +/*
> > + * The wmi_list is used to store the privacy_priv struct with mutex
> > +protecting  */ static LIST_HEAD(wmi_list); static
> > +DEFINE_MUTEX(list_mutex);
> > +/*
> > + * global privacy state for other modules to query if the privacy
> > +driver loaded successfully
> > + * or no privacy WMI device was detected at initial probe phase  */
> > +int privacy_valid = -EPROBE_DEFER; EXPORT_SYMBOL_GPL(privacy_valid);
> 
> None of the callers are interpreting the error-codes stored here.
> So this is really treated as a bool, and the info stored in the bool is already
> available inside the wmi_list-head. So just drop this.
> 
> > +
> > +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_state(void)
> 
> Please rename this to dell_privacy_present(), "state" is very generic / not
> descriptive and use list_first_entry_or_null(&wmi_list) as done in
> dell_privacy_process_event(), e.g.:
> 
> bool dell_privacy_present(void)
> {
> 	struct privacy_wmi_data *priv;
> 
> 	mutex_lock(&list_mutex);
> 	priv = list_first_entry_or_null(&wmi_list,
> 			struct privacy_wmi_data,
> 			list);
> 	mutex_unlock(&list_mutex);
> 
> 	return priv;
> }
> 
> 
> 
> > +{
> > +	return privacy_valid;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_privacy_state);
> > +/*
> > + * The flow of privacy event:
> > + * 1) User presses key. HW does stuff with this key (timeout is
> > +started)
> > + * 2) WMI event is emitted from BIOS
> > + * 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 defined by
> > + *    dell-privacy-acpi driver.
> > + *    codec driver will call like this to switch micmute led state.
> > + *	ledtrig_audio_set(LED_AUDIO_MICMUTE, 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.
> > + */
> > +void dell_privacy_process_event(int type, int code, int status)
> 
> Please make this return a bool, returning true when it has consumed the
> event and false when it did not consume the event. This will allow other
> 0x0012 0xXXXX codes to still be handled by the existing dell-wmi.c code for
> this.
> 
> > +{
> > +	struct privacy_wmi_data *priv;
> > +	const struct key_entry *key;
> 
> 	bool ret = false;
> 
> > +
> > +	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);
> 
> 		ret = true;
> 
> > +		break;
> > +	case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
> > +		priv->last_status = status;
> > +		sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> 
> 		ret = true;
> 
> > +		break;
> 
> These 2 cases are exactly the same, please merge them into a single case.
> 
> > +	default:
> > +		dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x
> 0x%04x",
> > +				type, code);
> > +	}
> > +
> > +error:
> > +	mutex_unlock(&list_mutex);
> 
> 	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
> > +
> > +static ssize_t devices_supported_show(struct device *dev,
> > +		struct device_attribute *attr,
> > +		char *buf)
> > +{
> > +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> > +
> > +	return sysfs_emit(buf, "%x\n", priv->features_present); }
> > +
> > +static ssize_t current_state_show(struct device *dev,
> > +		struct device_attribute *attr,
> > +		char *buf)
> > +{
> > +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
> > +
> > +	return sysfs_emit(buf, "%x\n", priv->last_status); }
> 
> Please change these to emit strings as discussed above (assuming you agree
> with my above proposal for this).
> 
> > +
> > +static DEVICE_ATTR_RO(devices_supported);
> > +static DEVICE_ATTR_RO(current_state);
> > +
> > +static struct attribute *privacy_attributes[] = {
> > +	&dev_attr_devices_supported.attr,
> > +	&dev_attr_current_state.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group privacy_attribute_group = {
> > +	.attrs = privacy_attributes
> > +};
> > +
> > +/*
> > + * 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;
> > +	}
> > +	/* 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;
> 
> As discussed, there is no need for the privacy_valid variable, remove all
> references to it here.
> 
> > +
> > +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;
> > +
> > +	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;
> > +	}
> 
> No need for the goto here just simplify this to:
> 
> 	if (!keymap)
> 		return -ENOMEM;
> 
> > +	/* 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);
> 
> You're done with the keymap here, please put a single:
> 
> 	kfree(keymap);
> 
> Here before the if (ret), then you won't need any goto style cleanup
> anywhere in this function.
> 
> This also fixes not free-ing the keymap when sparse_keymap_setup() fails.
> 
> > +	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");
> 
> You're not setting ret here.
> 
> > +		goto err_free_keymap;
> > +	}
> 
> No need for the pr_debug, the kernel will already complain if this fails.
> Also since all your resources are devm managed and we've already taken care
> of the keymap this can be simplified to:
> 
> 	ret = input_register_device(priv->input_dev);
> 	if (ret)
> 		returm ret;
> 
> > +	mutex_lock(&list_mutex);
> > +	list_add_tail(&priv->list, &wmi_list);
> > +	mutex_unlock(&list_mutex);
> 
> Move this to directly above the return 0; below, see below.
> 
> > +
> > +	if (get_current_status(priv->wdev))
> > +		goto err_free_input;
> 
> You're again not setting ret here before the goto.
> 
> And you are returning with an error here after adding the device to the
> wmi_list, the list_add_tail call should be done last, directly above the return 0
> call.
> 
> Also since all your resources are devm managed and we've already taken care
> of the keymap this can be simplified to:
> 
> 	ret = get_current_status(priv->wdev);
> 	if (ret)
> 		return ret;
> 
> > +
> > +	ret = devm_device_add_group(&wdev->dev,
> &privacy_attribute_group);
> > +	if (ret)
> > +		goto err_free_input;
> 
> Replace the goto err_free_input() with return ret;
> 
> > +
> > +	kfree(keymap);
> 
> Drop this kfree() (already done above now).
> 
> Move the list_add_tail() here.
> 
> > +	return 0;
> > +
> > +err_free_input:
> > +	input_unregister_device(priv->input_dev);
> 
> This is not necessary for an input_device allocated with
> devm_input_allocate_device(), which means that none of these error
> handling labels are necessary now, so this can all be removed.
> 
> 
> 
> > +err_free_keymap:
> > +	privacy_valid = -ENODEV;
> > +	kfree(keymap);
> > +err_free_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);
> 
> The input_unregister is not necessary (and privacy_valid is gone too).
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
> > +	{ .guid_string = DELL_PRIVACY_GUID },
> > +	{ },
> > +};
> > +
> > +static struct wmi_driver dell_privacy_wmi_driver = {
> > +	.driver = {
> > +		.name = "dell-privacy",
> > +	},
> > +	.probe = dell_privacy_wmi_probe,
> > +	.remove = dell_privacy_wmi_remove,
> > +	.id_table = dell_wmi_privacy_wmi_id_table, };
> > +
> > +static int __init init_dell_privacy(void) {
> > +	int ret;
> > +
> > +	ret = wmi_has_guid(DELL_PRIVACY_GUID);> +	if (!ret) {
> > +		privacy_valid = -ENODEV;
> > +		pr_debug("Unable to detect available Dell privacy
> devices: %d\n", ret);
> > +		return privacy_valid;
> > +	}
> 
> This driver will only get modprobed if the GUID is present and when it is not
> present then loading the driver is a no-op so this can be checked.
> 
> > +
> > +	ret = wmi_driver_register(&dell_privacy_wmi_driver);
> > +	if (ret) {
> > +		pr_err("failed to initialize privacy wmi driver: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = dell_privacy_acpi_init();
> > +	if (ret) {
> > +		pr_err("failed to initialize privacy acpi driver: %d\n", ret);
> > +		goto err_init;
> > +	}
> 
> And with the platform-device/driver removed this is not necessary too, which
> means ...
> 
> > +
> > +	return 0;
> > +
> > +err_init:
> > +	wmi_driver_unregister(&dell_privacy_wmi_driver);
> > +	return ret;
> > +}
> > +
> > +static void dell_privacy_wmi_exit(void) {
> > +	wmi_driver_unregister(&dell_privacy_wmi_driver);
> > +}
> > +
> > +static void __exit exit_dell_privacy(void) {
> > +	dell_privacy_wmi_exit();
> > +	dell_privacy_acpi_exit();
> > +}
> > +
> > +module_init(init_dell_privacy);
> > +module_exit(exit_dell_privacy);
> 
> That all of the init_dell_privacy / exit_dell_privacy stuff can be replaced with
> a single line:
> 
> module_wmi_driver(&dell_privacy_wmi_driver);
> 
> 
> 
> 
> 
> > +
> > +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
> > +MODULE_AUTHOR("Perry Yuan <[e-mail]>");
> > +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
> MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h
> > b/drivers/platform/x86/dell/dell-privacy-wmi.h
> > new file mode 100644
> > index 000000000000..6dda2990df5d
> > --- /dev/null
> > +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Dell privacy notification driver
> > + *
> > + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
> > + */
> > +
> > +#ifndef _DELL_PRIVACY_WMI_H_
> > +#define _DELL_PRIVACY_WMI_H_
> > +
> > +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
> > +extern int privacy_valid;
> 
> Please remove this from the global kernel namespace (make it a static global
> inside the dell-privacy-wmi.c code).
> 
> > +int  dell_privacy_state(void);
> > +void dell_privacy_process_event(int type, int code, int status);
> > +#else /* CONFIG_DELL_PRIVACY */ int privacy_valid = -ENODEV;
> 
> Drop this.
> 
> > +static inline int dell_privacy_state(void) {
> > +	return privacy_valid;
> 
> And directly return -ENODEV here.
> 
> > +}
> > +
> > +static inline void dell_privacy_process_event(int type, int code, int
> > +status) {} #endif /* CONFIG_DELL_PRIVACY */
> > +
> > +int  dell_privacy_acpi_init(void);
> > +void dell_privacy_acpi_exit(void);
> > +
> > +/* DELL Privacy Type */
> > +enum {
> > +	DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
> > +	DELL_PRIVACY_TYPE_AUDIO,
> > +	DELL_PRIVACY_TYPE_CAMERA,
> > +};
> 
> This enum is only used inside dell-privacy-wmi.c, please move it there.
> 
> > +#endif
> 
> With the platform-device/driver removal discussed above all this header file
> needs to do is defines the prototypes / stubs for 2 functions:
> 
> int  dell_privacy_state(void);
> void dell_privacy_process_event(int type, int code, int status);
> 
> I think it would be best to just add these to dell-smbios.h, rather then adding
> a new header just for these 2 prototypes / stubs.
> 
> 
> > diff --git a/drivers/platform/x86/dell/dell-wmi.c
> > b/drivers/platform/x86/dell/dell-wmi.c
> > index bbdb3e860892..389e86de24b0 100644
> > --- a/drivers/platform/x86/dell/dell-wmi.c
> > +++ b/drivers/platform/x86/dell/dell-wmi.c
> > @@ -13,20 +13,22 @@
> >
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > -#include <linux/kernel.h>
> > -#include <linux/module.h>
> > +#include <linux/acpi.h>
> > +#include <linux/dmi.h>
> >  #include <linux/init.h>
> > -#include <linux/slab.h>
> > -#include <linux/types.h>
> >  #include <linux/input.h>
> >  #include <linux/input/sparse-keymap.h> -#include <linux/acpi.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> >  #include <linux/string.h>
> > -#include <linux/dmi.h>
> > +#include <linux/types.h>
> >  #include <linux/wmi.h>
> >  #include <acpi/video.h>
> > +
> 
> Please split the changes sorting these includes alphabetically out into a
> separate cleanup patch.
> 
> >  #include "dell-smbios.h"
> >  #include "dell-wmi-descriptor.h"
> > +#include "dell-privacy-wmi.h"
> >
> >  MODULE_AUTHOR("Matthew Garrett <[e-mail]>");
> > MODULE_AUTHOR("Pali Rohár <[e-mail]>"); @@ -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;
> 
> Drop this.
> 
> >
> >  	if (obj->type != ACPI_TYPE_BUFFER) {
> >  		pr_warn("bad response type %x\n", obj->type); @@ -427,7
> +430,6 @@
> > 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]);
> > @@ -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]);
> 
> This needs a len > 4 check !
> 
> > +			} else {
> > +				if (len > 2)
> > +					dell_wmi_process_key(wdev,
> buffer_entry[1],
> > +							     buffer_entry[2]);
> > +			}
> 
> Combined with the change to have dell_privacy_process_event() return a
> bool, this should look something like this:
> 
> 			if (len > 4 &&
> dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
> 				; /* Handled by dell_privacy_process_event()
> */
> 			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]);
> >
> 
> Regards,
> 
> Hans

Perry Yuan March 27, 2021, 3:39 a.m. UTC | #10

Hi Pierre

Thanks for your review .

I changed the patch and explain the review comments as below

(Maybe the mail format has problem, sent from one new system thunderbird :)

On 3/23/21 2:57 PM, Pierre-Louis Bossart wrote:
> Minor comments below.
>
> On 3/22/21 4:38 AM, Perry Yuan wrote:
>> From: Perry Yuan <[e-mail]>
>>
>> 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
>
> typo: emits
fixed
>
>> SW_CAMERA_LENS_COVER event while the camera lens shutter will be
>> changed by EC & hw(hadware) control
>
> typo: hardware
fixed
>>
>> *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.
>
> what happens if there is timeout? You have an explicit description of 
> the timer handling in the success case, but not what happens on a 
> timeout...
>
add more explicit description for timeout triggered case

7) If "LED" is set to on dell-privacy notifies EC, and timeout is cancelled.

     HW mic mute activated. If EC not notified,HW mic mute will also be

     activated when timeout used up, it is just later than active ack

>> 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 000000000000..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
>
> 5.13 now?
changed to 5.13
>
>> +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;
>> +    static char *acpi_method = (char *)"ECAK";
>> +
>> +    handle = ec_get_handle();
>> +    if (!handle)
>> +        return -EIO;
>> +    if (!acpi_has_method(handle, acpi_method))
>> +        return -EIO;
>> +    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;
>> +    }
>> +    pr_debug("set dell privacy micmute ec ack event done\n");
>
> can we use dev_dbg() here? Same for all occurrences of pr_debug and 
> pr_err below, it's cleaner and easier to filter.
>
I changed some pr_xx to dev_xxx ,  but below code will be more complex 
to use dev_xxx to print the

log , because it need to get the priv->dev, but it is not registered at 
this time , I would prefer to keep use the pr_debug here.  and some 
other cases where  "priv->dev" cannot be used.


  static int __init init_dell_privacy(void)

{
         int ret;

         ret = wmi_has_guid(DELL_PRIVACY_GUID);
         if (!ret) {
                 privacy_valid = -ENODEV;
                 pr_debug("Unable to detect available Dell privacy 
devices: %d\n", ret);
                 return privacy_valid;
         }

>> +    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;
>
> useless init

Sorry, I do not get this point .

int should be needed to return error code if devm_led_classdev_register 
failed.


>
>> +
>> +    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)
>> +        return ret;
>> +    return 0;
>> +}
>> +
>> +static int dell_privacy_acpi_probe(struct platform_device *pdev)
>> +{
>> +    int ret;
>> +
>> +    platform_set_drvdata(pdev, privacy_acpi);
>> +    privacy_acpi->dev = &pdev->dev;
>> +    privacy_acpi->platform_device = pdev;
>> +
>> +    ret = dell_privacy_leds_setup(&pdev->dev);
>> +    if (ret)
>> +        return -EIO;
>
> any reason to hard-code -EIO, woud 'return ret' be enough?
>
>
fixed to use return ret
>> +
>> +    return 0;
>> +}
>> +
>> +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)
>
> is the __init necessary? You call this routine from another which 
> already has this qualifier.
Yes, I need to add this when driver loading and kernel will free __init 
section mem after driver registered.
>
>> +{
>> +    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;
>> +}
>> +
>> +void __exit dell_privacy_acpi_exit(void)
>
> is the __exit required here?
same reason as __init
>
>> +{
>> +    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 <[e-mail]>");
>> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
>> +MODULE_LICENSE("GPL");
>
>

Perry Yuan March 28, 2021, 2:32 p.m. UTC | #11

Hi Hans.

Thanks for your suggestion helping to improve the patch.


On 3/24/21 7:39 AM, Hans de Goede wrote:
> Hi,
>
> On 3/24/21 12:19 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 3/22/21 10:38 AM, Perry Yuan wrote:
>>> From: Perry Yuan <[e-mail]>
>>>
>>> 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 <[e-mail]>
> One last remark, I think that the dell_privacy_present() function which
> I suggested in my review should be renamed to:
>
> dell_privacy_has_micmute() and then its return should check the micmute
> feature bit in priv->features_present and this new function should then
> be used in dell-laptop.c to determine it dell-laptop.c should register
> its own mic-mute led_classdev.
>
> This way if a theoretical future laptop shows up where the micmute feature
> is not implemented by the privacy interface dell-laptop.c will still
> register the non-privacy mic-mute led_classdev.
>
> Regards,
>
> Hans

Yes, I rename the micmute checking function to dell_privacy_has_micmute.

and i rewrite it to simplify the micmute checking process.

I am doing some testing and drive other feedback to be finished.

Will post V6 soon for your confirm.

Perry.

>
>
>
>
>>> ---
>>> v4 -> v5:
>>> * addressed feedback from Randy Dunlap
>>> * addressed feedback from Pierre-Louis Bossart
>>> * rebase to latest 5.12 rc4 upstream kernel
>>> * fix some space alignment problem
>>> 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/dell/Kconfig             |  16 +
>>>   drivers/platform/x86/dell/Makefile            |   3 +
>>>   drivers/platform/x86/dell/dell-laptop.c       |  23 +-
>>>   drivers/platform/x86/dell/dell-privacy-acpi.c | 164 +++++++++
>>>   drivers/platform/x86/dell/dell-privacy-wmi.c  | 340 ++++++++++++++++++
>>>   drivers/platform/x86/dell/dell-privacy-wmi.h  |  35 ++
>>>   drivers/platform/x86/dell/dell-wmi.c          |  27 +-
>>>   8 files changed, 627 insertions(+), 13 deletions(-)
>>>   create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-privacy-wmi
>>>   create mode 100644 drivers/platform/x86/dell/dell-privacy-acpi.c
>>>   create mode 100644 drivers/platform/x86/dell/dell-privacy-wmi.c
>>>   create mode 100644 drivers/platform/x86/dell/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 000000000000..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:	"[e-mail]>"
>>> +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:	"[e-mail]>"
>>> +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
>> What is the definition of enabled/disabled ? Does enabled mean that the device
>> is enabled, or that the device is in privacy mode, so in case of the microphone
>> it is actually disabled (muted) ?
>>
>>> +		- Bit0 -> Microphone
>>> +		- Bit1 -> Camera
>>> +		- Bit2 -> ePrivacyScreen
>>> +
>> Normally we would use strings for something like this and not direclty export a bitfield,
>> this would also allow avoiding the enabled / disabled ambiguity
>>
>> E.g. you could have the following:
>>
>> # supported devices
>> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/devices_supported
>> Microphone Mute
>> Camera Shutter
>> ePrivacyScreen
>>
>> # All devices in privacy mode:
>> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state
>> Microphone muted
>> Camera Shutter closed
>> ePrivacyScreen privacy-filter enabled
>>
>> # All devices in non-privacy mode:
>>
>> $ cat /sys/bus/wmi/devices/6932965F-1671-4CEB-B988-D3AB0A901919/current_state
>> Microphone unmuted
>> Camera Shutter open
>> ePrivacyScreen privacy-filter disabled
>>
>> You can easily do the bitfield -> string mapping in the kernel by having 3
>> string arrays (support, privacy_on, privacy_off) indexed by the bitfield indices.
>>
>>
>>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>>> index e0a55337f51a..0e0f1eb35bd6 100644
>>> --- a/drivers/platform/x86/dell/Kconfig
>>> +++ b/drivers/platform/x86/dell/Kconfig
>>> @@ -204,4 +204,20 @@ config DELL_WMI_SYSMAN
>>>   	  To compile this driver as a module, choose M here: the module will
>>>   	  be called dell-wmi-sysman.
>>>   
>>> +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
>>> +	  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.
>> This suggests that that the driver implements the buttons, which is not the
>> case. How about:
>>
>> 	  This driver provides integration with the "Dell Hardware Privacy" feature
>> 	  of Dell laptops.
>>
>>
>> 	
>>
>>> +
>>> +	  To compile this driver as a module, choose M here: the module will
>>> +	  be called dell_privacy.
>>>   endif # X86_PLATFORM_DRIVERS_DELL
>>> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
>>> index d720a3e42ae3..8b0883ee9412 100644
>>> --- a/drivers/platform/x86/dell/Makefile
>>> +++ b/drivers/platform/x86/dell/Makefile
>>> @@ -19,3 +19,6 @@ obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
>>>   obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.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
>>> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
>>> index 70edc5bb3a14..4c840ab6cb59 100644
>>> --- a/drivers/platform/x86/dell/dell-laptop.c
>>> +++ b/drivers/platform/x86/dell/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,6 +92,7 @@ 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");
>>> @@ -2206,10 +2209,16 @@ static int __init dell_init(void)
>>>   
>>>   	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;
>>> +		if (!privacy_valid)
>> This privacy_valid variable is coming from another module, so you're adding
>> a "privacy_valid" symbol to the global kernel symbol namespace, even though this is
>> dell specific. Please use the dell_privacy_present() function (see below) instead, so you get:
>>
>> 		if (dell_privacy_present())
>> 			has_privacy = true;
>> 		else
>> 			has_privacy = false;
>>
>> Which makes much more sense then "if (!privacy_valid) has_privacy = true;" which
>> feels as if it is inverted to what it should be.
>>
>>> +			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)
>>> @@ -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 (!has_privacy)
>>> +		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 (!has_privacy)
>>> +		led_classdev_unregister(&micmute_led_cdev);
>>>   	dell_cleanup_rfkill();
>>>   	if (platform_device) {
>>>   		platform_device_unregister(platform_device);
>>> diff --git a/drivers/platform/x86/dell/dell-privacy-acpi.c b/drivers/platform/x86/dell/dell-privacy-acpi.c
>>> new file mode 100644
>>> index 000000000000..4c785b62c7c4
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/dell/dell-privacy-acpi.c
>>> @@ -0,0 +1,164 @@
>>> +// 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;
>> Please use container_of on the passed in led_cdev here instead
>> of relying on a global variable.
>>
>>> +	acpi_status status;
>>> +	acpi_handle handle;
>>> +	static char *acpi_method = (char *)"ECAK";
>>> +
>>> +	handle = ec_get_handle();
>>> +	if (!handle)
>>> +		return -EIO;
>>> +	if (!acpi_has_method(handle, acpi_method))
>>> +		return -EIO;
>>> +	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;
>>> +	}
>>> +	pr_debug("set dell privacy micmute ec ack event done\n");
>>> +	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)
>>> +		return ret;
>>> +	return 0;
>>> +}
>>> +
>>> +static int dell_privacy_acpi_probe(struct platform_device *pdev)
>>> +{
>>> +	int ret;
>>> +
>>> +	platform_set_drvdata(pdev, privacy_acpi);
>>> +	privacy_acpi->dev = &pdev->dev;
>>> +	privacy_acpi->platform_device = pdev;
>>> +
>>> +	ret = dell_privacy_leds_setup(&pdev->dev);
>>> +	if (ret)
>>> +		return -EIO;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +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;
>>> +	}
>> So you're creating a platform_device here, from code called from the
>> init_dell_privacy() function in dell-privacy-wmi.c, just so that
>> the probe() function for that platform-device can register the
>> led_classdev for the (alternative) mute LED.
>>
>> Please just put the cdev struct inside struct privacy_wmi_data
>> and call dell_privacy_leds_setup() with the wmi-dev as parameter
>> from dell_privacy_wmi_probe and have the wmi-dev as the parent
>> of the led_classdev.
>>
>> This allows removing both the platform-driver and the instantiation
>> of the platform-device.
>>
>> After removing the unnecessary platfrom-dev/driver there will be
>> so little left of dell-privacy-acpi.c, that what remains can and
>> should just be folded into dell-privacy-wmi.c. At which point
>> you're down to one file for the module which also simplifies the
>> Makefile.
>>
>>> +
>>> +	return 0;
>>> +
>>> +pdev_err:
>>> +	platform_device_unregister(pdev);
>>> +pdrv_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 <[e-mail]>");
>>> +MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
>>> new file mode 100644
>>> index 000000000000..8bde97613477
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
>>> @@ -0,0 +1,340 @@
>>> +// 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)
>>> +
>>> +/*
>>> + * The wmi_list is used to store the privacy_priv struct with mutex protecting
>>> + */
>>> +static LIST_HEAD(wmi_list);
>>> +static DEFINE_MUTEX(list_mutex);
>>> +/*
>>> + * global privacy state for other modules to query if the privacy driver loaded successfully
>>> + * or no privacy WMI device was detected at initial probe phase
>>> + */
>>> +int privacy_valid = -EPROBE_DEFER;
>>> +EXPORT_SYMBOL_GPL(privacy_valid);
>> None of the callers are interpreting the error-codes stored here.
>> So this is really treated as a bool, and the info stored in the bool
>> is already available inside the wmi_list-head. So just drop this.
>>
>>> +
>>> +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_state(void)
>> Please rename this to dell_privacy_present(), "state" is very generic / not descriptive
>> and use list_first_entry_or_null(&wmi_list) as done in dell_privacy_process_event(), e.g.:
>>
>> bool dell_privacy_present(void)
>> {
>> 	struct privacy_wmi_data *priv;
>>
>> 	mutex_lock(&list_mutex);
>> 	priv = list_first_entry_or_null(&wmi_list,
>> 			struct privacy_wmi_data,
>> 			list);
>> 	mutex_unlock(&list_mutex);
>>
>> 	return priv;
>> }
>>
>>
>>
>>> +{
>>> +	return privacy_valid;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dell_privacy_state);
>>> +/*
>>> + * The flow of privacy event:
>>> + * 1) User presses key. HW does stuff with this key (timeout is started)
>>> + * 2) WMI event is emitted from BIOS
>>> + * 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 defined by
>>> + *    dell-privacy-acpi driver.
>>> + *    codec driver will call like this to switch micmute led state.
>>> + *	ledtrig_audio_set(LED_AUDIO_MICMUTE, 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.
>>> + */
>>> +void dell_privacy_process_event(int type, int code, int status)
>> Please make this return a bool, returning true when it has consumed the event
>> and false when it did not consume the event. This will allow other 0x0012 0xXXXX
>> codes to still be handled by the existing dell-wmi.c code for this.
>>
>>> +{
>>> +	struct privacy_wmi_data *priv;
>>> +	const struct key_entry *key;
>> 	bool ret = false;
>>
>>> +
>>> +	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);
>> 		ret = true;
>>
>>> +		break;
>>> +	case DELL_PRIVACY_TYPE_CAMERA: /* Camera mute */
>>> +		priv->last_status = status;
>>> +		sparse_keymap_report_entry(priv->input_dev, key, 1, true);
>> 		ret = true;
>>
>>> +		break;
>> These 2 cases are exactly the same, please merge them into a single case.
>>
>>> +	default:
>>> +		dev_dbg(&priv->wdev->dev, "unknown event type 0x%04x 0x%04x",
>>> +				type, code);
>>> +	}
>>> +
>>> +error:
>>> +	mutex_unlock(&list_mutex);
>> 	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(dell_privacy_process_event);
>>> +
>>> +static ssize_t devices_supported_show(struct device *dev,
>>> +		struct device_attribute *attr,
>>> +		char *buf)
>>> +{
>>> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
>>> +
>>> +	return sysfs_emit(buf, "%x\n", priv->features_present);
>>> +}
>>> +
>>> +static ssize_t current_state_show(struct device *dev,
>>> +		struct device_attribute *attr,
>>> +		char *buf)
>>> +{
>>> +	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
>>> +
>>> +	return sysfs_emit(buf, "%x\n", priv->last_status);
>>> +}
>> Please change these to emit strings as discussed above
>> (assuming you agree with my above proposal for this).
>>
>>> +
>>> +static DEVICE_ATTR_RO(devices_supported);
>>> +static DEVICE_ATTR_RO(current_state);
>>> +
>>> +static struct attribute *privacy_attributes[] = {
>>> +	&dev_attr_devices_supported.attr,
>>> +	&dev_attr_current_state.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +static const struct attribute_group privacy_attribute_group = {
>>> +	.attrs = privacy_attributes
>>> +};
>>> +
>>> +/*
>>> + * 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;
>>> +	}
>>> +	/* 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;
>> As discussed, there is no need for the privacy_valid variable, remove all
>> references to it here.
>>
>>> +
>>> +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;
>>> +
>>> +	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;
>>> +	}
>> No need for the goto here just simplify this to:
>>
>> 	if (!keymap)
>> 		return -ENOMEM;
>>
>>> +	/* 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);
>> You're done with the keymap here, please put a single:
>>
>> 	kfree(keymap);
>>
>> Here before the if (ret), then you won't need any goto style
>> cleanup anywhere in this function.
>>
>> This also fixes not free-ing the keymap when sparse_keymap_setup() fails.
>>
>>> +	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");
>> You're not setting ret here.
>>
>>> +		goto err_free_keymap;
>>> +	}
>> No need for the pr_debug, the kernel will already complain if this fails.
>> Also since all your resources are devm managed and we've already taken
>> care of the keymap this can be simplified to:
>>
>> 	ret = input_register_device(priv->input_dev);
>> 	if (ret)
>> 		returm ret;
>>
>>> +	mutex_lock(&list_mutex);
>>> +	list_add_tail(&priv->list, &wmi_list);
>>> +	mutex_unlock(&list_mutex);
>> Move this to directly above the return 0; below, see below.
>>
>>> +
>>> +	if (get_current_status(priv->wdev))
>>> +		goto err_free_input;
>> You're again not setting ret here before the goto.
>>
>> And you are returning with an error here after adding the device to the wmi_list,
>> the list_add_tail call should be done last, directly above the return 0 call.
>>
>> Also since all your resources are devm managed and we've already taken
>> care of the keymap this can be simplified to:
>>
>> 	ret = get_current_status(priv->wdev);
>> 	if (ret)
>> 		return ret;
>>
>>> +
>>> +	ret = devm_device_add_group(&wdev->dev, &privacy_attribute_group);
>>> +	if (ret)
>>> +		goto err_free_input;
>> Replace the goto err_free_input() with return ret;
>>
>>> +
>>> +	kfree(keymap);
>> Drop this kfree() (already done above now).
>>
>> Move the list_add_tail() here.
>>
>>> +	return 0;
>>> +
>>> +err_free_input:
>>> +	input_unregister_device(priv->input_dev);
>> This is not necessary for an input_device allocated with devm_input_allocate_device(),
>> which means that none of these error handling labels are necessary now, so this can
>> all be removed.
>>
>>
>>
>>> +err_free_keymap:
>>> +	privacy_valid = -ENODEV;
>>> +	kfree(keymap);
>>> +err_free_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);
>> The input_unregister is not necessary (and privacy_valid is gone too).
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
>>> +	{ .guid_string = DELL_PRIVACY_GUID },
>>> +	{ },
>>> +};
>>> +
>>> +static struct wmi_driver dell_privacy_wmi_driver = {
>>> +	.driver = {
>>> +		.name = "dell-privacy",
>>> +	},
>>> +	.probe = dell_privacy_wmi_probe,
>>> +	.remove = dell_privacy_wmi_remove,
>>> +	.id_table = dell_wmi_privacy_wmi_id_table,
>>> +};
>>> +
>>> +static int __init init_dell_privacy(void)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = wmi_has_guid(DELL_PRIVACY_GUID);> +	if (!ret) {
>>> +		privacy_valid = -ENODEV;
>>> +		pr_debug("Unable to detect available Dell privacy devices: %d\n", ret);
>>> +		return privacy_valid;
>>> +	}
>> This driver will only get modprobed if the GUID is present and when it is
>> not present then loading the driver is a no-op so this can be checked.
>>
>>> +
>>> +	ret = wmi_driver_register(&dell_privacy_wmi_driver);
>>> +	if (ret) {
>>> +		pr_err("failed to initialize privacy wmi driver: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = dell_privacy_acpi_init();
>>> +	if (ret) {
>>> +		pr_err("failed to initialize privacy acpi driver: %d\n", ret);
>>> +		goto err_init;
>>> +	}
>> And with the platform-device/driver removed this is not necessary too,
>> which means ...
>>
>>> +
>>> +	return 0;
>>> +
>>> +err_init:
>>> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
>>> +	return ret;
>>> +}
>>> +
>>> +static void dell_privacy_wmi_exit(void)
>>> +{
>>> +	wmi_driver_unregister(&dell_privacy_wmi_driver);
>>> +}
>>> +
>>> +static void __exit exit_dell_privacy(void)
>>> +{
>>> +	dell_privacy_wmi_exit();
>>> +	dell_privacy_acpi_exit();
>>> +}
>>> +
>>> +module_init(init_dell_privacy);
>>> +module_exit(exit_dell_privacy);
>> That all of the init_dell_privacy / exit_dell_privacy stuff can be replaced with
>> a single line:
>>
>> module_wmi_driver(&dell_privacy_wmi_driver);
>>
>>
>>
>>
>>
>>> +
>>> +MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
>>> +MODULE_AUTHOR("Perry Yuan <[e-mail]>");
>>> +MODULE_DESCRIPTION("Dell Privacy WMI Driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
>>> new file mode 100644
>>> index 000000000000..6dda2990df5d
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
>>> @@ -0,0 +1,35 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Dell privacy notification driver
>>> + *
>>> + * Copyright (C) 2021 Dell Inc. All Rights Reserved.
>>> + */
>>> +
>>> +#ifndef _DELL_PRIVACY_WMI_H_
>>> +#define _DELL_PRIVACY_WMI_H_
>>> +
>>> +#if IS_ENABLED(CONFIG_DELL_PRIVACY)
>>> +extern int privacy_valid;
>> Please remove this from the global kernel namespace
>> (make it a static global inside the dell-privacy-wmi.c code).
>>
>>> +int  dell_privacy_state(void);
>>> +void dell_privacy_process_event(int type, int code, int status);
>>> +#else /* CONFIG_DELL_PRIVACY */
>>> +int privacy_valid = -ENODEV;
>> Drop this.
>>
>>> +static inline int dell_privacy_state(void)
>>> +{
>>> +	return privacy_valid;
>> And directly return -ENODEV here.
>>
>>> +}
>>> +
>>> +static inline void dell_privacy_process_event(int type, int code, int status)
>>> +{}
>>> +#endif /* CONFIG_DELL_PRIVACY */
>>> +
>>> +int  dell_privacy_acpi_init(void);
>>> +void dell_privacy_acpi_exit(void);
>>> +
>>> +/* DELL Privacy Type */
>>> +enum {
>>> +	DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
>>> +	DELL_PRIVACY_TYPE_AUDIO,
>>> +	DELL_PRIVACY_TYPE_CAMERA,
>>> +};
>> This enum is only used inside dell-privacy-wmi.c, please move it there.
>>
>>> +#endif
>> With the platform-device/driver removal discussed above all this header file
>> needs to do is defines the prototypes / stubs for 2 functions:
>>
>> int  dell_privacy_state(void);
>> void dell_privacy_process_event(int type, int code, int status);
>>
>> I think it would be best to just add these to dell-smbios.h, rather then
>> adding a new header just for these 2 prototypes / stubs.
>>
>>
>>> diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
>>> index bbdb3e860892..389e86de24b0 100644
>>> --- a/drivers/platform/x86/dell/dell-wmi.c
>>> +++ b/drivers/platform/x86/dell/dell-wmi.c
>>> @@ -13,20 +13,22 @@
>>>   
>>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>   
>>> -#include <linux/kernel.h>
>>> -#include <linux/module.h>
>>> +#include <linux/acpi.h>
>>> +#include <linux/dmi.h>
>>>   #include <linux/init.h>
>>> -#include <linux/slab.h>
>>> -#include <linux/types.h>
>>>   #include <linux/input.h>
>>>   #include <linux/input/sparse-keymap.h>
>>> -#include <linux/acpi.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/slab.h>
>>>   #include <linux/string.h>
>>> -#include <linux/dmi.h>
>>> +#include <linux/types.h>
>>>   #include <linux/wmi.h>
>>>   #include <acpi/video.h>
>>> +
>> Please split the changes sorting these includes alphabetically out
>> into a separate cleanup patch.
>>
>>>   #include "dell-smbios.h"
>>>   #include "dell-wmi-descriptor.h"
>>> +#include "dell-privacy-wmi.h"
>>>   
>>>   MODULE_AUTHOR("Matthew Garrett <[e-mail]>");
>>>   MODULE_AUTHOR("Pali Rohár <[e-mail]>");
>>> @@ -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;
>> Drop this.
>>
>>>   
>>>   	if (obj->type != ACPI_TYPE_BUFFER) {
>>>   		pr_warn("bad response type %x\n", obj->type);
>>> @@ -427,7 +430,6 @@ 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]);
>>> @@ -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]);
>> This needs a len > 4 check !
>>
>>> +			} else {
>>> +				if (len > 2)
>>> +					dell_wmi_process_key(wdev, buffer_entry[1],
>>> +							     buffer_entry[2]);
>>> +			}
>> Combined with the change to have dell_privacy_process_event() return a bool, this should
>> look something like this:
>>
>> 			if (len > 4 && dell_privacy_process_event(buffer_entry[1], buffer_entry[3], buffer_entry[4]))
>> 				; /* Handled by dell_privacy_process_event() */
>> 			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]);
>>>
>> Regards,
>>
>> Hans
>>

Patch

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 000000000000..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:	"[e-mail]>"
+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:	"[e-mail]>"
+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/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index e0a55337f51a..0e0f1eb35bd6 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -204,4 +204,20 @@  config DELL_WMI_SYSMAN
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-wmi-sysman.
 
+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
+	  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.
 endif # X86_PLATFORM_DRIVERS_DELL
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index d720a3e42ae3..8b0883ee9412 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -19,3 +19,6 @@  obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.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
diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 70edc5bb3a14..4c840ab6cb59 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/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,6 +92,7 @@  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");
@@ -2206,10 +2209,16 @@  static int __init dell_init(void)
 
 	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;
+		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)
@@ -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 (!has_privacy)
+		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 (!has_privacy)
+		led_classdev_unregister(&micmute_led_cdev);
 	dell_cleanup_rfkill();
 	if (platform_device) {
 		platform_device_unregister(platform_device);
diff --git a/drivers/platform/x86/dell/dell-privacy-acpi.c b/drivers/platform/x86/dell/dell-privacy-acpi.c
new file mode 100644
index 000000000000..4c785b62c7c4
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-privacy-acpi.c
@@ -0,0 +1,164 @@ 
+// 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;
+	static char *acpi_method = (char *)"ECAK";
+
+	handle = ec_get_handle();
+	if (!handle)
+		return -EIO;
+	if (!acpi_has_method(handle, acpi_method))
+		return -EIO;
+	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;
+	}
+	pr_debug("set dell privacy micmute ec ack event done\n");
+	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)
+		return ret;
+	return 0;
+}
+
+static int dell_privacy_acpi_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	platform_set_drvdata(pdev, privacy_acpi);
+	privacy_acpi->dev = &pdev->dev;
+	privacy_acpi->platform_device = pdev;
+
+	ret = dell_privacy_leds_setup(&pdev->dev);
+	if (ret)
+		return -EIO;
+
+	return 0;
+}
+
+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;
+}
+
+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 <[e-mail]>");
+MODULE_DESCRIPTION("DELL Privacy ACPI Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.c b/drivers/platform/x86/dell/dell-privacy-wmi.c
new file mode 100644
index 000000000000..8bde97613477
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-privacy-wmi.c
@@ -0,0 +1,340 @@ 
+// 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)
+
+/*
+ * The wmi_list is used to store the privacy_priv struct with mutex protecting
+ */
+static LIST_HEAD(wmi_list);
+static DEFINE_MUTEX(list_mutex);
+/*
+ * global privacy state for other modules to query if the privacy driver loaded successfully
+ * or no privacy WMI device was detected at initial probe phase
+ */
+int privacy_valid = -EPROBE_DEFER;
+EXPORT_SYMBOL_GPL(privacy_valid);
+
+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_state(void)
+{
+	return privacy_valid;
+}
+EXPORT_SYMBOL_GPL(dell_privacy_state);
+/*
+ * The flow of privacy event:
+ * 1) User presses key. HW does stuff with this key (timeout is started)
+ * 2) WMI event is emitted from BIOS
+ * 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 defined by
+ *    dell-privacy-acpi driver.
+ *    codec driver will call like this to switch micmute led state.
+ *	ledtrig_audio_set(LED_AUDIO_MICMUTE, 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.
+ */
+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;
+	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);
+
+static ssize_t devices_supported_show(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%x\n", priv->features_present);
+}
+
+static ssize_t current_state_show(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct privacy_wmi_data *priv = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%x\n", priv->last_status);
+}
+
+static DEVICE_ATTR_RO(devices_supported);
+static DEVICE_ATTR_RO(current_state);
+
+static struct attribute *privacy_attributes[] = {
+	&dev_attr_devices_supported.attr,
+	&dev_attr_current_state.attr,
+	NULL,
+};
+
+static const struct attribute_group privacy_attribute_group = {
+	.attrs = privacy_attributes
+};
+
+/*
+ * 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;
+	}
+	/* 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;
+
+	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:
+	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;
+}
+
+static const struct wmi_device_id dell_wmi_privacy_wmi_id_table[] = {
+	{ .guid_string = DELL_PRIVACY_GUID },
+	{ },
+};
+
+static struct wmi_driver dell_privacy_wmi_driver = {
+	.driver = {
+		.name = "dell-privacy",
+	},
+	.probe = dell_privacy_wmi_probe,
+	.remove = dell_privacy_wmi_remove,
+	.id_table = dell_wmi_privacy_wmi_id_table,
+};
+
+static int __init init_dell_privacy(void)
+{
+	int ret;
+
+	ret = wmi_has_guid(DELL_PRIVACY_GUID);
+	if (!ret) {
+		privacy_valid = -ENODEV;
+		pr_debug("Unable to detect available Dell privacy devices: %d\n", ret);
+		return privacy_valid;
+	}
+
+	ret = wmi_driver_register(&dell_privacy_wmi_driver);
+	if (ret) {
+		pr_err("failed to initialize privacy wmi driver: %d\n", ret);
+		return ret;
+	}
+
+	ret = dell_privacy_acpi_init();
+	if (ret) {
+		pr_err("failed to initialize privacy acpi driver: %d\n", ret);
+		goto err_init;
+	}
+
+	return 0;
+
+err_init:
+	wmi_driver_unregister(&dell_privacy_wmi_driver);
+	return ret;
+}
+
+static void dell_privacy_wmi_exit(void)
+{
+	wmi_driver_unregister(&dell_privacy_wmi_driver);
+}
+
+static void __exit exit_dell_privacy(void)
+{
+	dell_privacy_wmi_exit();
+	dell_privacy_acpi_exit();
+}
+
+module_init(init_dell_privacy);
+module_exit(exit_dell_privacy);
+
+MODULE_DEVICE_TABLE(wmi, dell_wmi_privacy_wmi_id_table);
+MODULE_AUTHOR("Perry Yuan <[e-mail]>");
+MODULE_DESCRIPTION("Dell Privacy WMI Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell/dell-privacy-wmi.h b/drivers/platform/x86/dell/dell-privacy-wmi.h
new file mode 100644
index 000000000000..6dda2990df5d
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-privacy-wmi.h
@@ -0,0 +1,35 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Dell privacy notification driver
+ *
+ * Copyright (C) 2021 Dell Inc. All Rights Reserved.
+ */
+
+#ifndef _DELL_PRIVACY_WMI_H_
+#define _DELL_PRIVACY_WMI_H_
+
+#if IS_ENABLED(CONFIG_DELL_PRIVACY)
+extern int privacy_valid;
+int  dell_privacy_state(void);
+void dell_privacy_process_event(int type, int code, int status);
+#else /* CONFIG_DELL_PRIVACY */
+int privacy_valid = -ENODEV;
+static inline int dell_privacy_state(void)
+{
+	return privacy_valid;
+}
+
+static inline void dell_privacy_process_event(int type, int code, int status)
+{}
+#endif /* CONFIG_DELL_PRIVACY */
+
+int  dell_privacy_acpi_init(void);
+void dell_privacy_acpi_exit(void);
+
+/* DELL Privacy Type */
+enum {
+	DELL_PRIVACY_TYPE_UNKNOWN = 0x0,
+	DELL_PRIVACY_TYPE_AUDIO,
+	DELL_PRIVACY_TYPE_CAMERA,
+};
+#endif
diff --git a/drivers/platform/x86/dell/dell-wmi.c b/drivers/platform/x86/dell/dell-wmi.c
index bbdb3e860892..389e86de24b0 100644
--- a/drivers/platform/x86/dell/dell-wmi.c
+++ b/drivers/platform/x86/dell/dell-wmi.c
@@ -13,20 +13,22 @@ 
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/types.h>
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
-#include <linux/acpi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
 #include <linux/string.h>
-#include <linux/dmi.h>
+#include <linux/types.h>
 #include <linux/wmi.h>
 #include <acpi/video.h>
+
 #include "dell-smbios.h"
 #include "dell-wmi-descriptor.h"
+#include "dell-privacy-wmi.h"
 
 MODULE_AUTHOR("Matthew Garrett <[e-mail]>");
 MODULE_AUTHOR("Pali Rohár <[e-mail]>");
@@ -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,7 +430,6 @@  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]);
@@ -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]);