| Message ID | 20221029225051.1171795-1-samsagax@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add OneXPlayer mini AMD board driver | expand |
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 >
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.
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
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
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
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");