Re: [PATCH v10 11/14] platform/x86: Split the alienware-wmi driver

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

On Fri, Feb 07, 2025 at 10:46:07AM -0500, Kurt Borja wrote:
> Split alienware-wmi WMI drivers into different files. This is done
> seamlessly by copying and pasting, however some blocks are reordered.

...

>  obj-$(CONFIG_ALIENWARE_WMI)		+= alienware-wmi.o
>  alienware-wmi-objs			:= alienware-wmi-base.o
> +alienware-wmi-y				+= alienware-wmi-legacy.o
> +alienware-wmi-y				+= alienware-wmi-wmax.o

Oh my... it's even inconsistent!

...

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/wmi.h>

Too little headers are being included. At very quick glance a dozen
or so is missing...

> +#include "alienware-wmi.h"

...

> +static struct attribute *hdmi_attrs[] = {
> +	&dev_attr_cable.attr,
> +	&dev_attr_source.attr,
> +	NULL,

No comma in the terminator entry.

> +};

...

> +static struct attribute *amplifier_attrs[] = {
> +	&dev_attr_status.attr,
> +	NULL,
> +};

Ditto.

...

> +{
> +	struct alienfx_platdata *pdata = dev_get_platdata(dev);
> +	struct wmax_basic_args in_args = {
> +		.arg = 0,
> +	};
> +	u32 out_data;
> +	int ret;
> +
> +	ret = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_STATUS,
> +				    &in_args, sizeof(in_args), &out_data);
> +	if (!ret) {
> +		if (out_data == 0)
> +			return sysfs_emit(buf, "[disabled] s5 s5_s4\n");
> +		else if (out_data == 1)
> +			return sysfs_emit(buf, "disabled [s5] s5_s4\n");
> +		else if (out_data == 2)
> +			return sysfs_emit(buf, "disabled s5 [s5_s4]\n");

The whole code inherited same issues like redundant 'else'. Please, refactor.

> +	}
> +
> +	pr_err("alienware-wmi: unknown deep sleep status: %d\n", ret);
> +	return sysfs_emit(buf, "disabled s5 s5_s4 [unknown]\n");
> +}

...

> +static ssize_t deepsleep_store(struct device *dev, struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct alienfx_platdata *pdata = dev_get_platdata(dev);
> +	struct wmax_basic_args args;
> +	int ret;
> +
> +	if (strcmp(buf, "disabled\n") == 0)
> +		args.arg = 0;
> +	else if (strcmp(buf, "s5\n") == 0)
> +		args.arg = 1;
> +	else
> +		args.arg = 2;

sysfs_match_string()

> +	pr_debug("alienware-wmi: setting deep sleep to %d : %s", args.arg, buf);
> +
> +	ret = alienware_wmi_command(pdata->wdev, WMAX_METHOD_DEEP_SLEEP_CONTROL,
> +				    &args, sizeof(args), NULL);
> +	if (!ret)
> +		pr_err("alienware-wmi: deep sleep control failed: results: %u\n", ret);
> +
> +	return count;
> +}

> +

Redundant blank line.

> +static DEVICE_ATTR_RW(deepsleep);

...

> +static struct attribute *deepsleep_attrs[] = {
> +	&dev_attr_deepsleep.attr,
> +	NULL,

No comma.

> +};

...

> +	if ((code & WMAX_THERMAL_TABLE_MASK) == WMAX_THERMAL_TABLE_USTT &&
> +	    (code & WMAX_THERMAL_MODE_MASK) <= THERMAL_MODE_USTT_LOW_POWER)
> +		return true;
> +
> +	return false;

	return ...

but if you wish, this one is okay.

...

> +static int thermal_profile_probe(void *drvdata, unsigned long *choices)
> +{
> +	enum platform_profile_option profile;
> +	struct awcc_priv *priv = drvdata;
> +	enum wmax_thermal_mode mode;
> +	u8 sys_desc[4];
> +	u32 first_mode;
> +	u32 out_data;
> +	int ret;
> +
> +	ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_SYS_DESCRIPTION,
> +				       0, (u32 *) &sys_desc);

How do you guarantee an alignment? Yes, it might be good for the specific
hardware, but in general this is broken code.

> +	if (ret < 0)
> +		return ret;
> +
> +	first_mode = sys_desc[0] + sys_desc[1];

> +	for (u32 i = 0; i < sys_desc[3]; i++) {

Why u32? unsigned int looks more natural here.

> +		ret = wmax_thermal_information(priv->wdev, WMAX_OPERATION_LIST_IDS,
> +					       i + first_mode, &out_data);
> +
> +		if (ret == -EIO)
> +			return ret;
> +
> +		if (ret == -EBADRQC)
> +			break;
> +
> +		if (!is_wmax_thermal_code(out_data))
> +			continue;
> +
> +		mode = out_data & WMAX_THERMAL_MODE_MASK;
> +		profile = wmax_mode_to_platform_profile[mode];
> +		priv->supported_thermal_profiles[profile] = out_data;

> +		set_bit(profile, choices);

Do you need it to be atomic?

> +	}
> +
> +	if (bitmap_empty(choices, PLATFORM_PROFILE_LAST))
> +		return -ENODEV;
> +
> +	if (awcc->gmode) {
> +		priv->supported_thermal_profiles[PLATFORM_PROFILE_PERFORMANCE] =
> +			WMAX_THERMAL_MODE_GMODE;
> +
> +		set_bit(PLATFORM_PROFILE_PERFORMANCE, choices);
> +	}
> +
> +	return 0;
> +}

...

> +static const struct wmi_device_id alienware_wmax_device_id_table[] = {
> +	{ WMAX_CONTROL_GUID, NULL },
> +	{ },

No comma.

> +};

...

> +int __init alienware_wmax_wmi_init(void)
> +{
> +	const struct dmi_system_id *id;
> +
> +	id = dmi_first_match(awcc_dmi_table);
> +	if (id)
> +		awcc = id->driver_data;
> +
> +	if (force_platform_profile) {
> +		if (!awcc)
> +			awcc = &empty_quirks;
> +
> +		awcc->pprof = true;
> +	}
> +
> +	if (force_gmode) {
> +		if (awcc)
> +			awcc->gmode = true;
> +		else
> +			pr_warn("force_gmode requires platform profile support\n");
> +	}
> +
> +	return wmi_driver_register(&alienware_wmax_wmi_driver);
> +}
> +
> +void __exit alienware_wmax_wmi_exit(void)
> +{
> +	wmi_driver_unregister(&alienware_wmax_wmi_driver);
> +}

Why not moving these boilerplate to ->probe() and use module_wmi_driver()?

-- 
With Best Regards,
Andy Shevchenko