Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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?

> +	}

...

> +	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().

> +}

...

> +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,
> +};

-- 
With Best Regards,
Andy Shevchenko