Re: [PATCH] Add OneXPlayer mini AMD board driver

[Guenter Roeck <linux@roeck-us.net>]

On 10/29/22 15:50, Joaquín Ignacio Aramendía wrote:
> Platform driver for OXP Handhelds that expose fan reading and control
> via hwmon sysfs.
> 
> As far as I could gather all OXP boards have the same DMI strings and
> they are told appart by the boot cpu vendor (Intel/AMD).
> Currently only AMD boards are supported but the code is made to be simple
> to add other handheld boards in the future.
> 
> Fan control is provided via pwm interface in the range [0-255]. AMD
> boards have [0-100] as range in the EC, the written value is scaled to
> accomodate for that.
> 
> PWM control is disabled by default, can be enabled via module parameter
> `fan_control=1`.
> ---
>   drivers/platform/x86/Kconfig        |   8 +
>   drivers/platform/x86/Makefile       |   3 +
>   drivers/platform/x86/oxp-platform.c | 393 ++++++++++++++++++++++++++++
>   3 files changed, 404 insertions(+)
>   create mode 100644 drivers/platform/x86/oxp-platform.c
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index f5312f51de19..8fe3ca1dd808 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -738,6 +738,14 @@ config XO1_RFKILL
>   	  Support for enabling/disabling the WLAN interface on the OLPC XO-1
>   	  laptop.
> 
> +config OXP_DEVICE
> +	tristate "OneXPlayer EC fan control"
> +	depends on ACPI
> +	depends on HWMON
> +	help
> +		Support for OneXPlayer device board EC fan control. Only AMD boards
> +		are supported.
> +
>   config PCENGINES_APU2
>   	tristate "PC Engines APUv2/3 front button and LEDs driver"
>   	depends on INPUT && INPUT_KEYBOARD && GPIOLIB
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 5a428caa654a..fa6f5c68ec45 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -80,6 +80,9 @@ obj-$(CONFIG_MSI_WMI)		+= msi-wmi.o
>   obj-$(CONFIG_XO15_EBOOK)	+= xo15-ebook.o
>   obj-$(CONFIG_XO1_RFKILL)	+= xo1-rfkill.o
> 
> +# OneXPlayer
> +obj-$(CONFIG_OXP_DEVICE) += oxp-platform.o
> +
>   # PC Engines
>   obj-$(CONFIG_PCENGINES_APU2)	+= pcengines-apuv2.o
> 
> diff --git a/drivers/platform/x86/oxp-platform.c b/drivers/platform/x86/oxp-platform.c
> new file mode 100644
> index 000000000000..4fb13e7450ff
> --- /dev/null
> +++ b/drivers/platform/x86/oxp-platform.c
> @@ -0,0 +1,393 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Platform driver for OXP Handhelds that expose fan reading and control
> + * via hwmon sysfs.
> + *
> + * All boards have the same DMI strings and they are told appart by the
> + * boot cpu vendor (Intel/AMD). Currently only AMD boards are supported
> + * but the code is made to be simple to add other handheld boards in the
> + * future.
> + * Fan control is provided via pwm interface in the range [0-255]. AMD
> + * boards use [0-100] as range in the EC, the written value is scaled to
> + * accomodate for that.
> + *
> + * PWM control is disabled by default, can be enabled via module parameter.
> + *
> + * Copyright (C) 2022 Joaquín I. Aramendía <samsagax@gmail.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/dev_printk.h>
> +#include <linux/dmi.h>
> +#include <linux/hwmon.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <asm/processor.h>
> +
> +static bool fan_control;
> +module_param_hw(fan_control, bool, other, 0644);

Runtime writeable parameter is unacceptable. Why would this be needed anyway ?
What is it supposed to accomplish that can not be done with a sysfs attribute ?

