Am 28.09.22 um 12:47 schrieb Andy Shevchenko:
On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
The dell-wmi-ddv driver adds support for reading
the current temperature and ePPID of ACPI batteries
on supported Dell machines.
Since the WMI interface used by this driver does not
do any input validation and thus cannot be used for probing,
the driver depends on the ACPI battery extension machanism
to discover batteries.
The driver also supports a debugfs interface for retrieving
buffers containing fan and thermal sensor information.
Since the meaing of the content of those buffers is currently
unknown, the interface is meant for reverse-engineering and
will likely be replaced with an hwmon interface once the
meaning has been understood.
The driver was tested on a Dell Inspiron 3505.
...
+config DELL_WMI_DDV
+ tristate "Dell WMI sensors Support"
+ default m
Why? (Imagine I have Dell, but old machine)
(And yes, I see that other Kconfig options are using it, but we shall avoid
cargo cult and each default choice like this has to be explained at least.)
...
+ * dell-wmi-ddv.c -- Linux driver for WMI sensor information on Dell notebooks.
Please, remove file name from the file. This will be an additional burden in
the future in case it will be renamed.
...
+#include <acpi/battery.h>
Is it required to be the first? Otherwise it seems ACPI specific to me and the
general rule is to put inclusions from generic towards custom. I.o.w. can you
move it after linux/wmi.h with a blank line in between?
+#include <linux/acpi.h>
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/kstrtox.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/limits.h>
+#include <linux/power_supply.h>
+#include <linux/seq_file.h>
+#include <linux/sysfs.h>
+#include <linux/wmi.h>
...
+struct dell_wmi_ddv_data {
+ struct acpi_battery_hook hook;
+ struct device_attribute temp_attr, eppid_attr;
It's hard to read and easy to miss that the data type has two members here.
Please, put one member per one line.
+ struct wmi_device *wdev;
+};
...
+ if (obj->type != type) {
+ kfree(obj);
+ return -EIO;
EINVAL?
In my opinion, EINVAL should be returned if the parameters are invalid.
In this case however, the error comes from the wmi device returning invalid
data, which would be represented better with EIO.
+ }
...
+ kfree(obj);
I'm wondering what is the best to use in the drivers:
1) kfree()
2) acpi_os_free()
3) ACPI_FREE()
?
...
+static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
+{
+ const char *uid_str = acpi_device_uid(acpi_dev);
+
+ if (!uid_str)
+ return -ENODEV;
It will be better for maintaining to have
const char *uid_str...;
uid_str = ...
if (!uid_str)
...
+ return kstrtou32(uid_str, 10, index);
+}
...
+ /* Return 0 instead of error to avoid being unloaded */
+ ret = dell_wmi_ddv_battery_index(to_acpi_device(battery->dev.parent), &index);
+ if (ret < 0)
+ return 0;
How index is used?
...
+ ret = device_create_file(&battery->dev, &data->temp_attr);
+ if (ret < 0)
+ return ret;
+
+ ret = device_create_file(&battery->dev, &data->eppid_attr);
+ if (ret < 0) {
+ device_remove_file(&battery->dev, &data->temp_attr);
+
+ return ret;
+ }
Why dev_groups member can't be utilized?
...
+static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
Strictly speaking this should return int (see below).
+{
+ struct dentry *entry;
+ char name[64];
+
+ scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
+ entry = debugfs_create_dir(name, NULL);
+
+ debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
+ dell_wmi_ddv_fan_read);
+ debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
+ dell_wmi_ddv_temp_read);
+
+ devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
return devm...
This is not related to debugfs and there is no rule to avoid checking error
codes from devm_add_action_or_reset().
According to the documentation of debugfs_create_dir(), drivers should work fine if debugfs
initialization fails. Thus the the return value of dell_wmi_ddv_debugfs_init() would be ignored
when called, which means that returning an error would serve no purpose.
Additionally, devm_add_action_or_reset() automatically executes the cleanup function if devres
registration fails, so we do not have to care about that.
+}
...
+static struct wmi_driver dell_wmi_ddv_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
I would use explicit literal since this is a (semi-) ABI, and having it as
a define feels not fully right.
The driver name is used in two places (init and debugfs), so having a define for it
avoids problems in case someone forgets to change both.
Armin Wolf
+ },
+ .id_table = dell_wmi_ddv_id_table,
+ .probe = dell_wmi_ddv_probe,
+};
