| Message ID | 20221031145308.341776-1-samsagax@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] Add OneXPlayer mini AMD sensors driver | expand |
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 >
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.
[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
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
[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
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.
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 >
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 :)
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
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");
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 -- 2.38.1