> +MODULE_PARM_DESC(fan_control, "Enable fan control");
> +
> +#define ACPI_LOCK_DELAY_MS	500
> +
> +/* Handle ACPI lock mechanism */
> +struct lock_data {
> +	u32 mutex;
> +	bool (*lock)(struct lock_data *data);
> +	bool (*unlock)(struct lock_data *data);
> +};
> +
> +static bool lock_global_acpi_lock(struct lock_data *data)
> +{
> +	return ACPI_SUCCESS(acpi_acquire_global_lock(ACPI_LOCK_DELAY_MS,
> +								 &data->mutex));
> +}
> +
> +static bool unlock_global_acpi_lock(struct lock_data *data)
> +{
> +	return ACPI_SUCCESS(acpi_release_global_lock(data->mutex));
> +}
> +
> +#define MAX_IDENTICAL_BOARD_VARIATIONS	2
> +
> +enum board_family {
> +	family_unknown,
> +	family_mini_amd,
> +};
> +
> +enum oxp_sensor_type {
> +	oxp_sensor_fan = 0,
> +	oxp_sensor_pwm,
> +	oxp_sensor_number,
> +};
> +
> +struct oxp_ec_sensor_addr {
> +	enum hwmon_sensor_types type;
> +	u8 reg;
> +	short size;
> +	union {
> +		struct {
> +			u8 enable;
> +			u8 val_enable;
> +			u8 val_disable;
> +		};
> +		struct {
> +		  int max_speed;
> +		};
> +	};
> +};
> +
> +

Extra empty line.

> +/* AMD board EC addresses */
> +static const struct oxp_ec_sensor_addr amd_sensors[] = {
> +	[oxp_sensor_fan] = {
> +		.type = hwmon_fan,
> +		.reg = 0x76,
> +		.size = 2,
> +		.max_speed = 5000,
> +	},
> +	[oxp_sensor_pwm] = {
> +		.type = hwmon_pwm,
> +		.reg = 0x4B,
> +		.size = 1,
> +		.enable = 0x4A,
> +		.val_enable = 0x01,
> +		.val_disable = 0x00,
> +	},

I don't see the point of this data structure. There is just one
entry. Why not use defines ?

> +	{},
> +};
> +
> +struct ec_board_info {
> +	const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS];
> +	enum board_family family;
> +	const struct oxp_ec_sensor_addr *sensors;
> +};
> +
> +static const struct ec_board_info board_info[] = {
> +	{
> +		.board_names = {"ONE XPLAYER", "ONEXPLAYER mini A07"},
> +		.family = family_mini_amd,
> +		.sensors = amd_sensors,
> +	},

There is just one family. What is the point of this data structure ?

> +	{}
> +};
> +
> +struct oxp_status {
> +	struct ec_board_info board;
> +	struct lock_data lock_data;
> +};
> +
> +/* Helper functions */
> +static int read_from_ec(u8 reg, int size, long *val)
> +{
> +	int i;
> +	int ret;
> +	u8 buffer;
> +
> +	*val = 0;
> +	for (i = 0; i < size; i++) {
> +		ret = ec_read(reg + i, &buffer);
> +		if (ret)
> +			return ret;
> +		(*val) <<= i*8;

space between i and 8

> +		*val += buffer;
> +	}
> +	return ret;

	return 0;

> +}
> +
> +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> +{
> +	struct oxp_status *state = dev_get_drvdata(dev);
> +	int ret = -1;

unnecessary (and bad) variable initialization

> +
> +	if (!state->lock_data.lock(&state->lock_data)) {
> +		dev_warn(dev, "Failed to acquire mutex");
> +		return -EBUSY;
> +	}
> +
> +	ret = ec_write(reg, value);
> +
> +	if (!state->lock_data.unlock(&state->lock_data))
> +		dev_err(dev, "Failed to release mutex");
> +
> +	return ret;
> +}
> +
> +/* Callbacks for hwmon interface */
> +static umode_t oxp_ec_hwmon_is_visible(const void *drvdata,
> +					enum hwmon_sensor_types type, u32 attr, int channel)
> +{
> +	switch (type) {
> +		case hwmon_fan:
> +			return S_IRUGO;
> +		case hwmon_pwm:
> +			return S_IRUGO | S_IWUSR;

Please use 0444 and 0644 directly. Checkpatch will tell.

> +		default:
> +			return 0;
> +	}
> +	return 0;
> +}
> +
> +static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> +		u32 attr, int channel, long *val)

Align continuation line with '('. Checkpatch will tell.

