Hi
2021. január 11., hétfő 14:42 keltezéssel, Perry Yuan írta:
[...]My comment was referring to the fact that you could've used
I need to set one static struct to let some function to get the priv pointer.+#define PRIVACY_PLATFORM_NAME "dell-privacy-acpi"Any reason it needs to be dynamically allocated?
+#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;
It is more simple if i add some function and get the priv easily.
If you have better suggestion, i would be glad to optimize it.
for example.
static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
struct privacy_acpi_priv *priv = privacy_acpi;
..........................
}
```
static struct privacy_acpi_priv privacy_acpi;
```
to achieve the same result without the dynamic memory allocation.
You could keep the method name in a static variable:Just want to make sure the BIOS has the correct method to call.
+I find this if-else a bit cumbersome. Any reason why
+static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct privacy_acpi_priv *priv = privacy_acpi;
+ acpi_status status;
+ acpi_handle handle;
+ char *acpi_method;
+
+ handle = ec_get_handle();
+ if (!handle)
+ return -EIO;
+ if (acpi_has_method(handle, "ECAK"))
+ acpi_method = "ECAK";
+ else
+ return -ENODEV;
if (!acpi_has_method(handle, "ECAK"))
return ...;
would not work? I believe you could also easily do away with the `acpi_method`
variable.
normally it will not be failed, just keep it safe to call BIOS, not to cause any panic.
I changed it as below .
handle = ec_get_handle();
if (!handle)
return -EIO;
acpi_method = "ECAK";
```
static char *acpi_method = (char *)"ECAK"; // this is inside the function
handle = ...;
if (!handle)
return ...
if (!acpi_has_method(handle, acpi_method))
return ...
... acpi_evaluate_object(handle, acpi_method, ...
```
Another thing is that I believe you could do these checks only once,
before registering the LED device.
I see. I'm wondering if the ACPI match table is needed at the moment. If I'm not[...]
+
+ status = acpi_evaluate_object(handle, acpi_method, NULL, NULL);
+ if (ACPI_FAILURE(status)) {
+ dev_err(priv->dev, "Error setting privacy EC ack value: %s\n",
+ acpi_format_exception(status));
+ return -EIO;
+ }
+ return 0;
+}
+
there will be some more privacy devices need to add some acpi interface function here.+static const struct acpi_device_id privacy_acpi_device_ids[] = {I think using a platform driver here just complicates things for no reason.
+ {"PNP0C09", 0},
+ { },
+};
+MODULE_DEVICE_TABLE(acpi, privacy_acpi_device_ids);
+
+static struct platform_driver dell_privacy_platform_drv = {
+ .driver = {
+ .name = PRIVACY_PLATFORM_NAME,
+ .acpi_match_table = ACPI_PTR(privacy_acpi_device_ids),
+ },
+ .remove = dell_privacy_acpi_remove,
+};
Furthermore, I'm not sure if there's actually any need for the ACPI match table.
including elctronic privacy screen and privacy camera.
the platform driver can provide more flexible framework to extend.
mistaken the platform driver already binds the platform device the module creates.
And there is no real need to bind to the ACPI EC devices.
[...]My problem is that those three states are never used.
[...]diff --git a/drivers/platform/x86/dell-privacy-wmi.c b/drivers/platform/x86/dell-privacy-wmi.c
new file mode 100644
index 000000000000..80637c7f617c
--- /dev/null
+++ b/drivers/platform/x86/dell-privacy-wmi.c
1. when the DELL_PRIVACY_GUID not found, it will return ENODEV showing no privacy devices.+int dell_privacy_valid(void)I find this function really confusing, and too verbose for what it does.
+{
+ int ret;
+
+ ret = wmi_has_guid(DELL_PRIVACY_GUID);
+ if (!ret)
+ return -ENODEV;
+ ret = privacy_valid;
+ return ret;
2. when DELL_PRIVACY_GUID found,and wmi privacy driver is registered, it will return "0"
3.when DELL_PRIVACY_GUID found,and wmi privacy driver is NOT registered yet, it will return "EPROBE_DEFE"
the EPROBE_DEFER is defined in "include/linux/errno.h"
#define EPROBE_DEFER 517 /* Driver requests probe retry */
when caller get this returned , it will defer the caller 1s ~ 7s to probe again.
This will make sure dell-laptop can get the correct privacy status to register its micmute led trigger driver or not.
----dell-laptoo.c-------------------
#if IS_ENABLED(CONFIG_DELL_PRIVACY)
ret = dell_privacy_valid();
if (!ret)
privacy_valid = true;
#endif
if (!privacy_valid) {
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;
}
}
The only thing that's ever checked is if the return value of dell_privacy_valid()
is zero or not. No part of the code distinguishes between -ENODEV and -EPROBE_DEFER.
(As far as I see, I may be missing something.)
My initial point was that - if I'm not missing anything significant - the whole
function could just be:
```
int dell_privacy_valid(void)
{
return privacy_valid;
}
```
given that you manipulate the value of `privacy_valid` appropriately. And assuming
the current form is needed, this would be enough:
```
int dell_privacy_valid(void)
{
if (!wmi_has_guid(...))
return -ENODEV;
return privacy_valid;
}
```
But I'd personally prefer the first one.
I'm not sure I fully get it.mutex_lock(&list_mutex);+}Can you please explain why this list is needed if only the first entry will
+EXPORT_SYMBOL_GPL(dell_privacy_valid);
+
+void dell_privacy_process_event(int type, int code, int status)
+{
+ struct privacy_wmi_data *priv;
+ const struct key_entry *key;
+
+ mutex_lock(&list_mutex);
+ priv = list_first_entry_or_null(&wmi_list,
+ struct privacy_wmi_data,
+ list);
ever be used?
list_add_tail(&priv->list, &wmi_list);
mutex_unlock(&list_mutex);
only one priv struct is added to the list with list mutex protecting.
So it will not have two more entry data added to the list .
My point here is that the two cases of the switch do the exact same thing. TheIt is needed here, camra mute and privacy screen will add more codes here.
+ if (!priv) {Is this switch needed at all?
+ 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);
+ }
It want to keep the switch for further extention.
whole switch could be replaced with:
```
sparse_keymap_report_entry(priv->input_dev, key, 1, true);
```
I would go as far as use this:
```
if (!sparse_keymap_report_event(priv->input_dev, (type << 16) | code, 1, true))
dev_dbg(&priv->wdev->dev, "unknown event type=0x%04x code=0x%04x\n", type, code)
```
This would elliminate the need for the `key` variable, the call to
`sparse_keymap_entry_from_scancode()`, and the whole switch.
[...]You can use `i` as the index, so no need for `pos`.
There is actually no need for the `pos` variable.It is used in this keymap codes.
for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
keymap[pos] = dell_wmi_keymap_type_0012[i];
keymap[pos].code |= (0x0012 << 16);
pos++;
}
My point is that if you had:fixed.
+Please use `sizeof(*priv)`.
+ priv = devm_kzalloc(&wdev->dev, sizeof(struct privacy_wmi_data),
I don`t quite undestand what you meant
+ GFP_KERNEL);I don't think that `+ 1` is not needed since the KE_END entry is already in the array.
+ if (!priv)
+ return -ENOMEM;
+
+ dev_set_drvdata(&wdev->dev, priv);
+ priv->wdev = wdev;
+ /* create evdev passing interface */
+ priv->input_dev = devm_input_allocate_device(&wdev->dev);
+ if (!priv->input_dev)
+ return -ENOMEM;
+ /* remap the wmi keymap event to new keymap */
+ keymap = kcalloc(ARRAY_SIZE(dell_wmi_keymap_type_0012) +
+ 1,
+ sizeof(struct key_entry), GFP_KERNEL);A copy of the keymap is created by `sparse_keymap_setup()`, so returning
+ if (!keymap) {
+ ret = -ENOMEM;
+ goto err_free_dev;
+ }
+ /* remap the keymap code with Dell privacy key type 0x12 as prefix
+ * KEY_MICMUTE scancode will be reported as 0x120001
+ */
+ for (i = 0; i < ARRAY_SIZE(dell_wmi_keymap_type_0012); i++) {
+ keymap[pos] = dell_wmi_keymap_type_0012[i];
+ keymap[pos].code |= (0x0012 << 16);
+ pos++;
+ }
+ ret = sparse_keymap_setup(priv->input_dev, keymap, NULL);
+ if (ret)
+ return ret;
here will leak `keymap`. You could just call `kfree(keymap)` directly after
the `sparse_keymap_setup()` call. But I find it completely unnecessary
to do this allocate-copy-modify thing. Is there any reason why the static array
(`dell_wmi_keymap_type_0012`) cannot already contain the correct values?
what "correct values" should be contained by the dell_wmi_keymap_type_0012?
```
static const struct key_entry dell_wmi_keymap_type_0012[] = {
{ KE_KEY, 0x00120001, { KEY_MICMUTE } },
{ KE_SW, 0x00120002, { SW_CAMERA_LENS_COVER } },
{ KE_END, 0},
};
```
then there would be no need to create a copy and the code would be simpler.
[...]My point is that if dell-privacy is a dependency of dell-laptop, and dell-privacy
Yes, the wmi driver is the entry for whole privacy driver.+static int __init init_dell_privacy(void)The init function of a module that exports symbols must not fail, otherwise
+{
+ int ret;
+
+ ret = wmi_has_guid(DELL_PRIVACY_GUID);
+ if (!ret)
+ return -ENODEV;
it'll prevent the loading of dependent modules.
if wmi guid query failed, the privacy driver will not be registered.
dell-laptop driver will get "-ENOVDE" from dell_privacy_valid().
it will register its micmute led trigger driver from dell laptop driver.
fails to load, then dell-laptop cannot be loaded. Effectively, the lack of
the DELL_PRIVACY_GUID WMI interface will cause the dell-laptop module not be
able to be loaded.
[...]I'm wondering if there are such guarantees, why is the length checked just a
when CONFIG_DELL_PRIVACY is enabled, and dell_privacy_valid return zero which means privacy driver loaded as expected.+#if IS_ENABLED(CONFIG_DELL_PRIVACY)What if `len < 5`?
+ err = dell_privacy_valid();
+ if (err == 0) {
+ dell_privacy_process_event(buffer_entry[1],
+ buffer_entry[3], buffer_entry[4]);
for example ,the micmute report wmi event data len is "5", it will not be less than 5 words.
couple lines below?
Process buffer (04 00 12 00 0e 00 01 00 03 00)One thing I might have forgotten to point out initially, is that there is no need
#if IS_ENABLED(CONFIG_DELL_PRIVACY)
for this #if as dell-privacy-wmi.h provides stub definitions for
`dell_privacy_valid()` and `dell_privacy_process_event()`.
err = dell_privacy_valid();Another thing I may have forgotten to say: the name `dell_privacy_valid()` is
if (err == 0) {
dell_privacy_process_event(buffer_entry[1],
buffer_entry[3], buffer_entry[4]);
} else {
if (len > 2)
dell_wmi_process_key(wdev, buffer_entry[1],
buffer_entry[2]);
/* Extended data is currently ignored */
}
[...]
misleading in my opinion, as it gives the impression of being a predicate, even
though it is not. `dell_privacy_state()` or something like that would be better,
I think.
Regards,
Barnabás Pőcze
p.s. please send text emails.