Messages in this thread | | | Subject | Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver | From | Armin Wolf <> | Date | Wed, 28 Sep 2022 22:57:16 +0200 |
| |
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, >> +};
| |