> +{
> +	int ret = -1;
> +	const struct oxp_status *state = dev_get_drvdata(dev);
> +	const struct ec_board_info *board = &state->board;
> +
> +	switch(type) {
> +		case hwmon_fan:
> +			switch(attr) {
> +				case hwmon_fan_input:
> +					ret = read_from_ec(board->sensors[oxp_sensor_fan].reg,
> +							board->sensors[oxp_sensor_fan].size, val);
> +					break;
> +				case hwmon_fan_max:
> +					ret = 0;
> +					*val = board->sensors[oxp_sensor_fan].max_speed;
> +					break;
> +				case hwmon_fan_min:
> +					ret = 0;
> +					*val = 0;
> +					break;

If fan_max and fan_min are not sent to/from the EC, do not provide the attributes.

> +				default:
> +					dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);

missing break;

> +			}
> +			return ret;
> +		case hwmon_pwm:
> +			switch(attr) {
> +				case hwmon_pwm_input:
> +					ret = read_from_ec(board->sensors[oxp_sensor_pwm].reg,
> +							board->sensors[oxp_sensor_pwm].size, val);
> +					if (board->family == family_mini_amd) {
> +						*val = (*val * 255) / 100;
> +					}
> +					break;
> +				case hwmon_pwm_enable:
> +					ret = read_from_ec(board->sensors[oxp_sensor_pwm].enable, 1, val);
> +					if (*val == board->sensors[oxp_sensor_pwm].val_enable) {
> +						*val = 1;
> +					} else {
> +						*val = 0;
> +					}

Unnecessary { }. Checkpatch would tell.

I don't see the point of this code. Why have board->sensors[oxp_sensor_pwm].val_enable
to start with ? It is 1. Can the EC return something else ? If so, what is the
rationale to map it to 0 ?

> +					break;
> +				default:
> +					dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);

missing break;

> +			}
> +			return ret;
> +		default:
> +			dev_dbg(dev, "Unknown sensor type %d.\n", type);
> +			return -1;

bad error code.

> +	}
> +}
> +
> +static int oxp_pwm_enable(const struct device *dev)
> +{
> +	int ret = -1;

Unnecessary (and bad) variable initialization.

> +	struct oxp_status *state = dev_get_drvdata(dev);
> +	const struct ec_board_info *board = &state->board;
> +
> +	if (!fan_control) {
> +		dev_info(dev, "Can't enable pwm, fan_control=0");
> +		return -EPERM;
> +	}
> +
> +	ret = write_to_ec(dev, board->sensors[oxp_sensor_pwm].enable,
> +		board->sensors[oxp_sensor_pwm].val_enable);
> +
> +	return ret;

... and unnecessary variable.

> +}
> +
> +static int oxp_pwm_disable(const struct device *dev)
> +{
> +	int ret = -1;

Unnecessary initialization, and bad error code.

> +	struct oxp_status *state = dev_get_drvdata(dev);
> +	const struct ec_board_info *board = &state->board;
> +
> +	if (!fan_control) {
> +		dev_info(dev, "Can't disable pwm, fan_control=0");
> +		return -EPERM;
> +	}

I really don't see the point of the "fan_control" module parameter.
One can set it to 1 (or true) to enable the pwm_enable attribute,
or set it to 0 to disable it. It is effectively the same as just
another attribute, forcing users to write two attributes instead
of one. That really doesn't make sense.

> +
> +	ret = write_to_ec(dev, board->sensors[oxp_sensor_pwm].enable,
> +		board->sensors[oxp_sensor_pwm].val_disable);
> +
> +	return ret;

Just
	return write_to_ec(...);

would do. There is no need for the ret variable. Same elsewhere.

> +}
> +
> +static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
> +		u32 attr, int channel, long val)
> +{
> +	int ret = -1;

bad error code.

> +	struct oxp_status *state = dev_get_drvdata(dev);
> +	const struct ec_board_info *board = &state->board;
> +
> +	switch(type) {
> +		case hwmon_pwm:
> +			if (!fan_control) {
> +				dev_info(dev, "Can't control fans, fan_control=0");
> +				return -EPERM;
> +			}
> +			switch(attr) {
> +				case hwmon_pwm_enable:
> +					if (val == 1) {
> +						ret = oxp_pwm_enable(dev);
> +					} else if (val == 0) {
> +						ret = oxp_pwm_disable(dev);
> +					} else {
> +						return -EINVAL;
> +					}

Unnecessary { }, and the single return on error instead of
						ret = -EINVAL;
is inconsistent.

> +					return ret;
> +				case hwmon_pwm_input:
> +					if (val < 0 || val > 255)
> +						return -EINVAL;
> +					if (board->family == family_mini_amd)
> +						val = (val * 100) / 255;
> +					ret = write_to_ec(dev, board->sensors[oxp_sensor_pwm].reg, val);
> +					return ret;
> +				default:
> +					dev_dbg(dev, "Unknown attribute for type %d: %d", type, attr);
> +					return ret;
> +			}
> +		default:
> +			dev_dbg(dev, "Unknown sensor type: %d", type);

break missing

> +	}
> +	return ret;
> +}
> +
> +/* Known sensors in the OXP EC controllers */
> +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> +	HWMON_CHANNEL_INFO(fan,
> +		HWMON_F_INPUT | HWMON_F_MAX | HWMON_F_MIN),
> +	HWMON_CHANNEL_INFO(pwm,
> +		HWMON_PWM_INPUT | HWMON_PWM_ENABLE),

