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