Hi,
On 9/28/22 12:47, Andy Shevchenko wrote:
> 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)
Then you can select N if you really want to.
> (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.)
This has been discussed during the review of v1 already.
There are quite a few dell modules and the choice has
been made to put these all behind a dell platform drivers
options and then default all the individual modules to 'm'.
> ...
>
>> + * 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?
>
>> + }
>
> ...
>
>> + kfree(obj);
>
> I'm wondering what is the best to use in the drivers:
> 1) kfree()
> 2) acpi_os_free()
> 3) ACPI_FREE()
>
> ?
Most ACPI driver code I know of just uses kfree() the other 2
are more ACPI-core / ACPICA internal helpers.
>
> ...
>
>> +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?
index is used in the registered sysfs attr show functions,
so if this fails then the sysfs attr should not be registered.
>
> ...
>
>> + 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?
Because this is an extension to the ACPI battery driver, IOW
this adds extra attributes to the power-supply-class device
registered by the ACPI battery driver. Note that the device
in this case is managed by the power-supply-class code, so
there is no access to dev_groups even in the ACPI battery code.
>
> ...
>
>> +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().
>
>> +}
>
> ...
>
>> +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.
>
>> + },
>> + .id_table = dell_wmi_ddv_id_table,
>> + .probe = dell_wmi_ddv_probe,
>> +};
>
Regards,
Hans