bad alignment. Please use checkpatch --strict and fix what it reports.

> +	NULL
> +};
> +
> +static const struct hwmon_ops oxp_ec_hwmon_ops = {
> +	.is_visible = oxp_ec_hwmon_is_visible,
> +	.read = oxp_platform_read,
> +	.write = oxp_platform_write,
> +};
> +
> +static const struct hwmon_chip_info oxp_ec_chip_info = {
> +	.ops = &oxp_ec_hwmon_ops,
> +	.info = oxp_platform_sensors,
> +};
> +
> +/* Initialization logic */
> +static const struct ec_board_info * __init get_board_info(struct device *dev)
> +{
> +	const char *dmi_board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR);
> +	const char *dmi_board_name = dmi_get_system_info(DMI_BOARD_NAME);
> +	const struct ec_board_info *board;
> +
> +	if (!dmi_board_vendor || !dmi_board_name ||
> +	    (strcasecmp(dmi_board_vendor, "ONE-NETBOOK TECHNOLOGY CO., LTD.") &&
> +	     strcasecmp(dmi_board_vendor, "ONE-NETBOOK")))
> +		return NULL;
> +
> +	/* Match our known boards */
> +	/* Need to check for AMD/Intel here */
> +	for (board = board_info; board->sensors; board++) {
> +		if (match_string(board->board_names,
> +				 MAX_IDENTICAL_BOARD_VARIATIONS,
> +				 dmi_board_name) >= 0) {
> +			if (board->family == family_mini_amd &&
> +					boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
> +				return board;
> +			}
> +		}
> +	}
> +	return NULL;

Why not use a dmi match table for the above code ?

> +}
> +
> +static int __init oxp_platform_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device *hwdev;
> +	const struct ec_board_info *pboard_info;
> +	struct oxp_status *state;
> +
> +	pboard_info = get_board_info(dev);
> +	if (!pboard_info)
> +		return -ENODEV;
> +
> +	state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->board = *pboard_info;
> +
> +	state->lock_data.mutex = 0;
> +	state->lock_data.lock = lock_global_acpi_lock;
> +	state->lock_data.unlock = unlock_global_acpi_lock;
> +
> +	hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", state,
> +							&oxp_ec_chip_info, NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwdev);

This only configures a hwmon driver and thus should reside in the hwmon
subsystem/directory.

> +}
> +
> +static const struct acpi_device_id acpi_ec_ids[] = {
> +	/* Embedded Controller Device */
> +	{ "PNP0C09", 0 },
> +	{}
> +};

I am not sure if this works. There are other drivers mapping to the same ACPI ID;
those may refuse to load if this driver is in the system. We had problems with
this before, so I am very concerned about side effects.

> +
> +static struct platform_driver oxp_platform_driver = {
> +	.driver = {
> +		.name	= "oxp-platform",
> +		.acpi_match_table = acpi_ec_ids,
> +	},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
> +module_platform_driver_probe(oxp_platform_driver, oxp_platform_probe);
> +
> +MODULE_AUTHOR("Joaquín Ignacio Aramendía <samsagax@gmail.com>");
> +MODULE_DESCRIPTION(
> +	"Platform driver that handles ACPI EC of OneXPlayer devices");
> +MODULE_LICENSE("GPL");
> --
> 2.38.1
>