* [PATCH] Add OneXPlayer mini AMD board driver
@ 2022-10-29 22:50 Joaquín Ignacio Aramendía
2022-10-29 23:30 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Joaquín Ignacio Aramendía @ 2022-10-29 22:50 UTC (permalink / raw)
To: hdegoede
Cc: markgross, jdelvare, linux, platform-driver-x86, linux-hwmon,
Joaquín Ignacio Aramendía
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);
+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;
+ };
+ };
+};
+
+
+/* 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,
+ },
+ {},
+};
+
+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,
+ },
+ {}
+};
+
+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;
+ *val += buffer;
+ }
+ return ret;
+}
+
+static int write_to_ec(const struct device *dev, u8 reg, u8 value)
+{
+ struct oxp_status *state = dev_get_drvdata(dev);
+ int ret = -1;
+
+ 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;
+ default:
+ return 0;
+ }
+ return 0;
+}
+
+static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ 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;
+ default:
+ dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
+ }
+ 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;
+ }
+ break;
+ default:
+ dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
+ }
+ return ret;
+ default:
+ dev_dbg(dev, "Unknown sensor type %d.\n", type);
+ return -1;
+ }
+}
+
+static int oxp_pwm_enable(const struct device *dev)
+{
+ int ret = -1;
+ 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;
+}
+
+static int oxp_pwm_disable(const struct device *dev)
+{
+ int ret = -1;
+ 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;
+ }
+
+ ret = write_to_ec(dev, board->sensors[oxp_sensor_pwm].enable,
+ board->sensors[oxp_sensor_pwm].val_disable);
+
+ return ret;
+}
+
+static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ int ret = -1;
+ 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;
+ }
+ 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);
+ }
+ 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),
+ 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;
+}
+
+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);
+}
+
+static const struct acpi_device_id acpi_ec_ids[] = {
+ /* Embedded Controller Device */
+ { "PNP0C09", 0 },
+ {}
+};
+
+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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] Add OneXPlayer mini AMD board driver
2022-10-29 22:50 [PATCH] Add OneXPlayer mini AMD board driver Joaquín Ignacio Aramendía
@ 2022-10-29 23:30 ` Guenter Roeck
2022-10-30 0:29 ` Joaquin Aramendia
0 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2022-10-29 23:30 UTC (permalink / raw)
To: Joaquín Ignacio Aramendía, hdegoede
Cc: markgross, jdelvare, platform-driver-x86, linux-hwmon
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Add OneXPlayer mini AMD board driver
2022-10-29 23:30 ` Guenter Roeck
@ 2022-10-30 0:29 ` Joaquin Aramendia
2022-10-30 3:24 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Joaquin Aramendia @ 2022-10-30 0:29 UTC (permalink / raw)
To: Guenter Roeck
Cc: hdegoede, markgross, jdelvare, platform-driver-x86, linux-hwmon
Hello, thanks so much to take the time for the feedback!
Maybe this one needs some clarification as for some design choices in
regards to some of the structures you see. I know there is only one
board supported at the moment, but those structures are meant to be
the boilerplate for possibly more boards. I'm aware that these devices
are a mess and the EC registers are all over the place. I wanted to
make it easier for myself. That said I'll try to address some of the
concerns and redo the patcha according to standards :)
El sáb, 29 oct 2022 a la(s) 20:30, Guenter Roeck (linux@roeck-us.net) escribió:
>
> On 10/29/22 15:50, Joaquín Ignacio Aramendía wrote:
> > +
> > +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 ?
Is meant for safety, I suppose, but you are right that it is better to
have the parameter non-writable at runtime.
The goal is to be able to use the fan readings without allowing the
pwm to be used unless you really know what you are doing, but I guess
there is no point in having this if already is the pwm_enable
attribute.
> > +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.
Removed. Thanks.
> > +/* 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 ?
It is one entry now, but i figured in a while there will be other
boards to support with different values. Having this structure seems
easier for future updates.
I can remove it and only use defines for now.
> > + {},
> > +};
> > +
> > +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 ?
It is meant for expansion on other boards. I only own a OXP mini AMD,
but others exist with their own quirks. For example, the same device
but with Intel CPU has completely different EC registers, values and
ranges.
I guess i can remove the whole "family" thing and bring it back when
it is appropriate.
> > + {}
> > +};
> > +
> > +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
Ok. Will remove.
> > + *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
Ok. Will improve on this.
> > +
> > + 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.
Oh. I did as the documentation suggested. I must confess I didn't run
checkpatch, will don in the next submission.
> > + 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.
Will do. I guess my vim settings are messed up.
> > +{
> > + 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.
They are not, but they are in the spec of the fan (in the attached
sticker, that is). Should I keep it if it's not read directly but has
a known value?
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
>
> missing break;
Ooops, sorry. Will improve on this one.
> > + }
> > + 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 ?
>
The enable/disable values are configurable, since they can vary from
board to board. AMD happens to be just 1 and 0, so it fits in this
case. My goal is to map them to 1 and 0 to be exposed in the same way
across all devices. That way userspace tools like fancontrold can use
this interface as is.
> > + break;
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
>
> missing break;
Oops, will correct. Thanks
> > + }
> > + return ret;
> > + default:
> > + dev_dbg(dev, "Unknown sensor type %d.\n", type);
> > + return -1;
>
> bad error code.
Should I return EINVAL in this case? Seems appropriate reading error
code headers.
> > + }
> > +}
> > +
> > +static int oxp_pwm_enable(const struct device *dev)
> > +{
> > + int ret = -1;
>
> Unnecessary (and bad) variable initialization.
Ok. Will improve.
> > + 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.
Then I would just do
return write_to_ec():
as stated below.
> > +}
> > +
> > +static int oxp_pwm_disable(const struct device *dev)
> > +{
> > + int ret = -1;
>
> Unnecessary initialization, and bad error code.
Seems like I really like to mess this up, really... Sorry.
> > + 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.
>
Ok. I can remove the `fan_control` parameter entirely and just leave
it to the userspace to handle pwm_enable without any other safeguards.
> > +
> > + 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.
>
Same as above, so I'll just do this.
> > +}
> > +
> > +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.
Ok. Will improve on this.
> > + 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.
Ok. Will improve on this one.
> > + 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
Oops... noted.
> > + }
> > + 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.
Will do, sorry.
> > + 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 ?
I could use a dmi match table.
> > +}
> > +
> > +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.
For now yes, it only provides hwmon. I'll move to it then.
> > +}
> > +
> > +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.
So should I just remove this and go for the DMI match table instead?
> > +
> > +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
> >
>
Thanks very much for taking the time.
--
Joaquín I. Aramendía
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Add OneXPlayer mini AMD board driver
2022-10-30 0:29 ` Joaquin Aramendia
@ 2022-10-30 3:24 ` Guenter Roeck
2022-10-30 20:32 ` [PATCH v2] Add OneXPlayer mini AMD sensors driver Joaquín Ignacio Aramendía
2022-10-31 11:45 ` [PATCH] Add OneXPlayer mini AMD board driver Joaquin Aramendia
0 siblings, 2 replies; 19+ messages in thread
From: Guenter Roeck @ 2022-10-30 3:24 UTC (permalink / raw)
To: Joaquin Aramendia
Cc: hdegoede, markgross, jdelvare, platform-driver-x86, linux-hwmon
On 10/29/22 17:29, Joaquin Aramendia wrote:
> Hello, thanks so much to take the time for the feedback!
> Maybe this one needs some clarification as for some design choices in
> regards to some of the structures you see. I know there is only one
> board supported at the moment, but those structures are meant to be
> the boilerplate for possibly more boards. I'm aware that these devices
> are a mess and the EC registers are all over the place. I wanted to
> make it easier for myself. That said I'll try to address some of the
> concerns and redo the patcha according to standards :)
>
Please introduce such details when they are needed, not for a possible
which may never happen.
> El sáb, 29 oct 2022 a la(s) 20:30, Guenter Roeck (linux@roeck-us.net) escribió:
>>
>> On 10/29/22 15:50, Joaquín Ignacio Aramendía wrote:
>>> +
>>> +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 ?
>
> Is meant for safety, I suppose, but you are right that it is better to
> have the parameter non-writable at runtime.
> The goal is to be able to use the fan readings without allowing the
> pwm to be used unless you really know what you are doing, but I guess
> there is no point in having this if already is the pwm_enable
> attribute.
>
Exactly. I do not see the pont of the attribute in the first place since
it just adds complexity and duplicates pwm_enable.
>>> +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.
>
> Removed. Thanks.
>
>>> +/* 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 ?
>
> It is one entry now, but i figured in a while there will be other
> boards to support with different values. Having this structure seems
> easier for future updates.
> I can remove it and only use defines for now.
>
Please do.
>>> + {},
>>> +};
>>> +
>>> +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 ?
>
> It is meant for expansion on other boards. I only own a OXP mini AMD,
> but others exist with their own quirks. For example, the same device
> but with Intel CPU has completely different EC registers, values and
> ranges.
> I guess i can remove the whole "family" thing and bring it back when
> it is appropriate.
>
Without knowing those boards it may well be that a separate driver would
be appropriate, and/or that the here introduced flexibility is insufficient.
It does not make sense to introduce such complexity unless it is known
that it is needed, and that it meets the requirements of additional boards.
>>> + {}
>>> +};
>>> +
>>> +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
>
> Ok. Will remove.
>
>>> + *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
>
> Ok. Will improve on this.
>
>>> +
>>> + 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.
>
> Oh. I did as the documentation suggested. I must confess I didn't run
> checkpatch, will don in the next submission.
>
That is long ago. Octal values are and have been preferred for
several years.
>>> + 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.
>
> Will do. I guess my vim settings are messed up.
>
>>> +{
>>> + 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.
>
> They are not, but they are in the spec of the fan (in the attached
> sticker, that is). Should I keep it if it's not read directly but has
> a known value?
>
No. That is not the purpose of hwmon sysfs attributes.
>>> + default:
>>> + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
>>
>> missing break;
> Ooops, sorry. Will improve on this one.
>
>>> + }
>>> + 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 ?
>>
> The enable/disable values are configurable, since they can vary from
> board to board. AMD happens to be just 1 and 0, so it fits in this
> case. My goal is to map them to 1 and 0 to be exposed in the same way
> across all devices. That way userspace tools like fancontrold can use
> this interface as is.
>
Again, that is not known at this time. Unless there are such boards
it is impossible to predict how they are programmed, and if that
programming even remotely fits into the scheme used by this driver.
>>> + break;
>>> + default:
>>> + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
>>
>> missing break;
> Oops, will correct. Thanks
>
>>> + }
>>> + return ret;
>>> + default:
>>> + dev_dbg(dev, "Unknown sensor type %d.\n", type);
>>> + return -1;
>>
>> bad error code.
> Should I return EINVAL in this case? Seems appropriate reading error
> code headers.
>
-EOPNOTSUPP
>>> + }
>>> +}
>>> +
>>> +static int oxp_pwm_enable(const struct device *dev)
>>> +{
>>> + int ret = -1;
>>
>> Unnecessary (and bad) variable initialization.
>
> Ok. Will improve.
>
>>> + 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.
> Then I would just do
> return write_to_ec():
> as stated below.
>
>>> +}
>>> +
>>> +static int oxp_pwm_disable(const struct device *dev)
>>> +{
>>> + int ret = -1;
>>
>> Unnecessary initialization, and bad error code.
>
> Seems like I really like to mess this up, really... Sorry.
>
>>> + 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.
>>
> Ok. I can remove the `fan_control` parameter entirely and just leave
> it to the userspace to handle pwm_enable without any other safeguards.
>
>>> +
>>> + 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.
>>
> Same as above, so I'll just do this.
>
>>> +}
>>> +
>>> +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.
> Ok. Will improve on this.
>
>>> + 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.
> Ok. Will improve on this one.
>
>>> + 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
> Oops... noted.
>
>>> + }
>>> + 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.
> Will do, sorry.
>
>>> + 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 ?
> I could use a dmi match table.
>
Please do.
>>> +}
>>> +
>>> +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.
>
> For now yes, it only provides hwmon. I'll move to it then.
>
>>> +}
>>> +
>>> +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.
>
> So should I just remove this and go for the DMI match table instead?
>
Yes, and a platform driver. Look at drivers/hwmon/asus-ec-sensors.c and its
history (specifically commit 88700d1396b) for problems with PNP0C09.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] Add OneXPlayer mini AMD sensors driver
2022-10-30 3:24 ` Guenter Roeck
@ 2022-10-30 20:32 ` Joaquín Ignacio Aramendía
2022-10-31 0:03 ` Barnabás Pőcze
2022-10-31 11:45 ` [PATCH] Add OneXPlayer mini AMD board driver Joaquin Aramendia
1 sibling, 1 reply; 19+ messages in thread
From: Joaquín Ignacio Aramendía @ 2022-10-30 20:32 UTC (permalink / raw)
To: hdegoede
Cc: markgross, jdelvare, linux, platform-driver-x86, linux-hwmon,
Joaquín Ignacio Aramendía
Sensors driver for OXP Handhelds from One-Netbook 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.
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
accommodate for that.
Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
---
Rewritten the driver according to feedback, checkpatch passes, removed
unnecessary complexity and moved the driver to hwmon
---
drivers/hwmon/Kconfig | 13 +-
drivers/hwmon/Makefile | 1 +
drivers/hwmon/oxp-sensors.c | 278 ++++++++++++++++++++++++++++++++++++
3 files changed, 291 insertions(+), 1 deletion(-)
create mode 100644 drivers/hwmon/oxp-sensors.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ac3daaf59ce..baa3e43b52b6 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
source "drivers/hwmon/occ/Kconfig"
+config SENSORS_OXP
+ tristate "OneXPlayer EC fan control"
+ depends on ACPI
+ depends on X86
+ help
+ If you say yes here you get support for fan readings and control over
+ OneXPlayer handheld devices. Only OneXPlayer mini AMD handheld variant
+ boards are supported.
+
+ Can also be built as a module. In that case it will be called oxp_sensors.
+
config SENSORS_PCF8591
tristate "Philips PCF8591 ADC/DAC"
depends on I2C
@@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
config SENSORS_AMC6821
tristate "Texas Instruments AMC6821"
- depends on I2C
+ depends on I2C
help
If you say yes here you get support for the Texas Instruments
AMC6821 hardware monitoring chips.
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 11d076cad8a2..35824f8be455 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o
obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
+obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
new file mode 100644
index 000000000000..128fdf4c46e2
--- /dev/null
+++ b/drivers/hwmon/oxp-sensors.c
@@ -0,0 +1,278 @@
+// 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
+ * accommodate 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 <linux/processor.h>
+
+#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */
+#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */
+
+static const struct dmi_system_id dmi_table[] = {
+ {
+ .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
+ "ONE-NETBOOK TECHNOLOGY CO., LTD."),
+ },
+ },
+ {
+ .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
+ "ONE-NETBOOK"),
+ },
+ },
+ {},
+};
+
+struct oxp_status {
+ struct lock_data lock_data;
+};
+
+/* Helper functions to handle EC read/write */
+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;
+ *val += buffer;
+ }
+ return ret;
+}
+
+static int write_to_ec(const struct device *dev, u8 reg, u8 value)
+{
+ struct oxp_status *state = dev_get_drvdata(dev);
+ int ret;
+
+ 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;
+}
+
+static int oxp_pwm_enable(const struct device *dev)
+{
+ return write_to_ec(dev, OXP_SENSOR_PWM_REG, 0x01);
+}
+
+static int oxp_pwm_disable(const struct device *dev)
+{
+ return write_to_ec(dev, OXP_SENSOR_PWM_REG, 0x00);
+}
+
+/* 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 0444;
+ case hwmon_pwm:
+ return 0644;
+ default:
+ return 0;
+ }
+ return 0;
+}
+
+static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ int ret;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ return read_from_ec(OXP_SENSOR_FAN_REG,
+ 2,
+ val);
+ default:
+ dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
+ return -EOPNOTSUPP;
+ }
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ ret = read_from_ec(OXP_SENSOR_PWM_REG,
+ 2, val);
+ *val = (*val * 255) / 100;
+ return ret;
+ case hwmon_pwm_enable:
+ return read_from_ec(OXP_SENSOR_PWM_REG, 1, val);
+ default:
+ dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
+ return -EOPNOTSUPP;
+ }
+ default:
+ dev_dbg(dev, "Unknown sensor type %d.\n", type);
+ return -EOPNOTSUPP;
+ }
+}
+
+static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ switch (type) {
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_enable:
+ if (val == 1)
+ return oxp_pwm_enable(dev);
+ else if (val == 0)
+ return oxp_pwm_disable(dev);
+ else
+ return -EINVAL;
+ case hwmon_pwm_input:
+ if (val < 0 || val > 255)
+ return -EINVAL;
+ val = (val * 100) / 255;
+ return write_to_ec(dev, OXP_SENSOR_PWM_REG, val);
+ default:
+ dev_dbg(dev, "Unknown attribute for type %d: %d", type, attr);
+ return -EOPNOTSUPP;
+ }
+ default:
+ dev_dbg(dev, "Unknown sensor type: %d", type);
+ return -EOPNOTSUPP;
+ }
+ return -EINVAL;
+}
+
+/* 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),
+ 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 int oxp_platform_probe(struct platform_device *pdev)
+{
+ const struct dmi_system_id *dmi_entry;
+ struct device *dev = &pdev->dev;
+ struct device *hwdev;
+ struct oxp_status *state;
+
+ /* Have to check for AMD processor here */
+ dmi_entry = dmi_first_match(dmi_table);
+ if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+ return -ENODEV;
+
+ state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;
+
+ 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);
+}
+
+static struct platform_driver oxp_platform_driver = {
+ .driver = {
+ .name = "oxp-platform",
+ },
+ .probe = oxp_platform_probe,
+};
+
+static struct platform_device *oxp_platform_device;
+
+static int __init oxp_platform_init(void)
+{
+ oxp_platform_device =
+ platform_create_bundle(&oxp_platform_driver,
+ oxp_platform_probe, NULL, 0, NULL, 0);
+
+ if (IS_ERR(oxp_platform_device))
+ return PTR_ERR(oxp_platform_device);
+
+ return 0;
+}
+
+static void __exit oxp_platform_exit(void)
+{
+ platform_device_unregister(oxp_platform_device);
+ platform_driver_unregister(&oxp_platform_driver);
+}
+
+MODULE_DEVICE_TABLE(dmi, dmi_table);
+module_init(oxp_platform_init);
+module_exit(oxp_platform_exit);
+
+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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] Add OneXPlayer mini AMD sensors driver
2022-10-30 20:32 ` [PATCH v2] Add OneXPlayer mini AMD sensors driver Joaquín Ignacio Aramendía
@ 2022-10-31 0:03 ` Barnabás Pőcze
2022-10-31 14:49 ` Joaquin Aramendia
2022-10-31 14:53 ` [PATCH v3] " Joaquín Ignacio Aramendía
0 siblings, 2 replies; 19+ messages in thread
From: Barnabás Pőcze @ 2022-10-31 0:03 UTC (permalink / raw)
To: Joaquín Ignacio Aramendía
Cc: hdegoede, markgross, jdelvare, linux, platform-driver-x86, linux-hwmon
Hi
2022. október 30., vasárnap 21:32 keltezéssel, Joaquín Ignacio Aramendía írta:
> Sensors driver for OXP Handhelds from One-Netbook 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.
>
> 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
> accommodate for that.
>
> Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> ---
> Rewritten the driver according to feedback, checkpatch passes, removed
> unnecessary complexity and moved the driver to hwmon
> ---
> drivers/hwmon/Kconfig | 13 +-
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/oxp-sensors.c | 278 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 291 insertions(+), 1 deletion(-)
> create mode 100644 drivers/hwmon/oxp-sensors.c
>
> [...]
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> new file mode 100644
> index 000000000000..128fdf4c46e2
> --- /dev/null
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -0,0 +1,278 @@
> +// 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
> + * accommodate for that.
> + *
> + * PWM control is disabled by default, can be enabled via module parameter.
As far as I can see this is not true anymore.
Also, have you checked if there is maybe a WMI interface for this?
> [...]
Regards,
Barnabás Pőcze
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Add OneXPlayer mini AMD board driver
2022-10-30 3:24 ` Guenter Roeck
2022-10-30 20:32 ` [PATCH v2] Add OneXPlayer mini AMD sensors driver Joaquín Ignacio Aramendía
@ 2022-10-31 11:45 ` Joaquin Aramendia
2022-10-31 13:07 ` Guenter Roeck
1 sibling, 1 reply; 19+ messages in thread
From: Joaquin Aramendia @ 2022-10-31 11:45 UTC (permalink / raw)
To: Guenter Roeck
Cc: hdegoede, markgross, jdelvare, platform-driver-x86, linux-hwmon
Hello again and thanks for the review. I submitted the patch again and
needs some polish before it can be accepted. I had one question left:
El dom, 30 oct 2022 a la(s) 00:24, Guenter Roeck (linux@roeck-us.net) escribió:
>
> >>> +/* 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.
> >
> > Oh. I did as the documentation suggested. I must confess I didn't run
> > checkpatch, will don in the next submission.
> >
>
> That is long ago. Octal values are and have been preferred for
> several years.
I've read this form here[1]. Should I send a patch to the
Documentation to reflect the preference?
[1]https://www.kernel.org/doc/html/latest/hwmon/hwmon-kernel-api.html#driver-callback-functions
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Add OneXPlayer mini AMD board driver
2022-10-31 11:45 ` [PATCH] Add OneXPlayer mini AMD board driver Joaquin Aramendia
@ 2022-10-31 13:07 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2022-10-31 13:07 UTC (permalink / raw)
To: Joaquin Aramendia
Cc: hdegoede, markgross, jdelvare, platform-driver-x86, linux-hwmon
On Mon, Oct 31, 2022 at 08:45:35AM -0300, Joaquin Aramendia wrote:
> Hello again and thanks for the review. I submitted the patch again and
> needs some polish before it can be accepted. I had one question left:
>
> El dom, 30 oct 2022 a la(s) 00:24, Guenter Roeck (linux@roeck-us.net) escribió:
> >
> > >>> +/* 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.
> > >
> > > Oh. I did as the documentation suggested. I must confess I didn't run
> > > checkpatch, will don in the next submission.
> > >
> >
> > That is long ago. Octal values are and have been preferred for
> > several years.
>
> I've read this form here[1]. Should I send a patch to the
> Documentation to reflect the preference?
>
Sure, patches are always welcome.
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] Add OneXPlayer mini AMD sensors driver
2022-10-31 0:03 ` Barnabás Pőcze
@ 2022-10-31 14:49 ` Joaquin Aramendia
2022-10-31 14:53 ` [PATCH v3] " Joaquín Ignacio Aramendía
1 sibling, 0 replies; 19+ messages in thread
From: Joaquin Aramendia @ 2022-10-31 14:49 UTC (permalink / raw)
To: Barnabás Pőcze
Cc: hdegoede, markgross, jdelvare, linux, platform-driver-x86, linux-hwmon
El dom, 30 oct 2022 a la(s) 21:03, Barnabás Pőcze
(pobrn@protonmail.com) escribió:
>
> Hi
>
>
> 2022. október 30., vasárnap 21:32 keltezéssel, Joaquín Ignacio Aramendía írta:
>
> > Sensors driver for OXP Handhelds from One-Netbook 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.
> >
> > 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
> > accommodate for that.
> >
> > Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> > ---
> > Rewritten the driver according to feedback, checkpatch passes, removed
> > unnecessary complexity and moved the driver to hwmon
> > ---
> > drivers/hwmon/Kconfig | 13 +-
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/oxp-sensors.c | 278 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 291 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/hwmon/oxp-sensors.c
> >
> > [...]
> > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> > new file mode 100644
> > index 000000000000..128fdf4c46e2
> > --- /dev/null
> > +++ b/drivers/hwmon/oxp-sensors.c
> > @@ -0,0 +1,278 @@
> > +// 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
> > + * accommodate for that.
> > + *
> > + * PWM control is disabled by default, can be enabled via module parameter.
>
> As far as I can see this is not true anymore.
True. Will remove that. Also I found a typo on my registers that needs
to be fixed
>
> Also, have you checked if there is maybe a WMI interface for this?
There is not as far as I can tell. Even on Windows the fan control app
uses EC registers directly.
--
Joaquín I. Aramendía
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 0:03 ` Barnabás Pőcze
2022-10-31 14:49 ` Joaquin Aramendia
@ 2022-10-31 14:53 ` Joaquín Ignacio Aramendía
2022-10-31 15:34 ` Guenter Roeck
` (2 more replies)
1 sibling, 3 replies; 19+ messages in thread
From: Joaquín Ignacio Aramendía @ 2022-10-31 14:53 UTC (permalink / raw)
To: pobrn
Cc: hdegoede, jdelvare, linux-hwmon, linux, markgross,
platform-driver-x86, Joaquín Ignacio Aramendía
Sensors driver for OXP Handhelds from One-Netbook 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.
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
accommodate for that.
Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
---
Removed fan_control reference in comment.
Bugfix MIX/MIN reporting not available
Bugfix pwm_enable register set wrong
---
drivers/hwmon/Kconfig | 13 +-
drivers/hwmon/Makefile | 1 +
drivers/hwmon/oxp-sensors.c | 277 ++++++++++++++++++++++++++++++++++++
3 files changed, 290 insertions(+), 1 deletion(-)
create mode 100644 drivers/hwmon/oxp-sensors.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ac3daaf59ce..a1cdb03b4d13 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
source "drivers/hwmon/occ/Kconfig"
+config SENSORS_OXP
+ tristate "OneXPlayer EC fan control"
+ depends on ACPI
+ depends on X86
+ help
+ If you say yes here you get support for fan readings and control over
+ OneXPlayer handheld devices. Only OneXPlayer mini AMD handheld variant
+ boards are supported.
+
+ Can also be built as a module. In that case it will be called oxp-sensors.
+
config SENSORS_PCF8591
tristate "Philips PCF8591 ADC/DAC"
depends on I2C
@@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
config SENSORS_AMC6821
tristate "Texas Instruments AMC6821"
- depends on I2C
+ depends on I2C
help
If you say yes here you get support for the Texas Instruments
AMC6821 hardware monitoring chips.
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 11d076cad8a2..35824f8be455 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o
obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
+obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
new file mode 100644
index 000000000000..f5895dc11094
--- /dev/null
+++ b/drivers/hwmon/oxp-sensors.c
@@ -0,0 +1,277 @@
+// 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
+ * accommodate for that.
+ *
+ * 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 <linux/processor.h>
+
+#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */
+#define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */
+#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */
+
+static const struct dmi_system_id dmi_table[] = {
+ {
+ .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
+ "ONE-NETBOOK TECHNOLOGY CO., LTD."),
+ },
+ },
+ {
+ .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
+ "ONE-NETBOOK"),
+ },
+ },
+ {},
+};
+
+struct oxp_status {
+ struct lock_data lock_data;
+};
+
+/* Helper functions to handle EC read/write */
+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;
+ *val += buffer;
+ }
+ return ret;
+}
+
+static int write_to_ec(const struct device *dev, u8 reg, u8 value)
+{
+ struct oxp_status *state = dev_get_drvdata(dev);
+ int ret;
+
+ 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;
+}
+
+static int oxp_pwm_enable(const struct device *dev)
+{
+ return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x01);
+}
+
+static int oxp_pwm_disable(const struct device *dev)
+{
+ return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x00);
+}
+
+/* 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 0444;
+ case hwmon_pwm:
+ return 0644;
+ default:
+ return 0;
+ }
+ return 0;
+}
+
+static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ int ret;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ return read_from_ec(OXP_SENSOR_FAN_REG,
+ 2,
+ val);
+ default:
+ dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
+ return -EOPNOTSUPP;
+ }
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ ret = read_from_ec(OXP_SENSOR_PWM_REG,
+ 2, val);
+ *val = (*val * 255) / 100;
+ return ret;
+ case hwmon_pwm_enable:
+ return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
+ default:
+ dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
+ return -EOPNOTSUPP;
+ }
+ default:
+ dev_dbg(dev, "Unknown sensor type %d.\n", type);
+ return -EOPNOTSUPP;
+ }
+}
+
+static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ switch (type) {
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_enable:
+ if (val == 1)
+ return oxp_pwm_enable(dev);
+ else if (val == 0)
+ return oxp_pwm_disable(dev);
+ else
+ return -EINVAL;
+ case hwmon_pwm_input:
+ if (val < 0 || val > 255)
+ return -EINVAL;
+ val = (val * 100) / 255;
+ return write_to_ec(dev, OXP_SENSOR_PWM_REG, val);
+ default:
+ dev_dbg(dev, "Unknown attribute for type %d: %d", type, attr);
+ return -EOPNOTSUPP;
+ }
+ default:
+ dev_dbg(dev, "Unknown sensor type: %d", type);
+ return -EOPNOTSUPP;
+ }
+ return -EINVAL;
+}
+
+/* Known sensors in the OXP EC controllers */
+static const struct hwmon_channel_info *oxp_platform_sensors[] = {
+ HWMON_CHANNEL_INFO(fan,
+ HWMON_F_INPUT),
+ HWMON_CHANNEL_INFO(pwm,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
+ 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 int oxp_platform_probe(struct platform_device *pdev)
+{
+ const struct dmi_system_id *dmi_entry;
+ struct device *dev = &pdev->dev;
+ struct device *hwdev;
+ struct oxp_status *state;
+
+ /* Have to check for AMD processor here */
+ dmi_entry = dmi_first_match(dmi_table);
+ if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+ return -ENODEV;
+
+ state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;
+
+ 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);
+}
+
+static struct platform_driver oxp_platform_driver = {
+ .driver = {
+ .name = "oxp-platform",
+ },
+ .probe = oxp_platform_probe,
+};
+
+static struct platform_device *oxp_platform_device;
+
+static int __init oxp_platform_init(void)
+{
+ oxp_platform_device =
+ platform_create_bundle(&oxp_platform_driver,
+ oxp_platform_probe, NULL, 0, NULL, 0);
+
+ if (IS_ERR(oxp_platform_device))
+ return PTR_ERR(oxp_platform_device);
+
+ return 0;
+}
+
+static void __exit oxp_platform_exit(void)
+{
+ platform_device_unregister(oxp_platform_device);
+ platform_driver_unregister(&oxp_platform_driver);
+}
+
+MODULE_DEVICE_TABLE(dmi, dmi_table);
+module_init(oxp_platform_init);
+module_exit(oxp_platform_exit);
+
+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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 14:53 ` [PATCH v3] " Joaquín Ignacio Aramendía
@ 2022-10-31 15:34 ` Guenter Roeck
2022-10-31 15:57 ` Joaquin Aramendia
2022-10-31 16:43 ` Limonciello, Mario
2022-10-31 19:56 ` Guenter Roeck
2 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2022-10-31 15:34 UTC (permalink / raw)
To: Joaquín Ignacio Aramendía
Cc: pobrn, hdegoede, jdelvare, linux-hwmon, markgross, platform-driver-x86
On Mon, Oct 31, 2022 at 11:53:08AM -0300, Joaquín Ignacio Aramendía wrote:
> Sensors driver for OXP Handhelds from One-Netbook 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.
>
> 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
> accommodate for that.
>
> Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
I didn't have time to look at your patch, but _pease_ stop sending new
versions of your patches in response to previous versions.
Guenter
> ---
> Removed fan_control reference in comment.
> Bugfix MIX/MIN reporting not available
> Bugfix pwm_enable register set wrong
> ---
> drivers/hwmon/Kconfig | 13 +-
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/oxp-sensors.c | 277 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 290 insertions(+), 1 deletion(-)
> create mode 100644 drivers/hwmon/oxp-sensors.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7ac3daaf59ce..a1cdb03b4d13 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
>
> source "drivers/hwmon/occ/Kconfig"
>
> +config SENSORS_OXP
> + tristate "OneXPlayer EC fan control"
> + depends on ACPI
> + depends on X86
> + help
> + If you say yes here you get support for fan readings and control over
> + OneXPlayer handheld devices. Only OneXPlayer mini AMD handheld variant
> + boards are supported.
> +
> + Can also be built as a module. In that case it will be called oxp-sensors.
> +
> config SENSORS_PCF8591
> tristate "Philips PCF8591 ADC/DAC"
> depends on I2C
> @@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
>
> config SENSORS_AMC6821
> tristate "Texas Instruments AMC6821"
> - depends on I2C
> + depends on I2C
> help
> If you say yes here you get support for the Texas Instruments
> AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 11d076cad8a2..35824f8be455 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o
> obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
> +obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
> obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> new file mode 100644
> index 000000000000..f5895dc11094
> --- /dev/null
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -0,0 +1,277 @@
> +// 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
> + * accommodate for that.
> + *
> + * 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 <linux/processor.h>
> +
> +#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */
> +#define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */
> +#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */
> +
> +static const struct dmi_system_id dmi_table[] = {
> + {
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> + "ONE-NETBOOK TECHNOLOGY CO., LTD."),
> + },
> + },
> + {
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> + "ONE-NETBOOK"),
> + },
> + },
> + {},
> +};
> +
> +struct oxp_status {
> + struct lock_data lock_data;
> +};
> +
> +/* Helper functions to handle EC read/write */
> +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;
> + *val += buffer;
> + }
> + return ret;
> +}
> +
> +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> +{
> + struct oxp_status *state = dev_get_drvdata(dev);
> + int ret;
> +
> + 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;
> +}
> +
> +static int oxp_pwm_enable(const struct device *dev)
> +{
> + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x01);
> +}
> +
> +static int oxp_pwm_disable(const struct device *dev)
> +{
> + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x00);
> +}
> +
> +/* 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 0444;
> + case hwmon_pwm:
> + return 0644;
> + default:
> + return 0;
> + }
> + return 0;
> +}
> +
> +static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + int ret;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + return read_from_ec(OXP_SENSOR_FAN_REG,
> + 2,
> + val);
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
> + return -EOPNOTSUPP;
> + }
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + ret = read_from_ec(OXP_SENSOR_PWM_REG,
> + 2, val);
> + *val = (*val * 255) / 100;
> + return ret;
> + case hwmon_pwm_enable:
> + return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
> + return -EOPNOTSUPP;
> + }
> + default:
> + dev_dbg(dev, "Unknown sensor type %d.\n", type);
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + switch (type) {
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_enable:
> + if (val == 1)
> + return oxp_pwm_enable(dev);
> + else if (val == 0)
> + return oxp_pwm_disable(dev);
> + else
> + return -EINVAL;
> + case hwmon_pwm_input:
> + if (val < 0 || val > 255)
> + return -EINVAL;
> + val = (val * 100) / 255;
> + return write_to_ec(dev, OXP_SENSOR_PWM_REG, val);
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d: %d", type, attr);
> + return -EOPNOTSUPP;
> + }
> + default:
> + dev_dbg(dev, "Unknown sensor type: %d", type);
> + return -EOPNOTSUPP;
> + }
> + return -EINVAL;
> +}
> +
> +/* Known sensors in the OXP EC controllers */
> +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> + HWMON_CHANNEL_INFO(fan,
> + HWMON_F_INPUT),
> + HWMON_CHANNEL_INFO(pwm,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> + 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 int oxp_platform_probe(struct platform_device *pdev)
> +{
> + const struct dmi_system_id *dmi_entry;
> + struct device *dev = &pdev->dev;
> + struct device *hwdev;
> + struct oxp_status *state;
> +
> + /* Have to check for AMD processor here */
> + dmi_entry = dmi_first_match(dmi_table);
> + if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> + return -ENODEV;
> +
> + state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> + if (!state)
> + return -ENOMEM;
> +
> + 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);
> +}
> +
> +static struct platform_driver oxp_platform_driver = {
> + .driver = {
> + .name = "oxp-platform",
> + },
> + .probe = oxp_platform_probe,
> +};
> +
> +static struct platform_device *oxp_platform_device;
> +
> +static int __init oxp_platform_init(void)
> +{
> + oxp_platform_device =
> + platform_create_bundle(&oxp_platform_driver,
> + oxp_platform_probe, NULL, 0, NULL, 0);
> +
> + if (IS_ERR(oxp_platform_device))
> + return PTR_ERR(oxp_platform_device);
> +
> + return 0;
> +}
> +
> +static void __exit oxp_platform_exit(void)
> +{
> + platform_device_unregister(oxp_platform_device);
> + platform_driver_unregister(&oxp_platform_driver);
> +}
> +
> +MODULE_DEVICE_TABLE(dmi, dmi_table);
> +module_init(oxp_platform_init);
> +module_exit(oxp_platform_exit);
> +
> +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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 15:34 ` Guenter Roeck
@ 2022-10-31 15:57 ` Joaquin Aramendia
0 siblings, 0 replies; 19+ messages in thread
From: Joaquin Aramendia @ 2022-10-31 15:57 UTC (permalink / raw)
To: Guenter Roeck
Cc: pobrn, hdegoede, jdelvare, linux-hwmon, markgross, platform-driver-x86
El lun, 31 oct 2022 a la(s) 12:34, Guenter Roeck (linux@roeck-us.net) escribió:
>
> I didn't have time to look at your patch, but _pease_ stop sending new
> versions of your patches in response to previous versions.
>
> Guenter
Sorry, new to this, not entirely sure about the etiquette of new
versions. The v3 is the last one and hopefully the definitive one.
--
Joaquín I. Aramendía
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 14:53 ` [PATCH v3] " Joaquín Ignacio Aramendía
2022-10-31 15:34 ` Guenter Roeck
@ 2022-10-31 16:43 ` Limonciello, Mario
2022-10-31 18:57 ` Joaquin Aramendia
2022-10-31 19:56 ` Guenter Roeck
2 siblings, 1 reply; 19+ messages in thread
From: Limonciello, Mario @ 2022-10-31 16:43 UTC (permalink / raw)
To: Joaquín Ignacio Aramendía, pobrn
Cc: hdegoede, jdelvare, linux-hwmon, linux, markgross, platform-driver-x86
[Public]
> -----Original Message-----
> From: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> Sent: Monday, October 31, 2022 09:53
> To: pobrn@protonmail.com
> Cc: hdegoede@redhat.com; jdelvare@suse.com; linux-
> hwmon@vger.kernel.org; linux@roeck-us.net; markgross@kernel.org;
> platform-driver-x86@vger.kernel.org; Joaquín Ignacio Aramendía
> <samsagax@gmail.com>
> Subject: [PATCH v3] Add OneXPlayer mini AMD sensors driver
>
> Sensors driver for OXP Handhelds from One-Netbook 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.
>
> 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
> accommodate for that.
>
> Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> ---
> Removed fan_control reference in comment.
> Bugfix MIX/MIN reporting not available
> Bugfix pwm_enable register set wrong
> ---
> drivers/hwmon/Kconfig | 13 +-
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/oxp-sensors.c | 277
> ++++++++++++++++++++++++++++++++++++
> 3 files changed, 290 insertions(+), 1 deletion(-)
> create mode 100644 drivers/hwmon/oxp-sensors.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7ac3daaf59ce..a1cdb03b4d13 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
>
> source "drivers/hwmon/occ/Kconfig"
>
> +config SENSORS_OXP
> + tristate "OneXPlayer EC fan control"
> + depends on ACPI
> + depends on X86
> + help
> + If you say yes here you get support for fan readings and
> control over
> + OneXPlayer handheld devices. Only OneXPlayer mini AMD
> handheld variant
> + boards are supported.
> +
> + Can also be built as a module. In that case it will be called oxp-
> sensors.
> +
> config SENSORS_PCF8591
> tristate "Philips PCF8591 ADC/DAC"
> depends on I2C
> @@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
>
> config SENSORS_AMC6821
> tristate "Texas Instruments AMC6821"
> - depends on I2C
> + depends on I2C
> help
> If you say yes here you get support for the Texas Instruments
> AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 11d076cad8a2..35824f8be455 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-
> hwmon.o
> obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
> +obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
> obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> new file mode 100644
> index 000000000000..f5895dc11094
> --- /dev/null
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -0,0 +1,277 @@
> +// 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
> + * accommodate for that.
What happens on the Intel variant with this code? Are they not the same EC?
Why doesn't it work there? If you keep the AMD check in the code, I think it
would be good to document the problems with the Intel one at least.
> + *
> + * 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 <linux/processor.h>
> +
> +#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2
> registers long */
> +#define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1
> register long */
> +#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1
> register long */
> +
> +static const struct dmi_system_id dmi_table[] = {
> + {
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> + "ONE-NETBOOK TECHNOLOGY CO.,
> LTD."),
> + },
> + },
> + {
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> + "ONE-NETBOOK"),
> + },
> + },
> + {},
> +};
> +
> +struct oxp_status {
> + struct lock_data lock_data;
> +};
> +
> +/* Helper functions to handle EC read/write */
> +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;
> + *val += buffer;
> + }
> + return ret;
Don't you need to acquire your mutex for reading too?
Otherwise you could potentially have userspace trying to read
and write another at the same time and get indeterminate results.
> +}
> +
> +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> +{
> + struct oxp_status *state = dev_get_drvdata(dev);
> + int ret;
> +
> + 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;
> +}
> +
> +static int oxp_pwm_enable(const struct device *dev)
> +{
> + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x01);
> +}
> +
> +static int oxp_pwm_disable(const struct device *dev)
> +{
> + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x00);
> +}
> +
> +/* 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 0444;
> + case hwmon_pwm:
> + return 0644;
> + default:
> + return 0;
> + }
> + return 0;
> +}
> +
> +static int oxp_platform_read(struct device *dev, enum
> hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + int ret;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + return read_from_ec(OXP_SENSOR_FAN_REG,
> + 2,
> + val);
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d:
> %d\n", type, attr);
> + return -EOPNOTSUPP;
> + }
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + ret = read_from_ec(OXP_SENSOR_PWM_REG,
> + 2, val);
> + *val = (*val * 255) / 100;
> + return ret;
> + case hwmon_pwm_enable:
> + return
> read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d:
> %d\n", type, attr);
> + return -EOPNOTSUPP;
> + }
> + default:
> + dev_dbg(dev, "Unknown sensor type %d.\n", type);
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int oxp_platform_write(struct device *dev, enum
> hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + switch (type) {
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_enable:
> + if (val == 1)
> + return oxp_pwm_enable(dev);
> + else if (val == 0)
> + return oxp_pwm_disable(dev);
> + else
> + return -EINVAL;
> + case hwmon_pwm_input:
> + if (val < 0 || val > 255)
> + return -EINVAL;
> + val = (val * 100) / 255;
> + return write_to_ec(dev, OXP_SENSOR_PWM_REG,
> val);
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d: %d",
> type, attr);
> + return -EOPNOTSUPP;
> + }
> + default:
> + dev_dbg(dev, "Unknown sensor type: %d", type);
> + return -EOPNOTSUPP;
> + }
> + return -EINVAL;
Can you actually hit this scenario? I would think not; you'll hit "default" and return -EOPNOTSUPP.
Maybe it's better to just drop the default label and then outside the switch/case do this:
dev_dbg(dev, "Unknown sensor type: %d", type);
return -EOPNOTSUPP;
> +}
> +
> +/* Known sensors in the OXP EC controllers */
> +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> + HWMON_CHANNEL_INFO(fan,
> + HWMON_F_INPUT),
> + HWMON_CHANNEL_INFO(pwm,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> + 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 int oxp_platform_probe(struct platform_device *pdev)
> +{
> + const struct dmi_system_id *dmi_entry;
> + struct device *dev = &pdev->dev;
> + struct device *hwdev;
> + struct oxp_status *state;
> +
> + /* Have to check for AMD processor here */
> + dmi_entry = dmi_first_match(dmi_table);
> + if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> + return -ENODEV;
So it's shared DMI data values for the Intel and AMD variants of this platform? What
happens if you run all this code on the Intel one?
> +
> + state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> + if (!state)
> + return -ENOMEM;
> +
> + 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);
> +}
> +
> +static struct platform_driver oxp_platform_driver = {
> + .driver = {
> + .name = "oxp-platform",
> + },
> + .probe = oxp_platform_probe,
> +};
> +
> +static struct platform_device *oxp_platform_device;
> +
> +static int __init oxp_platform_init(void)
> +{
> + oxp_platform_device =
> + platform_create_bundle(&oxp_platform_driver,
> + oxp_platform_probe, NULL, 0, NULL, 0);
> +
> + if (IS_ERR(oxp_platform_device))
> + return PTR_ERR(oxp_platform_device);
> +
> + return 0;
> +}
> +
> +static void __exit oxp_platform_exit(void)
> +{
> + platform_device_unregister(oxp_platform_device);
> + platform_driver_unregister(&oxp_platform_driver);
> +}
> +
> +MODULE_DEVICE_TABLE(dmi, dmi_table);
> +module_init(oxp_platform_init);
> +module_exit(oxp_platform_exit);
> +
> +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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 16:43 ` Limonciello, Mario
@ 2022-10-31 18:57 ` Joaquin Aramendia
2022-10-31 19:21 ` Limonciello, Mario
0 siblings, 1 reply; 19+ messages in thread
From: Joaquin Aramendia @ 2022-10-31 18:57 UTC (permalink / raw)
To: Limonciello, Mario
Cc: pobrn, hdegoede, jdelvare, linux-hwmon, linux, markgross,
platform-driver-x86
El lun, 31 oct 2022 a la(s) 13:43, Limonciello, Mario
(Mario.Limonciello@amd.com) escribió:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> > Sent: Monday, October 31, 2022 09:53
> > To: pobrn@protonmail.com
> > Cc: hdegoede@redhat.com; jdelvare@suse.com; linux-
> > hwmon@vger.kernel.org; linux@roeck-us.net; markgross@kernel.org;
> > platform-driver-x86@vger.kernel.org; Joaquín Ignacio Aramendía
> > <samsagax@gmail.com>
> > Subject: [PATCH v3] Add OneXPlayer mini AMD sensors driver
> >
> > Sensors driver for OXP Handhelds from One-Netbook 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.
> >
> > 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
> > accommodate for that.
> >
> > Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> > ---
> > Removed fan_control reference in comment.
> > Bugfix MIX/MIN reporting not available
> > Bugfix pwm_enable register set wrong
> > ---
> > drivers/hwmon/Kconfig | 13 +-
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/oxp-sensors.c | 277
> > ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 290 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/hwmon/oxp-sensors.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 7ac3daaf59ce..a1cdb03b4d13 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
> >
> > source "drivers/hwmon/occ/Kconfig"
> >
> > +config SENSORS_OXP
> > + tristate "OneXPlayer EC fan control"
> > + depends on ACPI
> > + depends on X86
> > + help
> > + If you say yes here you get support for fan readings and
> > control over
> > + OneXPlayer handheld devices. Only OneXPlayer mini AMD
> > handheld variant
> > + boards are supported.
> > +
> > + Can also be built as a module. In that case it will be called oxp-
> > sensors.
> > +
> > config SENSORS_PCF8591
> > tristate "Philips PCF8591 ADC/DAC"
> > depends on I2C
> > @@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
> >
> > config SENSORS_AMC6821
> > tristate "Texas Instruments AMC6821"
> > - depends on I2C
> > + depends on I2C
> > help
> > If you say yes here you get support for the Texas Instruments
> > AMC6821 hardware monitoring chips.
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 11d076cad8a2..35824f8be455 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-
> > hwmon.o
> > obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> > obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> > obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
> > +obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
> > obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> > obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> > new file mode 100644
> > index 000000000000..f5895dc11094
> > --- /dev/null
> > +++ b/drivers/hwmon/oxp-sensors.c
> > @@ -0,0 +1,277 @@
> > +// 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
> > + * accommodate for that.
>
> What happens on the Intel variant with this code? Are they not the same EC?
> Why doesn't it work there? If you keep the AMD check in the code, I think it
> would be good to document the problems with the Intel one at least.
I don't own an intel board but a friend of mine does. It won't work.
The EC registers are different, even though they have the same DMI
strings for board manufacturer and board name. There is also a variant
for the board vendor that is programmed here "ONE-NETBOOK" and
"ONE-NETBOOK TECHNOLOGY CO., LTD." for the same device.
The explanation for the Intel issue is I couldn't figure out the EC
registers and values to read/write. I have a version of this on my
repo with a non-functional Intel case. I can document it in a code
comment over the amd cpu check.
> > + *
> > + * 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 <linux/processor.h>
> > +
> > +#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2
> > registers long */
> > +#define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1
> > register long */
> > +#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1
> > register long */
> > +
> > +static const struct dmi_system_id dmi_table[] = {
> > + {
> > + .matches = {
> > + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > + "ONE-NETBOOK TECHNOLOGY CO.,
> > LTD."),
> > + },
> > + },
> > + {
> > + .matches = {
> > + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > + "ONE-NETBOOK"),
> > + },
> > + },
> > + {},
> > +};
> > +
> > +struct oxp_status {
> > + struct lock_data lock_data;
> > +};
> > +
> > +/* Helper functions to handle EC read/write */
> > +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;
> > + *val += buffer;
> > + }
> > + return ret;
>
> Don't you need to acquire your mutex for reading too?
> Otherwise you could potentially have userspace trying to read
> and write another at the same time and get indeterminate results.
Will add since it doesn't seem to hurt. I added the mutex to the write
case only since it can indeed present an issue.
> > +}
> > +
> > +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> > +{
> > + struct oxp_status *state = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + 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;
> > +}
> > +
> > +static int oxp_pwm_enable(const struct device *dev)
> > +{
> > + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x01);
> > +}
> > +
> > +static int oxp_pwm_disable(const struct device *dev)
> > +{
> > + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x00);
> > +}
> > +
> > +/* 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 0444;
> > + case hwmon_pwm:
> > + return 0644;
> > + default:
> > + return 0;
> > + }
> > + return 0;
> > +}
> > +
> > +static int oxp_platform_read(struct device *dev, enum
> > hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + int ret;
> > +
> > + switch (type) {
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + return read_from_ec(OXP_SENSOR_FAN_REG,
> > + 2,
> > + val);
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d:
> > %d\n", type, attr);
> > + return -EOPNOTSUPP;
> > + }
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + ret = read_from_ec(OXP_SENSOR_PWM_REG,
> > + 2, val);
> > + *val = (*val * 255) / 100;
> > + return ret;
> > + case hwmon_pwm_enable:
> > + return
> > read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d:
> > %d\n", type, attr);
> > + return -EOPNOTSUPP;
> > + }
> > + default:
> > + dev_dbg(dev, "Unknown sensor type %d.\n", type);
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static int oxp_platform_write(struct device *dev, enum
> > hwmon_sensor_types type,
> > + u32 attr, int channel, long val)
> > +{
> > + switch (type) {
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_enable:
> > + if (val == 1)
> > + return oxp_pwm_enable(dev);
> > + else if (val == 0)
> > + return oxp_pwm_disable(dev);
> > + else
> > + return -EINVAL;
> > + case hwmon_pwm_input:
> > + if (val < 0 || val > 255)
> > + return -EINVAL;
> > + val = (val * 100) / 255;
> > + return write_to_ec(dev, OXP_SENSOR_PWM_REG,
> > val);
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d",
> > type, attr);
> > + return -EOPNOTSUPP;
> > + }
> > + default:
> > + dev_dbg(dev, "Unknown sensor type: %d", type);
> > + return -EOPNOTSUPP;
> > + }
> > + return -EINVAL;
>
> Can you actually hit this scenario? I would think not; you'll hit "default" and return -EOPNOTSUPP.
> Maybe it's better to just drop the default label and then outside the switch/case do this:
>
> dev_dbg(dev, "Unknown sensor type: %d", type);
> return -EOPNOTSUPP;
>
It shouldn't since there are no other attributes present. I can
simplify this logic by a catch all case as you stated.
> > +}
> > +
> > +/* Known sensors in the OXP EC controllers */
> > +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> > + HWMON_CHANNEL_INFO(fan,
> > + HWMON_F_INPUT),
> > + HWMON_CHANNEL_INFO(pwm,
> > + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> > + 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 int oxp_platform_probe(struct platform_device *pdev)
> > +{
> > + const struct dmi_system_id *dmi_entry;
> > + struct device *dev = &pdev->dev;
> > + struct device *hwdev;
> > + struct oxp_status *state;
> > +
> > + /* Have to check for AMD processor here */
> > + dmi_entry = dmi_first_match(dmi_table);
> > + if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> > + return -ENODEV;
>
> So it's shared DMI data values for the Intel and AMD variants of this platform? What
> happens if you run all this code on the Intel one?
>
Yeah... These devices have all the same DMI strings. In facto all the
OneXPlayers have the same DMI model name also "ONEXPLAYER".
Running this code on Intel won't work, the EC registers and values
seem to differ.
> > +
> > + state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> > + if (!state)
> > + return -ENOMEM;
> > +
> > + 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);
> > +}
> > +
> > +static struct platform_driver oxp_platform_driver = {
> > + .driver = {
> > + .name = "oxp-platform",
> > + },
> > + .probe = oxp_platform_probe,
> > +};
> > +
> > +static struct platform_device *oxp_platform_device;
> > +
> > +static int __init oxp_platform_init(void)
> > +{
> > + oxp_platform_device =
> > + platform_create_bundle(&oxp_platform_driver,
> > + oxp_platform_probe, NULL, 0, NULL, 0);
> > +
> > + if (IS_ERR(oxp_platform_device))
> > + return PTR_ERR(oxp_platform_device);
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit oxp_platform_exit(void)
> > +{
> > + platform_device_unregister(oxp_platform_device);
> > + platform_driver_unregister(&oxp_platform_driver);
> > +}
> > +
> > +MODULE_DEVICE_TABLE(dmi, dmi_table);
> > +module_init(oxp_platform_init);
> > +module_exit(oxp_platform_exit);
> > +
> > +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
--
Joaquín I. Aramendía
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 18:57 ` Joaquin Aramendia
@ 2022-10-31 19:21 ` Limonciello, Mario
2022-10-31 19:33 ` Joaquin Aramendia
0 siblings, 1 reply; 19+ messages in thread
From: Limonciello, Mario @ 2022-10-31 19:21 UTC (permalink / raw)
To: Joaquin Aramendia
Cc: pobrn, hdegoede, jdelvare, linux-hwmon, linux, markgross,
platform-driver-x86
[Public]
> -----Original Message-----
> From: Joaquin Aramendia <samsagax@gmail.com>
> Sent: Monday, October 31, 2022 13:58
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: pobrn@protonmail.com; hdegoede@redhat.com; jdelvare@suse.com;
> linux-hwmon@vger.kernel.org; linux@roeck-us.net; markgross@kernel.org;
> platform-driver-x86@vger.kernel.org
> Subject: Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver
>
> El lun, 31 oct 2022 a la(s) 13:43, Limonciello, Mario
> (Mario.Limonciello@amd.com) escribió:
> >
> > [Public]
> >
> >
> >
> > > -----Original Message-----
> > > From: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> > > Sent: Monday, October 31, 2022 09:53
> > > To: pobrn@protonmail.com
> > > Cc: hdegoede@redhat.com; jdelvare@suse.com; linux-
> > > hwmon@vger.kernel.org; linux@roeck-us.net; markgross@kernel.org;
> > > platform-driver-x86@vger.kernel.org; Joaquín Ignacio Aramendía
> > > <samsagax@gmail.com>
> > > Subject: [PATCH v3] Add OneXPlayer mini AMD sensors driver
> > >
> > > Sensors driver for OXP Handhelds from One-Netbook 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.
> > >
> > > 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
> > > accommodate for that.
> > >
> > > Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> > > ---
> > > Removed fan_control reference in comment.
> > > Bugfix MIX/MIN reporting not available
> > > Bugfix pwm_enable register set wrong
> > > ---
> > > drivers/hwmon/Kconfig | 13 +-
> > > drivers/hwmon/Makefile | 1 +
> > > drivers/hwmon/oxp-sensors.c | 277
> > > ++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 290 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/hwmon/oxp-sensors.c
> > >
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 7ac3daaf59ce..a1cdb03b4d13 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
> > >
> > > source "drivers/hwmon/occ/Kconfig"
> > >
> > > +config SENSORS_OXP
> > > + tristate "OneXPlayer EC fan control"
> > > + depends on ACPI
> > > + depends on X86
> > > + help
> > > + If you say yes here you get support for fan readings and
> > > control over
> > > + OneXPlayer handheld devices. Only OneXPlayer mini AMD
> > > handheld variant
> > > + boards are supported.
> > > +
> > > + Can also be built as a module. In that case it will be called oxp-
> > > sensors.
> > > +
> > > config SENSORS_PCF8591
> > > tristate "Philips PCF8591 ADC/DAC"
> > > depends on I2C
> > > @@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
> > >
> > > config SENSORS_AMC6821
> > > tristate "Texas Instruments AMC6821"
> > > - depends on I2C
> > > + depends on I2C
> > > help
> > > If you say yes here you get support for the Texas Instruments
> > > AMC6821 hardware monitoring chips.
> > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > index 11d076cad8a2..35824f8be455 100644
> > > --- a/drivers/hwmon/Makefile
> > > +++ b/drivers/hwmon/Makefile
> > > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-
> > > hwmon.o
> > > obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> > > obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> > > obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
> > > +obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
> > > obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> > > obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-
> sensors.c
> > > new file mode 100644
> > > index 000000000000..f5895dc11094
> > > --- /dev/null
> > > +++ b/drivers/hwmon/oxp-sensors.c
> > > @@ -0,0 +1,277 @@
> > > +// 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
> > > + * accommodate for that.
> >
> > What happens on the Intel variant with this code? Are they not the same
> EC?
> > Why doesn't it work there? If you keep the AMD check in the code, I think
> it
> > would be good to document the problems with the Intel one at least.
>
> I don't own an intel board but a friend of mine does. It won't work.
> The EC registers are different, even though they have the same DMI
> strings for board manufacturer and board name. There is also a variant
> for the board vendor that is programmed here "ONE-NETBOOK" and
> "ONE-NETBOOK TECHNOLOGY CO., LTD." for the same device.
> The explanation for the Intel issue is I couldn't figure out the EC
> registers and values to read/write. I have a version of this on my
> repo with a non-functional Intel case. I can document it in a code
> comment over the amd cpu check.
>
Have you tried comparing all of the DMI strings? I'm really surprised that
they would all be the same. My suspicion would be that at least the SKU
string should be different. If it is, you should be able to drop the CPU match
and use that instead.
> > > + *
> > > + * 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 <linux/processor.h>
> > > +
> > > +#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2
> > > registers long */
> > > +#define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1
> > > register long */
> > > +#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1
> > > register long */
> > > +
> > > +static const struct dmi_system_id dmi_table[] = {
> > > + {
> > > + .matches = {
> > > + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > > + "ONE-NETBOOK TECHNOLOGY CO.,
> > > LTD."),
> > > + },
> > > + },
> > > + {
> > > + .matches = {
> > > + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > > + "ONE-NETBOOK"),
> > > + },
> > > + },
> > > + {},
> > > +};
> > > +
> > > +struct oxp_status {
> > > + struct lock_data lock_data;
> > > +};
> > > +
> > > +/* Helper functions to handle EC read/write */
> > > +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;
> > > + *val += buffer;
> > > + }
> > > + return ret;
> >
> > Don't you need to acquire your mutex for reading too?
> > Otherwise you could potentially have userspace trying to read
> > and write another at the same time and get indeterminate results.
>
> Will add since it doesn't seem to hurt. I added the mutex to the write
> case only since it can indeed present an issue.
>
> > > +}
> > > +
> > > +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> > > +{
> > > + struct oxp_status *state = dev_get_drvdata(dev);
> > > + int ret;
> > > +
> > > + 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;
> > > +}
> > > +
> > > +static int oxp_pwm_enable(const struct device *dev)
> > > +{
> > > + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x01);
> > > +}
> > > +
> > > +static int oxp_pwm_disable(const struct device *dev)
> > > +{
> > > + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x00);
> > > +}
> > > +
> > > +/* 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 0444;
> > > + case hwmon_pwm:
> > > + return 0644;
> > > + default:
> > > + return 0;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +static int oxp_platform_read(struct device *dev, enum
> > > hwmon_sensor_types type,
> > > + u32 attr, int channel, long *val)
> > > +{
> > > + int ret;
> > > +
> > > + switch (type) {
> > > + case hwmon_fan:
> > > + switch (attr) {
> > > + case hwmon_fan_input:
> > > + return read_from_ec(OXP_SENSOR_FAN_REG,
> > > + 2,
> > > + val);
> > > + default:
> > > + dev_dbg(dev, "Unknown attribute for type %d:
> > > %d\n", type, attr);
> > > + return -EOPNOTSUPP;
> > > + }
> > > + case hwmon_pwm:
> > > + switch (attr) {
> > > + case hwmon_pwm_input:
> > > + ret = read_from_ec(OXP_SENSOR_PWM_REG,
> > > + 2, val);
> > > + *val = (*val * 255) / 100;
> > > + return ret;
> > > + case hwmon_pwm_enable:
> > > + return
> > > read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
> > > + default:
> > > + dev_dbg(dev, "Unknown attribute for type %d:
> > > %d\n", type, attr);
> > > + return -EOPNOTSUPP;
> > > + }
> > > + default:
> > > + dev_dbg(dev, "Unknown sensor type %d.\n", type);
> > > + return -EOPNOTSUPP;
> > > + }
> > > +}
> > > +
> > > +static int oxp_platform_write(struct device *dev, enum
> > > hwmon_sensor_types type,
> > > + u32 attr, int channel, long val)
> > > +{
> > > + switch (type) {
> > > + case hwmon_pwm:
> > > + switch (attr) {
> > > + case hwmon_pwm_enable:
> > > + if (val == 1)
> > > + return oxp_pwm_enable(dev);
> > > + else if (val == 0)
> > > + return oxp_pwm_disable(dev);
> > > + else
> > > + return -EINVAL;
> > > + case hwmon_pwm_input:
> > > + if (val < 0 || val > 255)
> > > + return -EINVAL;
> > > + val = (val * 100) / 255;
> > > + return write_to_ec(dev, OXP_SENSOR_PWM_REG,
> > > val);
> > > + default:
> > > + dev_dbg(dev, "Unknown attribute for type %d: %d",
> > > type, attr);
> > > + return -EOPNOTSUPP;
> > > + }
> > > + default:
> > > + dev_dbg(dev, "Unknown sensor type: %d", type);
> > > + return -EOPNOTSUPP;
> > > + }
> > > + return -EINVAL;
> >
> > Can you actually hit this scenario? I would think not; you'll hit "default" and
> return -EOPNOTSUPP.
> > Maybe it's better to just drop the default label and then outside the
> switch/case do this:
> >
> > dev_dbg(dev, "Unknown sensor type: %d", type);
> > return -EOPNOTSUPP;
> >
> It shouldn't since there are no other attributes present. I can
> simplify this logic by a catch all case as you stated.
>
> > > +}
> > > +
> > > +/* Known sensors in the OXP EC controllers */
> > > +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> > > + HWMON_CHANNEL_INFO(fan,
> > > + HWMON_F_INPUT),
> > > + HWMON_CHANNEL_INFO(pwm,
> > > + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> > > + 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 int oxp_platform_probe(struct platform_device *pdev)
> > > +{
> > > + const struct dmi_system_id *dmi_entry;
> > > + struct device *dev = &pdev->dev;
> > > + struct device *hwdev;
> > > + struct oxp_status *state;
> > > +
> > > + /* Have to check for AMD processor here */
> > > + dmi_entry = dmi_first_match(dmi_table);
> > > + if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> > > + return -ENODEV;
> >
> > So it's shared DMI data values for the Intel and AMD variants of this
> platform? What
> > happens if you run all this code on the Intel one?
> >
> Yeah... These devices have all the same DMI strings. In facto all the
> OneXPlayers have the same DMI model name also "ONEXPLAYER".
> Running this code on Intel won't work, the EC registers and values
> seem to differ.
>
> > > +
> > > + state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> > > + if (!state)
> > > + return -ENOMEM;
> > > +
> > > + 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);
> > > +}
> > > +
> > > +static struct platform_driver oxp_platform_driver = {
> > > + .driver = {
> > > + .name = "oxp-platform",
> > > + },
> > > + .probe = oxp_platform_probe,
> > > +};
> > > +
> > > +static struct platform_device *oxp_platform_device;
> > > +
> > > +static int __init oxp_platform_init(void)
> > > +{
> > > + oxp_platform_device =
> > > + platform_create_bundle(&oxp_platform_driver,
> > > + oxp_platform_probe, NULL, 0, NULL, 0);
> > > +
> > > + if (IS_ERR(oxp_platform_device))
> > > + return PTR_ERR(oxp_platform_device);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void __exit oxp_platform_exit(void)
> > > +{
> > > + platform_device_unregister(oxp_platform_device);
> > > + platform_driver_unregister(&oxp_platform_driver);
> > > +}
> > > +
> > > +MODULE_DEVICE_TABLE(dmi, dmi_table);
> > > +module_init(oxp_platform_init);
> > > +module_exit(oxp_platform_exit);
> > > +
> > > +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
>
>
>
> --
> Joaquín I. Aramendía
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 19:21 ` Limonciello, Mario
@ 2022-10-31 19:33 ` Joaquin Aramendia
0 siblings, 0 replies; 19+ messages in thread
From: Joaquin Aramendia @ 2022-10-31 19:33 UTC (permalink / raw)
To: Limonciello, Mario
Cc: pobrn, hdegoede, jdelvare, linux-hwmon, linux, markgross,
platform-driver-x86
Hi again, Mario. Sad but true
El lun, 31 oct 2022 a la(s) 16:21, Limonciello, Mario
(Mario.Limonciello@amd.com) escribió:
>
> Have you tried comparing all of the DMI strings? I'm really surprised that
> they would all be the same. My suspicion would be that at least the SKU
> string should be different. If it is, you should be able to drop the CPU match
> and use that instead.
I tried all DMI strings: Here is the SKU in particular:
# cat /sys/class/dmi/id/product_sku
ONE XPLAYER
Is the same as product_name and board_name. product_family is empty,
product version is "Default string", same as chassis_version and
board_asset_tag. Yeah. These devices are a mess... Hope the next ones
can be better and eventually they release a firmware upgrade to change
the strings.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 14:53 ` [PATCH v3] " Joaquín Ignacio Aramendía
2022-10-31 15:34 ` Guenter Roeck
2022-10-31 16:43 ` Limonciello, Mario
@ 2022-10-31 19:56 ` Guenter Roeck
2022-10-31 20:53 ` Joaquin Aramendia
2 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2022-10-31 19:56 UTC (permalink / raw)
To: Joaquín Ignacio Aramendía
Cc: pobrn, hdegoede, jdelvare, linux-hwmon, markgross, platform-driver-x86
On Mon, Oct 31, 2022 at 11:53:08AM -0300, Joaquín Ignacio Aramendía wrote:
> Sensors driver for OXP Handhelds from One-Netbook 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.
>
> 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
> accommodate for that.
>
> Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
Please run "checkpatch --strict" on your patch and fix the CHECK
messages. Also see Documentation/hwmon/submitting-patches.rst.
> ---
> Removed fan_control reference in comment.
> Bugfix MIX/MIN reporting not available
> Bugfix pwm_enable register set wrong
> ---
> drivers/hwmon/Kconfig | 13 +-
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/oxp-sensors.c | 277 ++++++++++++++++++++++++++++++++++++
Also needs Documentation/hwmon/oxp-sensors.rst
> 3 files changed, 290 insertions(+), 1 deletion(-)
> create mode 100644 drivers/hwmon/oxp-sensors.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7ac3daaf59ce..a1cdb03b4d13 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
>
> source "drivers/hwmon/occ/Kconfig"
>
> +config SENSORS_OXP
> + tristate "OneXPlayer EC fan control"
> + depends on ACPI
> + depends on X86
> + help
> + If you say yes here you get support for fan readings and control over
> + OneXPlayer handheld devices. Only OneXPlayer mini AMD handheld variant
> + boards are supported.
> +
> + Can also be built as a module. In that case it will be called oxp-sensors.
> +
> config SENSORS_PCF8591
> tristate "Philips PCF8591 ADC/DAC"
> depends on I2C
> @@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
>
> config SENSORS_AMC6821
> tristate "Texas Instruments AMC6821"
> - depends on I2C
> + depends on I2C
Please refrain from making unrelated changes. If you want to fix the extra
space, submit a separate patch.
> help
> If you say yes here you get support for the Texas Instruments
> AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 11d076cad8a2..35824f8be455 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o
> obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
> +obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
> obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> new file mode 100644
> index 000000000000..f5895dc11094
> --- /dev/null
> +++ b/drivers/hwmon/oxp-sensors.c
> @@ -0,0 +1,277 @@
> +// 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
> + * accommodate for that.
> + *
> + * 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 <linux/processor.h>
> +
> +#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */
> +#define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */
> +#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */
> +
> +static const struct dmi_system_id dmi_table[] = {
> + {
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> + "ONE-NETBOOK TECHNOLOGY CO., LTD."),
> + },
> + },
> + {
> + .matches = {
> + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> + "ONE-NETBOOK"),
Are there any others devices which start with "ONE-NETBOOK" but are not
supported ? If not a single entry with DMI_MATCH() woud be sufficient.
Either case I would like to see some additional entry or entries here.
Just matching on the vendor name seems risky. At the very least there
should also be a match for the "ONE XPLAYER" sku.
> + },
> + },
> + {},
> +};
> +
> +struct oxp_status {
> + struct lock_data lock_data;
> +};
> +
> +/* Helper functions to handle EC read/write */
> +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;
Unnecessary ( )
> + *val += buffer;
> + }
> + return ret;
> +}
> +
> +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> +{
> + struct oxp_status *state = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!state->lock_data.lock(&state->lock_data)) {
> + dev_warn(dev, "Failed to acquire mutex");
Is that message necessary ? If so it should be dev_err().
If it is expected, ie if acquiring the lock is observed
to fail sometimes, there should be no log message.
> + return -EBUSY;
> + }
> +
> + ret = ec_write(reg, value);
> +
> + if (!state->lock_data.unlock(&state->lock_data))
> + dev_err(dev, "Failed to release mutex");
No error return ? Then it is not an error and should not be
logged as one.
I am a bit concerned about those error messages. If they are seen
the errors are either sporadic and there should be no log,
or they are persistent and would clog the kernel log. If you think
that will indeed happen, is not normal operation, and that the
message is essential enough to be logged, please at least consider
using _once variants of the message to avoid clogging the kernel
log.
> +
> + return ret;
> +}
> +
> +static int oxp_pwm_enable(const struct device *dev)
> +{
> + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x01);
> +}
> +
> +static int oxp_pwm_disable(const struct device *dev)
> +{
> + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x00);
> +}
> +
> +/* 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 0444;
> + case hwmon_pwm:
> + return 0644;
> + default:
> + return 0;
> + }
> + return 0;
> +}
> +
> +static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + int ret;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + return read_from_ec(OXP_SENSOR_FAN_REG,
> + 2,
> + val);
Unnecessary continuation lines
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
> + return -EOPNOTSUPP;
> + }
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + ret = read_from_ec(OXP_SENSOR_PWM_REG,
> + 2, val);
Please, no unnecessary continuation lines, and make sure that continuation
lines match with '(' in the preceding line.
> + *val = (*val * 255) / 100;
> + return ret;
> + case hwmon_pwm_enable:
> + return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
> + return -EOPNOTSUPP;
> + }
> + default:
> + dev_dbg(dev, "Unknown sensor type %d.\n", type);
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + switch (type) {
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_enable:
> + if (val == 1)
> + return oxp_pwm_enable(dev);
> + else if (val == 0)
> + return oxp_pwm_disable(dev);
> + else
> + return -EINVAL;
> + case hwmon_pwm_input:
> + if (val < 0 || val > 255)
> + return -EINVAL;
> + val = (val * 100) / 255;
> + return write_to_ec(dev, OXP_SENSOR_PWM_REG, val);
> + default:
> + dev_dbg(dev, "Unknown attribute for type %d: %d", type, attr);
> + return -EOPNOTSUPP;
> + }
> + default:
> + dev_dbg(dev, "Unknown sensor type: %d", type);
> + return -EOPNOTSUPP;
> + }
> + return -EINVAL;
> +}
> +
> +/* Known sensors in the OXP EC controllers */
> +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> + HWMON_CHANNEL_INFO(fan,
> + HWMON_F_INPUT),
> + HWMON_CHANNEL_INFO(pwm,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> + 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 int oxp_platform_probe(struct platform_device *pdev)
> +{
> + const struct dmi_system_id *dmi_entry;
> + struct device *dev = &pdev->dev;
> + struct device *hwdev;
> + struct oxp_status *state;
> +
> + /* Have to check for AMD processor here */
> + dmi_entry = dmi_first_match(dmi_table);
> + if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> + return -ENODEV;
> +
> + state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> + if (!state)
> + return -ENOMEM;
> +
> + state->lock_data.mutex = 0;
Unnecessary initialization. The data structure is initialized with
devm_kzalloc().
> + state->lock_data.lock = lock_global_acpi_lock;
> + state->lock_data.unlock = unlock_global_acpi_lock;
What is the purpose of this callback function ?
Why not just call the lock and unlock functions directly ?
> +
> + hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", state,
> + &oxp_ec_chip_info, NULL);
> +
> + return PTR_ERR_OR_ZERO(hwdev);
> +}
> +
> +static struct platform_driver oxp_platform_driver = {
> + .driver = {
> + .name = "oxp-platform",
> + },
> + .probe = oxp_platform_probe,
> +};
> +
> +static struct platform_device *oxp_platform_device;
> +
> +static int __init oxp_platform_init(void)
> +{
> + oxp_platform_device =
> + platform_create_bundle(&oxp_platform_driver,
> + oxp_platform_probe, NULL, 0, NULL, 0);
> +
> + if (IS_ERR(oxp_platform_device))
> + return PTR_ERR(oxp_platform_device);
> +
> + return 0;
> +}
> +
> +static void __exit oxp_platform_exit(void)
> +{
> + platform_device_unregister(oxp_platform_device);
> + platform_driver_unregister(&oxp_platform_driver);
> +}
> +
> +MODULE_DEVICE_TABLE(dmi, dmi_table);
> +module_init(oxp_platform_init);
> +module_exit(oxp_platform_exit);
> +
> +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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 19:56 ` Guenter Roeck
@ 2022-10-31 20:53 ` Joaquin Aramendia
2022-10-31 21:30 ` Guenter Roeck
0 siblings, 1 reply; 19+ messages in thread
From: Joaquin Aramendia @ 2022-10-31 20:53 UTC (permalink / raw)
To: Guenter Roeck
Cc: pobrn, hdegoede, jdelvare, linux-hwmon, markgross, platform-driver-x86
Thanks for your time and review. I'll follow your advice and
corrections. Should I send the next patch version in a separate
thread? Or should I answer to this one?
El lun, 31 oct 2022 a la(s) 16:56, Guenter Roeck (linux@roeck-us.net) escribió:
>
> On Mon, Oct 31, 2022 at 11:53:08AM -0300, Joaquín Ignacio Aramendía wrote:
> > Sensors driver for OXP Handhelds from One-Netbook 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.
> >
> > 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
> > accommodate for that.
> >
> > Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
>
> Please run "checkpatch --strict" on your patch and fix the CHECK
> messages. Also see Documentation/hwmon/submitting-patches.rst.
There is a Warning about MAINTAINERS to be updated. Should I add
myself to it? If yes, Should it be under a new header?
> > ---
> > Removed fan_control reference in comment.
> > Bugfix MIX/MIN reporting not available
> > Bugfix pwm_enable register set wrong
> > ---
> > drivers/hwmon/Kconfig | 13 +-
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/oxp-sensors.c | 277 ++++++++++++++++++++++++++++++++++++
>
> Also needs Documentation/hwmon/oxp-sensors.rst
Will add.
> > 3 files changed, 290 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/hwmon/oxp-sensors.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 7ac3daaf59ce..a1cdb03b4d13 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
> >
> > source "drivers/hwmon/occ/Kconfig"
> >
> > +config SENSORS_OXP
> > + tristate "OneXPlayer EC fan control"
> > + depends on ACPI
> > + depends on X86
> > + help
> > + If you say yes here you get support for fan readings and control over
> > + OneXPlayer handheld devices. Only OneXPlayer mini AMD handheld variant
> > + boards are supported.
> > +
> > + Can also be built as a module. In that case it will be called oxp-sensors.
> > +
> > config SENSORS_PCF8591
> > tristate "Philips PCF8591 ADC/DAC"
> > depends on I2C
> > @@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
> >
> > config SENSORS_AMC6821
> > tristate "Texas Instruments AMC6821"
> > - depends on I2C
> > + depends on I2C
>
> Please refrain from making unrelated changes. If you want to fix the extra
> space, submit a separate patch.
Sorry this must have been vim removing trailing spaces. Will remove
this chunk from the patch.
> > help
> > If you say yes here you get support for the Texas Instruments
> > AMC6821 hardware monitoring chips.
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 11d076cad8a2..35824f8be455 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o
> > obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> > obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> > obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
> > +obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
> > obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> > obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> > new file mode 100644
> > index 000000000000..f5895dc11094
> > --- /dev/null
> > +++ b/drivers/hwmon/oxp-sensors.c
> > @@ -0,0 +1,277 @@
> > +// 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
> > + * accommodate for that.
> > + *
> > + * 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 <linux/processor.h>
> > +
> > +#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */
> > +#define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */
> > +#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */
> > +
> > +static const struct dmi_system_id dmi_table[] = {
> > + {
> > + .matches = {
> > + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > + "ONE-NETBOOK TECHNOLOGY CO., LTD."),
> > + },
> > + },
> > + {
> > + .matches = {
> > + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > + "ONE-NETBOOK"),
>
> Are there any others devices which start with "ONE-NETBOOK" but are not
> supported ? If not a single entry with DMI_MATCH() woud be sufficient.
> Either case I would like to see some additional entry or entries here.
> Just matching on the vendor name seems risky. At the very least there
> should also be a match for the "ONE XPLAYER" sku.
I would add a match for the board name instead of the sku if that is
ok. The rest will be added.
> > + },
> > + },
> > + {},
> > +};
> > +
> > +struct oxp_status {
> > + struct lock_data lock_data;
> > +};
> > +
> > +/* Helper functions to handle EC read/write */
> > +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;
>
> Unnecessary ( )
Will remove.
> > + *val += buffer;
> > + }
> > + return ret;
> > +}
> > +
> > +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> > +{
> > + struct oxp_status *state = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (!state->lock_data.lock(&state->lock_data)) {
> > + dev_warn(dev, "Failed to acquire mutex");
>
> Is that message necessary ? If so it should be dev_err().
> If it is expected, ie if acquiring the lock is observed
> to fail sometimes, there should be no log message.
The messages are there in case this fails, never failed on me,
honestly, but I've seen it in other ec-sensors drivers and adopted it
as a "good practice", I guess? Anyway I'll add a _once error message
and return error if it fails.
> > + return -EBUSY;
> > + }
> > +
> > + ret = ec_write(reg, value);
> > +
> > + if (!state->lock_data.unlock(&state->lock_data))
> > + dev_err(dev, "Failed to release mutex");
>
> No error return ? Then it is not an error and should not be
> logged as one.
>
> I am a bit concerned about those error messages. If they are seen
> the errors are either sporadic and there should be no log,
> or they are persistent and would clog the kernel log. If you think
> that will indeed happen, is not normal operation, and that the
> message is essential enough to be logged, please at least consider
> using _once variants of the message to avoid clogging the kernel
> log.
Never saw those errors in about a month I used this driver on my own
device. As said, I saw the practice in other drivers. I think the best
way is to check for it and return an error while reporting it with the
_once variant if that is ok.
> > +
> > + return ret;
> > +}
> > +
> > +static int oxp_pwm_enable(const struct device *dev)
> > +{
> > + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x01);
> > +}
> > +
> > +static int oxp_pwm_disable(const struct device *dev)
> > +{
> > + return write_to_ec(dev, OXP_SENSOR_PWM_ENABLE_REG, 0x00);
> > +}
> > +
> > +/* 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 0444;
> > + case hwmon_pwm:
> > + return 0644;
> > + default:
> > + return 0;
> > + }
> > + return 0;
> > +}
> > +
> > +static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + int ret;
> > +
> > + switch (type) {
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + return read_from_ec(OXP_SENSOR_FAN_REG,
> > + 2,
> > + val);
>
> Unnecessary continuation lines
Ok. Will correct.
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
> > + return -EOPNOTSUPP;
> > + }
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + ret = read_from_ec(OXP_SENSOR_PWM_REG,
> > + 2, val);
>
> Please, no unnecessary continuation lines, and make sure that continuation
> lines match with '(' in the preceding line.
Ok. Will correct.
> > + *val = (*val * 255) / 100;
> > + return ret;
> > + case hwmon_pwm_enable:
> > + return read_from_ec(OXP_SENSOR_PWM_ENABLE_REG, 1, val);
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d\n", type, attr);
> > + return -EOPNOTSUPP;
> > + }
> > + default:
> > + dev_dbg(dev, "Unknown sensor type %d.\n", type);
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long val)
> > +{
> > + switch (type) {
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_enable:
> > + if (val == 1)
> > + return oxp_pwm_enable(dev);
> > + else if (val == 0)
> > + return oxp_pwm_disable(dev);
> > + else
> > + return -EINVAL;
> > + case hwmon_pwm_input:
> > + if (val < 0 || val > 255)
> > + return -EINVAL;
> > + val = (val * 100) / 255;
> > + return write_to_ec(dev, OXP_SENSOR_PWM_REG, val);
> > + default:
> > + dev_dbg(dev, "Unknown attribute for type %d: %d", type, attr);
> > + return -EOPNOTSUPP;
> > + }
> > + default:
> > + dev_dbg(dev, "Unknown sensor type: %d", type);
> > + return -EOPNOTSUPP;
> > + }
> > + return -EINVAL;
> > +}
> > +
> > +/* Known sensors in the OXP EC controllers */
> > +static const struct hwmon_channel_info *oxp_platform_sensors[] = {
> > + HWMON_CHANNEL_INFO(fan,
> > + HWMON_F_INPUT),
> > + HWMON_CHANNEL_INFO(pwm,
> > + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> > + 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 int oxp_platform_probe(struct platform_device *pdev)
> > +{
> > + const struct dmi_system_id *dmi_entry;
> > + struct device *dev = &pdev->dev;
> > + struct device *hwdev;
> > + struct oxp_status *state;
> > +
> > + /* Have to check for AMD processor here */
> > + dmi_entry = dmi_first_match(dmi_table);
> > + if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> > + return -ENODEV;
> > +
> > + state = devm_kzalloc(dev, sizeof(struct oxp_status), GFP_KERNEL);
> > + if (!state)
> > + return -ENOMEM;
> > +
> > + state->lock_data.mutex = 0;
>
> Unnecessary initialization. The data structure is initialized with
> devm_kzalloc().
Ok. Will remove.
> > + state->lock_data.lock = lock_global_acpi_lock;
> > + state->lock_data.unlock = unlock_global_acpi_lock;
>
> What is the purpose of this callback function ?
> Why not just call the lock and unlock functions directly ?
Can be called directly and simplify the structure a little. Will change it.
> > +
> > + hwdev = devm_hwmon_device_register_with_info(dev, "oxpec", state,
> > + &oxp_ec_chip_info, NULL);
> > +
> > + return PTR_ERR_OR_ZERO(hwdev);
> > +}
> > +
> > +static struct platform_driver oxp_platform_driver = {
> > + .driver = {
> > + .name = "oxp-platform",
> > + },
> > + .probe = oxp_platform_probe,
> > +};
> > +
> > +static struct platform_device *oxp_platform_device;
> > +
> > +static int __init oxp_platform_init(void)
> > +{
> > + oxp_platform_device =
> > + platform_create_bundle(&oxp_platform_driver,
> > + oxp_platform_probe, NULL, 0, NULL, 0);
> > +
> > + if (IS_ERR(oxp_platform_device))
> > + return PTR_ERR(oxp_platform_device);
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit oxp_platform_exit(void)
> > +{
> > + platform_device_unregister(oxp_platform_device);
> > + platform_driver_unregister(&oxp_platform_driver);
> > +}
> > +
> > +MODULE_DEVICE_TABLE(dmi, dmi_table);
> > +module_init(oxp_platform_init);
> > +module_exit(oxp_platform_exit);
> > +
> > +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
> >
Thanks again :)
--
Joaquín I. Aramendía
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] Add OneXPlayer mini AMD sensors driver
2022-10-31 20:53 ` Joaquin Aramendia
@ 2022-10-31 21:30 ` Guenter Roeck
0 siblings, 0 replies; 19+ messages in thread
From: Guenter Roeck @ 2022-10-31 21:30 UTC (permalink / raw)
To: Joaquin Aramendia
Cc: pobrn, hdegoede, jdelvare, linux-hwmon, markgross, platform-driver-x86
On Mon, Oct 31, 2022 at 05:53:20PM -0300, Joaquin Aramendia wrote:
> Thanks for your time and review. I'll follow your advice and
> corrections. Should I send the next patch version in a separate
> thread? Or should I answer to this one?
>
> El lun, 31 oct 2022 a la(s) 16:56, Guenter Roeck (linux@roeck-us.net) escribió:
> >
> > On Mon, Oct 31, 2022 at 11:53:08AM -0300, Joaquín Ignacio Aramendía wrote:
> > > Sensors driver for OXP Handhelds from One-Netbook 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.
> > >
> > > 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
> > > accommodate for that.
> > >
> > > Signed-off-by: Joaquín Ignacio Aramendía <samsagax@gmail.com>
> >
> > Please run "checkpatch --strict" on your patch and fix the CHECK
> > messages. Also see Documentation/hwmon/submitting-patches.rst.
>
> There is a Warning about MAINTAINERS to be updated. Should I add
> myself to it? If yes, Should it be under a new header?
>
Feel free to do so if you like, but that one isn't mandatory.
> > > ---
> > > Removed fan_control reference in comment.
> > > Bugfix MIX/MIN reporting not available
> > > Bugfix pwm_enable register set wrong
> > > ---
> > > drivers/hwmon/Kconfig | 13 +-
> > > drivers/hwmon/Makefile | 1 +
> > > drivers/hwmon/oxp-sensors.c | 277 ++++++++++++++++++++++++++++++++++++
> >
> > Also needs Documentation/hwmon/oxp-sensors.rst
>
> Will add.
>
> > > 3 files changed, 290 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/hwmon/oxp-sensors.c
> > >
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 7ac3daaf59ce..a1cdb03b4d13 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -1607,6 +1607,17 @@ config SENSORS_NZXT_SMART2
> > >
> > > source "drivers/hwmon/occ/Kconfig"
> > >
> > > +config SENSORS_OXP
> > > + tristate "OneXPlayer EC fan control"
> > > + depends on ACPI
> > > + depends on X86
> > > + help
> > > + If you say yes here you get support for fan readings and control over
> > > + OneXPlayer handheld devices. Only OneXPlayer mini AMD handheld variant
> > > + boards are supported.
> > > +
> > > + Can also be built as a module. In that case it will be called oxp-sensors.
> > > +
> > > config SENSORS_PCF8591
> > > tristate "Philips PCF8591 ADC/DAC"
> > > depends on I2C
> > > @@ -1957,7 +1968,7 @@ config SENSORS_ADS7871
> > >
> > > config SENSORS_AMC6821
> > > tristate "Texas Instruments AMC6821"
> > > - depends on I2C
> > > + depends on I2C
> >
> > Please refrain from making unrelated changes. If you want to fix the extra
> > space, submit a separate patch.
>
> Sorry this must have been vim removing trailing spaces. Will remove
> this chunk from the patch.
>
> > > help
> > > If you say yes here you get support for the Texas Instruments
> > > AMC6821 hardware monitoring chips.
> > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > index 11d076cad8a2..35824f8be455 100644
> > > --- a/drivers/hwmon/Makefile
> > > +++ b/drivers/hwmon/Makefile
> > > @@ -167,6 +167,7 @@ obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o
> > > obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o
> > > obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> > > obj-$(CONFIG_SENSORS_NZXT_SMART2) += nzxt-smart2.o
> > > +obj-$(CONFIG_SENSORS_OXP) += oxp-sensors.o
> > > obj-$(CONFIG_SENSORS_PC87360) += pc87360.o
> > > obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c
> > > new file mode 100644
> > > index 000000000000..f5895dc11094
> > > --- /dev/null
> > > +++ b/drivers/hwmon/oxp-sensors.c
> > > @@ -0,0 +1,277 @@
> > > +// 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
> > > + * accommodate for that.
> > > + *
> > > + * 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 <linux/processor.h>
> > > +
> > > +#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 OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */
> > > +#define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */
> > > +#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */
> > > +
> > > +static const struct dmi_system_id dmi_table[] = {
> > > + {
> > > + .matches = {
> > > + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > > + "ONE-NETBOOK TECHNOLOGY CO., LTD."),
> > > + },
> > > + },
> > > + {
> > > + .matches = {
> > > + DMI_EXACT_MATCH(DMI_BOARD_VENDOR,
> > > + "ONE-NETBOOK"),
> >
> > Are there any others devices which start with "ONE-NETBOOK" but are not
> > supported ? If not a single entry with DMI_MATCH() woud be sufficient.
> > Either case I would like to see some additional entry or entries here.
> > Just matching on the vendor name seems risky. At the very least there
> > should also be a match for the "ONE XPLAYER" sku.
>
> I would add a match for the board name instead of the sku if that is
> ok. The rest will be added.
>
Sure, if it contains useful information.
> > > + },
> > > + },
> > > + {},
> > > +};
> > > +
> > > +struct oxp_status {
> > > + struct lock_data lock_data;
> > > +};
> > > +
> > > +/* Helper functions to handle EC read/write */
> > > +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;
> >
> > Unnecessary ( )
>
> Will remove.
>
> > > + *val += buffer;
> > > + }
> > > + return ret;
> > > +}
> > > +
> > > +static int write_to_ec(const struct device *dev, u8 reg, u8 value)
> > > +{
> > > + struct oxp_status *state = dev_get_drvdata(dev);
> > > + int ret;
> > > +
> > > + if (!state->lock_data.lock(&state->lock_data)) {
> > > + dev_warn(dev, "Failed to acquire mutex");
> >
> > Is that message necessary ? If so it should be dev_err().
> > If it is expected, ie if acquiring the lock is observed
> > to fail sometimes, there should be no log message.
>
> The messages are there in case this fails, never failed on me,
> honestly, but I've seen it in other ec-sensors drivers and adopted it
> as a "good practice", I guess? Anyway I'll add a _once error message
> and return error if it fails.
It is not a good practice, it is developers inisisting to add noise.
It makes me cringe each time I see it, but I often let it go because
arguing about it is not worth wasting my time.
>
> > > + return -EBUSY;
> > > + }
> > > +
> > > + ret = ec_write(reg, value);
> > > +
> > > + if (!state->lock_data.unlock(&state->lock_data))
> > > + dev_err(dev, "Failed to release mutex");
> >
> > No error return ? Then it is not an error and should not be
> > logged as one.
> >
> > I am a bit concerned about those error messages. If they are seen
> > the errors are either sporadic and there should be no log,
> > or they are persistent and would clog the kernel log. If you think
> > that will indeed happen, is not normal operation, and that the
> > message is essential enough to be logged, please at least consider
> > using _once variants of the message to avoid clogging the kernel
> > log.
>
> Never saw those errors in about a month I used this driver on my own
> device. As said, I saw the practice in other drivers. I think the best
> way is to check for it and return an error while reporting it with the
> _once variant if that is ok.
>
I'd prefer to not have a message at all, but then again it is not worth
wasting my time over it.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-10-31 21:30 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-29 22:50 [PATCH] Add OneXPlayer mini AMD board driver Joaquín Ignacio Aramendía
2022-10-29 23:30 ` Guenter Roeck
2022-10-30 0:29 ` Joaquin Aramendia
2022-10-30 3:24 ` Guenter Roeck
2022-10-30 20:32 ` [PATCH v2] Add OneXPlayer mini AMD sensors driver Joaquín Ignacio Aramendía
2022-10-31 0:03 ` Barnabás Pőcze
2022-10-31 14:49 ` Joaquin Aramendia
2022-10-31 14:53 ` [PATCH v3] " Joaquín Ignacio Aramendía
2022-10-31 15:34 ` Guenter Roeck
2022-10-31 15:57 ` Joaquin Aramendia
2022-10-31 16:43 ` Limonciello, Mario
2022-10-31 18:57 ` Joaquin Aramendia
2022-10-31 19:21 ` Limonciello, Mario
2022-10-31 19:33 ` Joaquin Aramendia
2022-10-31 19:56 ` Guenter Roeck
2022-10-31 20:53 ` Joaquin Aramendia
2022-10-31 21:30 ` Guenter Roeck
2022-10-31 11:45 ` [PATCH] Add OneXPlayer mini AMD board driver Joaquin Aramendia
2022-10-31 13:07 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.