* [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
@ 2020-07-14 10:03 jaap aarts
2020-07-14 23:01 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: jaap aarts @ 2020-07-14 10:03 UTC (permalink / raw)
To: Delvare, Guenter Roeck, linux-hwmon, linux-usb; +Cc: jaap aarts
Adds fan/pwm support for H1000i platinum.
Custom temp/fan curves are not supported, however
the presets found in the proprietary drivers are avaiable.
Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
---
drivers/hwmon/Kconfig | 6 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/asetek_gen6.c | 801 ++++++++++++++++++++++++++++++++++++
3 files changed, 808 insertions(+)
create mode 100644 drivers/hwmon/asetek_gen6.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 288ae9f63588..521a9e0c88ca 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -378,6 +378,12 @@ config SENSORS_ARM_SCPI
and power sensors available on ARM Ltd's SCP based platforms. The
actual number and type of sensors exported depend on the platform.
+config SENSORS_ASETEK_GEN6
+ tristate "Asetek generation 6 driver"
+ help
+ If you say yes here you get support for asetek generation 6 boards
+ found on most AIO liquid coolers with an asetek pump.
+
config SENSORS_ASB100
tristate "Asus ASB100 Bach"
depends on X86 && I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3e32c21f5efe..9683d2aae2b2 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_W83793) += w83793.o
obj-$(CONFIG_SENSORS_W83795) += w83795.o
obj-$(CONFIG_SENSORS_W83781D) += w83781d.o
obj-$(CONFIG_SENSORS_W83791D) += w83791d.o
+obj-$(CONFIG_SENSORS_ASETEK_GEN6) += asetek_gen6.o
obj-$(CONFIG_SENSORS_AB8500) += abx500.o ab8500.o
obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o
diff --git a/drivers/hwmon/asetek_gen6.c b/drivers/hwmon/asetek_gen6.c
new file mode 100644
index 000000000000..4aea9ae0b944
--- /dev/null
+++ b/drivers/hwmon/asetek_gen6.c
@@ -0,0 +1,801 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * A hwmon driver for most asetek gen 6 all-in-one liquid coolers.
+ * Copyright (c) Jaap Aarts 2020
+ *
+ * Protocol reverse engineered by audiohacked
+ * https://github.com/audiohacked/OpendriverLink
+ */
+
+/*
+ * Supports following chips:
+ * h100i platinum
+ *
+ * Other products should work with this driver but no testing has been done.
+ *
+ * Note: platinum is the codename name for pro within driver, so h100i platinum = h1ooi pro
+ *
+ * Note: fan curve control has not been implemented
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/usb.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/errno.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+struct driver {
+ struct usb_device *udev;
+
+ char *bulk_out_buffer;
+ char *bulk_in_buffer;
+ size_t bulk_out_size;
+ size_t bulk_in_size;
+ char bulk_in_endpointAddr;
+ char bulk_out_endpointAddr;
+
+ struct usb_interface *interface; /* the interface for this device */
+ struct mutex io_mutex; /* synchronize I/O with disconnect */
+ struct semaphore
+ limit_sem; /* limiting the number of writes in progress */
+
+ struct kref kref;
+};
+
+struct curve_point {
+ uint8_t temp;
+ uint8_t pwm;
+};
+
+struct hwmon_fan_data {
+ char fan_channel;
+ long fan_target;
+ unsigned char fan_pwm_target;
+ long mode;
+ struct curve_point curve[7];
+};
+
+struct hwmon_data {
+ struct driver *dev;
+ int channel_count;
+ void **channel_data;
+};
+
+struct curve_point quiet_curve[] = {
+ {
+ .temp = 0x1F,
+ .pwm = 0x15,
+ },
+ {
+ .temp = 0x21,
+ .pwm = 0x1E,
+ },
+ {
+ .temp = 0x24,
+ .pwm = 0x25,
+ },
+ {
+ .temp = 0x27,
+ .pwm = 0x2D,
+ },
+ {
+ .temp = 0x29,
+ .pwm = 0x38,
+ },
+ {
+ .temp = 0x2C,
+ .pwm = 0x4A,
+ },
+ {
+ .temp = 0x2F,
+ .pwm = 0x64,
+ },
+};
+
+struct curve_point balanced_curve[] = {
+ {
+ .temp = 0x1C,
+ .pwm = 0x15,
+ },
+ {
+ .temp = 0x1E,
+ .pwm = 0x1B,
+ },
+ {
+ .temp = 0x20,
+ .pwm = 0x23,
+ },
+ {
+ .temp = 0x22,
+ .pwm = 0x28,
+ },
+ {
+ .temp = 0x24,
+ .pwm = 0x32,
+ },
+ {
+ .temp = 0x27,
+ .pwm = 0x48,
+ },
+ {
+ .temp = 0x29,
+ .pwm = 0x64,
+ },
+};
+
+struct curve_point extreme_curve[] = {
+ {
+ .temp = 0x19,
+ .pwm = 0x28,
+ },
+ {
+ .temp = 0x1B,
+ .pwm = 0x2E,
+ },
+ {
+ .temp = 0x1D,
+ .pwm = 0x37,
+ },
+ {
+ .temp = 0x1E,
+ .pwm = 0x41,
+ },
+ {
+ .temp = 0x1F,
+ .pwm = 0x4C,
+ },
+ {
+ .temp = 0x20,
+ .pwm = 0x56,
+ },
+ {
+ .temp = 0x21,
+ .pwm = 0x64,
+ },
+};
+
+#define default_curve quiet_curve
+
+static const char SUCCESS[2] = { 0x12, 0x34 };
+
+#define SUCCES_LENGTH 3
+
+static bool check_succes(char command, char ret[SUCCES_LENGTH])
+{
+ char success[SUCCES_LENGTH] = { command };
+
+ strncpy(&success[1], SUCCESS, SUCCES_LENGTH - 1);
+ return strncmp(ret, success, SUCCES_LENGTH - 1) == 0;
+}
+
+int set_fan_rpm_curve(struct driver *cdev, struct hwmon_fan_data *fan_data,
+ struct curve_point point[7])
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
+ char *send_buf = cdev->bulk_out_buffer;
+ char *recv_buf = cdev->bulk_in_buffer;
+
+ memcpy(fan_data->curve, point, sizeof(fan_data->curve));
+
+ send_buf[0] = 0x40;
+ send_buf[1] = fan_data->fan_channel;
+ send_buf[2] = point[0].temp;
+ send_buf[3] = point[1].temp;
+ send_buf[4] = point[2].temp;
+ send_buf[5] = point[3].temp;
+ send_buf[6] = point[4].temp;
+ send_buf[7] = point[5].temp;
+ send_buf[8] = point[6].temp;
+ send_buf[9] = point[0].pwm;
+ send_buf[10] = point[1].pwm;
+ send_buf[11] = point[2].pwm;
+ send_buf[12] = point[3].pwm;
+ send_buf[13] = point[4].pwm;
+ send_buf[14] = point[5].pwm;
+ send_buf[15] = point[6].pwm;
+
+ retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 16, &wrote, 100);
+ if (retval != 0)
+ return retval;
+
+ retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
+ if (retval != 0)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf))
+ dev_info("[*] Failled setting fan curve %d,%d,%d/%d\n",
+ recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
+ return 0;
+}
+
+int set_fan_target_rpm(struct driver *cdev, struct hwmon_fan_data *fan_data,
+ long val)
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
+
+ char *send_buf = cdev->bulk_out_buffer;
+ char *recv_buf = cdev->bulk_in_buffer;
+
+ fan_data->fan_target = val;
+ fan_data->fan_pwm_target = 0;
+
+ send_buf[0] = 0x43;
+ send_buf[1] = fan_data->fan_channel;
+ send_buf[2] = (fan_data->fan_target >> 8);
+ send_buf[3] = fan_data->fan_target;
+
+ retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
+ if (retval != 0)
+ return retval;
+
+ retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
+ if (retval != 0)
+ return retval;
+
+ //no error
+ if (!check_succes(send_buf[0], recv_buf))
+ dev_info("[*] Failled setting fan rpm %d,%d,%d/%d\n",
+ recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
+ return 0;
+}
+
+void get_fan_target_rpm(struct hwmon_fan_data *fan_data, long *val)
+{
+ *val = fan_data->fan_target;
+}
+
+int get_fan_current_rpm(struct driver *cdev, struct hwmon_fan_data *fan_data,
+ long *val)
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
+
+ char *send_buf = cdev->bulk_out_buffer;
+ char *recv_buf = cdev->bulk_in_buffer;
+
+ send_buf[0] = 0x41;
+ send_buf[1] = fan_data->fan_channel;
+
+ retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 2, &wrote, 100);
+ if (retval != 0)
+ return retval;
+
+ retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
+ if (retval != 0)
+ return retval;
+
+ if (!check_succes(0x41, recv_buf) ||
+ recv_buf[3] != fan_data->fan_channel)
+ dev_info("[*] Failled retrieving fan rmp %d,%d,%d/%d\n",
+ recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
+
+ *val = (((uint8_t)recv_buf[4]) << 8) + (uint8_t)recv_buf[5];
+ return 0;
+}
+
+int set_fan_target_pwm(struct driver *cdev, struct hwmon_fan_data *fan_data,
+ long val)
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
+
+ unsigned char *send_buf = cdev->bulk_out_buffer;
+ unsigned char *recv_buf = cdev->bulk_in_buffer;
+
+ fan_data->fan_pwm_target = val;
+ fan_data->fan_target = 0;
+
+ send_buf[0] = 0x42;
+ send_buf[1] = fan_data->fan_channel;
+ send_buf[3] = fan_data->fan_pwm_target;
+
+ retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
+ if (retval != 0)
+ return retval;
+
+ retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
+ if (retval != 0)
+ return retval;
+
+ //no error
+ if (!check_succes(send_buf[0], recv_buf))
+ dev_info("[*] Failled setting fan pwm %d,%d,%d/%d\n",
+ recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
+ return 0;
+}
+
+umode_t is_visible_func(const void *d, enum hwmon_sensor_types type, u32 attr,
+ int channel)
+{
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ return 0444;
+ break;
+ case hwmon_fan_target:
+ return 0644;
+ break;
+ case hwmon_fan_min:
+ return 0444;
+ break;
+ default:
+ break;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ return 0200;
+ break;
+ case hwmon_pwm_mode:
+ return 0644;
+ break;
+ default:
+ break;
+ }
+ break;
+
+ default:
+ break;
+ }
+ return 0;
+}
+
+static int write_func(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ struct hwmon_data *data = dev_get_drvdata(dev);
+ struct driver *cdev = data->dev;
+ struct hwmon_fan_data *fan_data;
+ int retval = 0;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_target:
+ fan_data = data->channel_data[channel];
+ if (fan_data->mode != 1)
+ return -EINVAL;
+
+ retval = usb_autopm_get_interface(cdev->interface);
+ if (retval)
+ goto exit;
+
+ if (down_trylock(&cdev->limit_sem)) {
+ retval = -EAGAIN;
+ goto cleanup_interface;
+ }
+
+ retval = set_fan_target_rpm(cdev, fan_data, val);
+ if (retval)
+ goto cleanup;
+
+ goto exit;
+ default:
+ return -EINVAL;
+ }
+ goto exit;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ fan_data = data->channel_data[channel];
+ if (fan_data->mode != 1)
+ return -EINVAL;
+
+ retval = usb_autopm_get_interface(cdev->interface);
+ if (retval)
+ goto exit;
+
+ if (down_trylock(&cdev->limit_sem)) {
+ retval = -EAGAIN;
+ goto cleanup_interface;
+ }
+
+ retval = set_fan_target_pwm(cdev, fan_data, val);
+ if (retval)
+ return retval;
+
+ goto cleanup;
+ case hwmon_pwm_enable:
+ fan_data = data->channel_data[channel];
+
+ retval = usb_autopm_get_interface(cdev->interface);
+ if (retval)
+ goto exit;
+
+ if (down_trylock(&cdev->limit_sem)) {
+ retval = -EAGAIN;
+ goto cleanup_interface;
+ }
+ fan_data->mode = val;
+
+ switch (val) {
+ case 0:
+ set_fan_rpm_curve(cdev, fan_data,
+ default_curve);
+ break;
+ case 1:
+ if (fan_data->fan_target != 0) {
+ retval = set_fan_target_rpm(
+ cdev, fan_data,
+ fan_data->fan_target);
+ if (retval)
+ goto cleanup;
+ } else if (fan_data->fan_pwm_target != 0) {
+ retval = set_fan_target_pwm(
+ cdev, fan_data,
+ fan_data->fan_pwm_target);
+ if (retval)
+ goto cleanup;
+ }
+ break;
+ case 2:
+ set_fan_rpm_curve(cdev, fan_data, quiet_curve);
+ break;
+ case 3:
+ set_fan_rpm_curve(cdev, fan_data,
+ balanced_curve);
+ break;
+ case 4:
+ set_fan_rpm_curve(cdev, fan_data,
+ extreme_curve);
+ break;
+ }
+ goto cleanup;
+ default:
+ return -EINVAL;
+ }
+ goto exit;
+ default:
+ return -EINVAL;
+ }
+
+cleanup:
+ up(&cdev->limit_sem);
+cleanup_interface:
+ usb_autopm_put_interface(cdev->interface);
+exit:
+ return retval;
+}
+
+int read_func(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct hwmon_data *data = dev_get_drvdata(dev);
+ struct driver *cdev = data->dev;
+ struct hwmon_fan_data *fan_data;
+ int retval = 0;
+
+ if (channel >= data->channel_count)
+ return -EAGAIN;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ fan_data = data->channel_data[channel];
+
+ retval = usb_autopm_get_interface(cdev->interface);
+ if (retval)
+ goto exit;
+
+ if (down_trylock(&cdev->limit_sem)) {
+ retval = -EAGAIN;
+ goto cleanup_interface;
+ }
+
+ retval = get_fan_current_rpm(cdev, fan_data, val);
+ if (retval)
+ goto cleanup;
+
+ goto cleanup;
+ case hwmon_fan_target:
+ fan_data = data->channel_data[channel];
+ if (fan_data->mode != 1) {
+ *val = 0;
+ goto exit;
+ }
+
+ get_fan_target_rpm(fan_data, val);
+ goto exit;
+ case hwmon_fan_min:
+ *val = 200;
+ goto exit;
+
+ default:
+ return -EINVAL;
+ }
+ goto exit;
+
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_mode:
+ fan_data = data->channel_data[channel];
+ *val = fan_data->mode;
+ goto exit;
+ default:
+ return -EINVAL;
+ }
+ goto exit;
+
+ default:
+ return -EINVAL;
+ }
+
+cleanup:
+ up(&cdev->limit_sem);
+cleanup_interface:
+ usb_autopm_put_interface(cdev->interface);
+exit:
+ return retval;
+}
+
+#define fan_config (HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN)
+#define pwm_config (HWMON_PWM_INPUT | HWMON_PWM_ENABLE)
+
+static const struct hwmon_ops asetek_6_ops = {
+ .is_visible = is_visible_func,
+ .read = read_func,
+ .write = write_func,
+};
+
+bool does_fan_exist(struct driver *cdev, int channel)
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
+
+ char *send_buf = cdev->bulk_out_buffer;
+ char *recv_buf = cdev->bulk_in_buffer;
+
+ send_buf[0] = 0x43;
+ send_buf[1] = channel;
+ send_buf[2] = (600 >> 8);
+ send_buf[3] = 600;
+
+ retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
+ if (retval != 0)
+ return false;
+
+ retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
+ if (retval != 0)
+ return false;
+
+ if (!check_succes(send_buf[0], recv_buf))
+ return false;
+ return true;
+}
+
+int get_fan_count(struct driver *dev)
+{
+ int fan;
+
+ for (fan = 0; does_fan_exist(dev, fan); fan += 1)
+ ;
+ return fan;
+}
+
+void hwmon_init(struct driver *dev)
+{
+ int fan_id;
+ struct device *hwmon_dev;
+ struct hwmon_fan_data *fan;
+ struct hwmon_data *data = devm_kzalloc(
+ &dev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
+ struct hwmon_chip_info *hwmon_info = devm_kzalloc(
+ &dev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
+ //Allocate the info table
+ struct hwmon_channel_info **aio_info =
+ devm_kzalloc(&dev->udev->dev,
+ sizeof(struct hwmon_channel_info *) * 2,
+ GFP_KERNEL); //2 for each channel info.
+
+ //Allocate the fan and PWM configuration
+ u32 *fans_config = devm_kzalloc(&dev->udev->dev,
+ sizeof(u32) * (data->channel_count + 1),
+ GFP_KERNEL);
+ u32 *pwms_config = devm_kzalloc(&dev->udev->dev,
+ sizeof(u32) * (data->channel_count + 1),
+ GFP_KERNEL);
+
+ data->channel_count = get_fan_count(dev); //amount of fans
+ data->channel_data =
+ devm_kzalloc(&dev->udev->dev,
+ sizeof(char *) * data->channel_count, GFP_KERNEL);
+
+ //For each fan create a data channel a fan config entry and a pwm config entry
+ for (fan_id = 0; fan_id <= data->channel_count; fan_id++) {
+ fan = devm_kzalloc(&dev->udev->dev,
+ sizeof(struct hwmon_fan_data), GFP_KERNEL);
+ fan->fan_channel = fan_id;
+ fan->mode = 2;
+ data->channel_data[fan_id] = fan;
+ fans_config[fan_id] = fan_config;
+ pwms_config[fan_id] = pwm_config;
+ }
+ fans_config[data->channel_count] = 0;
+ pwms_config[data->channel_count] = 0;
+
+ aio_info[0] = devm_kzalloc(
+ &dev->udev->dev, sizeof(struct hwmon_channel_info), GFP_KERNEL);
+ aio_info[0]->type = hwmon_fan;
+ aio_info[0]->config = fans_config;
+
+ aio_info[1] = devm_kzalloc(
+ &dev->udev->dev, sizeof(struct hwmon_channel_info), GFP_KERNEL);
+ aio_info[1]->type = hwmon_pwm;
+ aio_info[1]->config = pwms_config;
+
+ hwmon_info->ops = &asetek_6_ops;
+ hwmon_info->info = (const struct hwmon_channel_info **)aio_info;
+
+ data->dev = dev;
+ hwmon_dev = devm_hwmon_device_register_with_info(
+ &dev->udev->dev, "driver_fan", data, hwmon_info, NULL);
+ dev_info("[*] Setup hwmon\n");
+}
+
+void hwmon_deinit(struct driver *dev)
+{
+ hwmon_device_unregister(&dev->udev->dev);
+ dev_info("[*] HWMON DISCONNECT\n");
+}
+
+/*
+ * Devices that work with this driver.
+ * More devices should work, however none have been tested.
+ */
+static const struct usb_device_id astk_table[] = {
+ { USB_DEVICE(0x1b1c, 0x0c15) },
+ {},
+};
+
+MODULE_DEVICE_TABLE(usb, astk_table);
+
+int init_device(struct usb_device *udev)
+{
+ int retval;
+
+ retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
+ 0xffff, 0x0000, 0, 0, 0);
+ //this always returns error
+ if (retval != 0)
+ ;
+
+ retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
+ 0x0002, 0x0000, 0, 0, 0);
+ if (retval != 0)
+ return retval;
+
+ return 0;
+}
+
+int deinit_device(struct usb_device *udev)
+{
+ int retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
+ 0x0004, 0x0000, 0, 0, 0);
+ if (retval != 0)
+ return retval;
+
+ return 0;
+}
+
+static void astk_delete(struct kref *kref)
+{
+ struct driver *dev = container_of(kref, struct driver, kref);
+
+ usb_put_intf(dev->interface);
+ usb_put_dev(dev->udev);
+ kfree(dev->bulk_in_buffer);
+ kfree(dev->bulk_out_buffer);
+ kfree(dev);
+}
+
+static int astk_probe(struct usb_interface *interface,
+ const struct usb_device_id *id)
+{
+ struct driver *dev;
+ int retval = 0;
+ struct usb_endpoint_descriptor *bulk_in, *bulk_out;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ retval = -ENOMEM;
+ goto exit;
+ }
+
+ retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
+ &bulk_out, NULL, NULL);
+ if (retval != 0)
+ goto exit;
+
+ dev->udev = usb_get_dev(interface_to_usbdev(interface));
+ dev->interface = usb_get_intf(interface);
+
+ /* set up the endpoint information */
+ /* use only the first bulk-in and bulk-out endpoints */
+ dev->bulk_in_size = usb_endpoint_maxp(bulk_in);
+ dev->bulk_in_buffer = kmalloc(dev->bulk_in_size, GFP_KERNEL);
+ dev->bulk_in_endpointAddr = bulk_in->bEndpointAddress;
+ dev->bulk_out_size = usb_endpoint_maxp(bulk_out);
+ dev->bulk_out_buffer = kmalloc(dev->bulk_out_size, GFP_KERNEL);
+ dev->bulk_out_endpointAddr = bulk_out->bEndpointAddress;
+
+ retval = init_device(dev->udev);
+ if (retval) {
+ dev_err(&interface->dev, "Failled initialising this device.\n");
+ goto exit;
+ }
+
+ hwmon_init(dev);
+
+ usb_set_intfdata(interface, dev);
+ kref_init(&dev->kref);
+ mutex_init(&dev->io_mutex);
+ sema_init(&dev->limit_sem, 8);
+exit:
+ return retval;
+}
+
+static void astk_disconnect(struct usb_interface *interface)
+{
+ struct driver *dev = usb_get_intfdata(interface);
+
+ hwmon_deinit(dev);
+ usb_set_intfdata(interface, NULL);
+ kref_put(&dev->kref, astk_delete);
+ deinit_device(dev->udev);
+}
+static int astk_resume(struct usb_interface *intf)
+{
+ return 0;
+}
+
+static int astk_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ return 0;
+}
+
+static struct usb_driver astk_driver = {
+ .name = "Asetek gen6 driver",
+ .id_table = astk_table,
+ .probe = astk_probe,
+ .disconnect = astk_disconnect,
+ .resume = astk_resume,
+ .suspend = astk_suspend,
+ .supports_autosuspend = 1,
+};
+
+static int __init astk_init(void)
+{
+ int ret = usb_register(&astk_driver);
+
+ return ret;
+}
+
+static void __exit astk_exit(void)
+{
+ usb_deregister(&astk_driver);
+}
+
+module_init(astk_init);
+module_exit(astk_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jaap Aarts <jaap.aarts1@gmail.com>");
+MODULE_DESCRIPTION("Asetek gen6 driver");
--
2.27.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-14 10:03 [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
@ 2020-07-14 23:01 ` kernel test robot
2020-07-15 3:07 ` Guenter Roeck
2020-07-17 12:20 ` jaap aarts
2 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2020-07-14 23:01 UTC (permalink / raw)
To: jaap aarts, Delvare, Guenter Roeck, linux-hwmon, linux-usb
Cc: kbuild-all, jaap aarts
[-- Attachment #1: Type: text/plain, Size: 20908 bytes --]
Hi jaap,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v5.8-rc5 next-20200714]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/jaap-aarts/hwmon-add-fan-pwm-driver-for-corsair-h100i-platinum/20200714-180650
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
In file included from arch/m68k/include/asm/io_mm.h:25,
from arch/m68k/include/asm/io.h:8,
from include/linux/io.h:13,
from include/linux/irq.h:20,
from include/asm-generic/hardirq.h:13,
from ./arch/m68k/include/generated/asm/hardirq.h:1,
from include/linux/hardirq.h:10,
from include/linux/interrupt.h:11,
from include/linux/usb.h:16,
from drivers/hwmon/asetek_gen6.c:23:
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsb':
arch/m68k/include/asm/raw_io.h:83:7: warning: variable '__w' set but not used [-Wunused-but-set-variable]
83 | ({u8 __w, __v = (b); u32 _addr = ((u32) (addr)); \
| ^~~
arch/m68k/include/asm/raw_io.h:430:3: note: in expansion of macro 'rom_out_8'
430 | rom_out_8(port, *buf++);
| ^~~~~~~~~
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw':
arch/m68k/include/asm/raw_io.h:86:8: warning: variable '__w' set but not used [-Wunused-but-set-variable]
86 | ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
| ^~~
arch/m68k/include/asm/raw_io.h:448:3: note: in expansion of macro 'rom_out_be16'
448 | rom_out_be16(port, *buf++);
| ^~~~~~~~~~~~
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw_swapw':
arch/m68k/include/asm/raw_io.h:90:8: warning: variable '__w' set but not used [-Wunused-but-set-variable]
90 | ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
| ^~~
arch/m68k/include/asm/raw_io.h:466:3: note: in expansion of macro 'rom_out_le16'
466 | rom_out_le16(port, *buf++);
| ^~~~~~~~~~~~
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:174:5: warning: no previous prototype for 'set_fan_rpm_curve' [-Wmissing-prototypes]
174 | int set_fan_rpm_curve(struct driver *cdev, struct hwmon_fan_data *fan_data,
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:15,
from include/linux/usb/ch9.h:36,
from include/linux/usb.h:6,
from drivers/hwmon/asetek_gen6.c:23:
drivers/hwmon/asetek_gen6.c: In function 'set_fan_rpm_curve':
>> drivers/hwmon/asetek_gen6.c:212:12: error: passing argument 1 of '_dev_info' from incompatible pointer type [-Werror=incompatible-pointer-types]
212 | dev_info("[*] Failled setting fan curve %d,%d,%d/%d\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| char *
include/linux/dev_printk.h:110:12: note: in definition of macro 'dev_info'
110 | _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:48:37: note: expected 'const struct device *' but argument is of type 'char *'
48 | void _dev_info(const struct device *dev, const char *fmt, ...);
| ~~~~~~~~~~~~~~~~~~~~~^~~
>> drivers/hwmon/asetek_gen6.c:213:13: warning: passing argument 2 of '_dev_info' makes pointer from integer without a cast [-Wint-conversion]
213 | recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
| ~~~~~~~~^~~
| |
| char
include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
19 | #define dev_fmt(fmt) fmt
| ^~~
drivers/hwmon/asetek_gen6.c:212:3: note: in expansion of macro 'dev_info'
212 | dev_info("[*] Failled setting fan curve %d,%d,%d/%d\n",
| ^~~~~~~~
include/linux/dev_printk.h:48:54: note: expected 'const char *' but argument is of type 'char'
48 | void _dev_info(const struct device *dev, const char *fmt, ...);
| ~~~~~~~~~~~~^~~
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:217:5: warning: no previous prototype for 'set_fan_target_rpm' [-Wmissing-prototypes]
217 | int set_fan_target_rpm(struct driver *cdev, struct hwmon_fan_data *fan_data,
| ^~~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:15,
from include/linux/usb/ch9.h:36,
from include/linux/usb.h:6,
from drivers/hwmon/asetek_gen6.c:23:
drivers/hwmon/asetek_gen6.c: In function 'set_fan_target_rpm':
drivers/hwmon/asetek_gen6.c:246:12: error: passing argument 1 of '_dev_info' from incompatible pointer type [-Werror=incompatible-pointer-types]
246 | dev_info("[*] Failled setting fan rpm %d,%d,%d/%d\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| char *
include/linux/dev_printk.h:110:12: note: in definition of macro 'dev_info'
110 | _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:48:37: note: expected 'const struct device *' but argument is of type 'char *'
48 | void _dev_info(const struct device *dev, const char *fmt, ...);
| ~~~~~~~~~~~~~~~~~~~~~^~~
drivers/hwmon/asetek_gen6.c:247:13: warning: passing argument 2 of '_dev_info' makes pointer from integer without a cast [-Wint-conversion]
247 | recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
| ~~~~~~~~^~~
| |
| char
include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
19 | #define dev_fmt(fmt) fmt
| ^~~
drivers/hwmon/asetek_gen6.c:246:3: note: in expansion of macro 'dev_info'
246 | dev_info("[*] Failled setting fan rpm %d,%d,%d/%d\n",
| ^~~~~~~~
include/linux/dev_printk.h:48:54: note: expected 'const char *' but argument is of type 'char'
48 | void _dev_info(const struct device *dev, const char *fmt, ...);
| ~~~~~~~~~~~~^~~
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:251:6: warning: no previous prototype for 'get_fan_target_rpm' [-Wmissing-prototypes]
251 | void get_fan_target_rpm(struct hwmon_fan_data *fan_data, long *val)
| ^~~~~~~~~~~~~~~~~~
drivers/hwmon/asetek_gen6.c:256:5: warning: no previous prototype for 'get_fan_current_rpm' [-Wmissing-prototypes]
256 | int get_fan_current_rpm(struct driver *cdev, struct hwmon_fan_data *fan_data,
| ^~~~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:15,
from include/linux/usb/ch9.h:36,
from include/linux/usb.h:6,
from drivers/hwmon/asetek_gen6.c:23:
drivers/hwmon/asetek_gen6.c: In function 'get_fan_current_rpm':
drivers/hwmon/asetek_gen6.c:280:12: error: passing argument 1 of '_dev_info' from incompatible pointer type [-Werror=incompatible-pointer-types]
280 | dev_info("[*] Failled retrieving fan rmp %d,%d,%d/%d\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| char *
include/linux/dev_printk.h:110:12: note: in definition of macro 'dev_info'
110 | _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:48:37: note: expected 'const struct device *' but argument is of type 'char *'
48 | void _dev_info(const struct device *dev, const char *fmt, ...);
| ~~~~~~~~~~~~~~~~~~~~~^~~
drivers/hwmon/asetek_gen6.c:281:13: warning: passing argument 2 of '_dev_info' makes pointer from integer without a cast [-Wint-conversion]
281 | recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
| ~~~~~~~~^~~
| |
| char
include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
19 | #define dev_fmt(fmt) fmt
| ^~~
drivers/hwmon/asetek_gen6.c:280:3: note: in expansion of macro 'dev_info'
280 | dev_info("[*] Failled retrieving fan rmp %d,%d,%d/%d\n",
| ^~~~~~~~
include/linux/dev_printk.h:48:54: note: expected 'const char *' but argument is of type 'char'
48 | void _dev_info(const struct device *dev, const char *fmt, ...);
| ~~~~~~~~~~~~^~~
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:287:5: warning: no previous prototype for 'set_fan_target_pwm' [-Wmissing-prototypes]
287 | int set_fan_target_pwm(struct driver *cdev, struct hwmon_fan_data *fan_data,
| ^~~~~~~~~~~~~~~~~~
In file included from include/linux/device.h:15,
from include/linux/usb/ch9.h:36,
from include/linux/usb.h:6,
from drivers/hwmon/asetek_gen6.c:23:
drivers/hwmon/asetek_gen6.c: In function 'set_fan_target_pwm':
drivers/hwmon/asetek_gen6.c:315:12: error: passing argument 1 of '_dev_info' from incompatible pointer type [-Werror=incompatible-pointer-types]
315 | dev_info("[*] Failled setting fan pwm %d,%d,%d/%d\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| char *
include/linux/dev_printk.h:110:12: note: in definition of macro 'dev_info'
110 | _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
| ^~~
include/linux/dev_printk.h:48:37: note: expected 'const struct device *' but argument is of type 'char *'
48 | void _dev_info(const struct device *dev, const char *fmt, ...);
| ~~~~~~~~~~~~~~~~~~~~~^~~
drivers/hwmon/asetek_gen6.c:316:13: warning: passing argument 2 of '_dev_info' makes pointer from integer without a cast [-Wint-conversion]
316 | recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
| ~~~~~~~~^~~
| |
| unsigned char
include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
19 | #define dev_fmt(fmt) fmt
| ^~~
drivers/hwmon/asetek_gen6.c:315:3: note: in expansion of macro 'dev_info'
315 | dev_info("[*] Failled setting fan pwm %d,%d,%d/%d\n",
| ^~~~~~~~
include/linux/dev_printk.h:48:54: note: expected 'const char *' but argument is of type 'unsigned char'
48 | void _dev_info(const struct device *dev, const char *fmt, ...);
| ~~~~~~~~~~~~^~~
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:320:9: warning: no previous prototype for 'is_visible_func' [-Wmissing-prototypes]
320 | umode_t is_visible_func(const void *d, enum hwmon_sensor_types type, u32 attr,
| ^~~~~~~~~~~~~~~
drivers/hwmon/asetek_gen6.c:475:5: warning: no previous prototype for 'read_func' [-Wmissing-prototypes]
475 | int read_func(struct device *dev, enum hwmon_sensor_types type, u32 attr,
| ^~~~~~~~~
drivers/hwmon/asetek_gen6.c:556:6: warning: no previous prototype for 'does_fan_exist' [-Wmissing-prototypes]
556 | bool does_fan_exist(struct driver *cdev, int channel)
| ^~~~~~~~~~~~~~
drivers/hwmon/asetek_gen6.c: In function 'does_fan_exist':
drivers/hwmon/asetek_gen6.c:569:16: warning: overflow in conversion from 'int' to 'char' changes value from '600' to '88' [-Woverflow]
569 | send_buf[3] = 600;
| ^~~
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:584:5: warning: no previous prototype for 'get_fan_count' [-Wmissing-prototypes]
584 | int get_fan_count(struct driver *dev)
| ^~~~~~~~~~~~~
drivers/hwmon/asetek_gen6.c:593:6: warning: no previous prototype for 'hwmon_init' [-Wmissing-prototypes]
593 | void hwmon_init(struct driver *dev)
| ^~~~~~~~~~
drivers/hwmon/asetek_gen6.c: In function 'hwmon_init':
>> drivers/hwmon/asetek_gen6.c:650:30: error: macro "dev_info" requires 3 arguments, but only 1 given
650 | dev_info("[*] Setup hwmon\n");
| ^
In file included from include/linux/device.h:15,
from include/linux/usb/ch9.h:36,
from include/linux/usb.h:6,
from drivers/hwmon/asetek_gen6.c:23:
include/linux/dev_printk.h:109: note: macro "dev_info" defined here
109 | #define dev_info(dev, fmt, ...) \
|
>> drivers/hwmon/asetek_gen6.c:650:2: error: 'dev_info' undeclared (first use in this function); did you mean '_dev_info'?
650 | dev_info("[*] Setup hwmon\n");
| ^~~~~~~~
| _dev_info
drivers/hwmon/asetek_gen6.c:650:2: note: each undeclared identifier is reported only once for each function it appears in
drivers/hwmon/asetek_gen6.c:596:17: warning: variable 'hwmon_dev' set but not used [-Wunused-but-set-variable]
596 | struct device *hwmon_dev;
| ^~~~~~~~~
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:653:6: warning: no previous prototype for 'hwmon_deinit' [-Wmissing-prototypes]
653 | void hwmon_deinit(struct driver *dev)
| ^~~~~~~~~~~~
drivers/hwmon/asetek_gen6.c: In function 'hwmon_deinit':
drivers/hwmon/asetek_gen6.c:656:35: error: macro "dev_info" requires 3 arguments, but only 1 given
656 | dev_info("[*] HWMON DISCONNECT\n");
| ^
In file included from include/linux/device.h:15,
from include/linux/usb/ch9.h:36,
from include/linux/usb.h:6,
from drivers/hwmon/asetek_gen6.c:23:
include/linux/dev_printk.h:109: note: macro "dev_info" defined here
109 | #define dev_info(dev, fmt, ...) \
|
drivers/hwmon/asetek_gen6.c:656:2: error: 'dev_info' undeclared (first use in this function); did you mean '_dev_info'?
656 | dev_info("[*] HWMON DISCONNECT\n");
| ^~~~~~~~
| _dev_info
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:670:5: warning: no previous prototype for 'init_device' [-Wmissing-prototypes]
670 | int init_device(struct usb_device *udev)
| ^~~~~~~~~~~
drivers/hwmon/asetek_gen6.c: In function 'init_device':
drivers/hwmon/asetek_gen6.c:678:3: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
678 | ;
| ^
drivers/hwmon/asetek_gen6.c: At top level:
drivers/hwmon/asetek_gen6.c:688:5: warning: no previous prototype for 'deinit_device' [-Wmissing-prototypes]
688 | int deinit_device(struct usb_device *udev)
| ^~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/_dev_info +212 drivers/hwmon/asetek_gen6.c
> 23 #include <linux/usb.h>
24 #include <linux/slab.h>
25 #include <linux/mutex.h>
26 #include <linux/errno.h>
27 #include <linux/hwmon.h>
28 #include <linux/hwmon-sysfs.h>
29
30 struct driver {
31 struct usb_device *udev;
32
33 char *bulk_out_buffer;
34 char *bulk_in_buffer;
35 size_t bulk_out_size;
36 size_t bulk_in_size;
37 char bulk_in_endpointAddr;
38 char bulk_out_endpointAddr;
39
40 struct usb_interface *interface; /* the interface for this device */
41 struct mutex io_mutex; /* synchronize I/O with disconnect */
42 struct semaphore
43 limit_sem; /* limiting the number of writes in progress */
44
45 struct kref kref;
46 };
47
48 struct curve_point {
49 uint8_t temp;
50 uint8_t pwm;
51 };
52
53 struct hwmon_fan_data {
54 char fan_channel;
55 long fan_target;
56 unsigned char fan_pwm_target;
57 long mode;
58 struct curve_point curve[7];
59 };
60
61 struct hwmon_data {
62 struct driver *dev;
63 int channel_count;
64 void **channel_data;
65 };
66
67 struct curve_point quiet_curve[] = {
68 {
69 .temp = 0x1F,
70 .pwm = 0x15,
71 },
72 {
73 .temp = 0x21,
74 .pwm = 0x1E,
75 },
76 {
77 .temp = 0x24,
78 .pwm = 0x25,
79 },
80 {
81 .temp = 0x27,
82 .pwm = 0x2D,
83 },
84 {
85 .temp = 0x29,
86 .pwm = 0x38,
87 },
88 {
89 .temp = 0x2C,
90 .pwm = 0x4A,
91 },
92 {
93 .temp = 0x2F,
94 .pwm = 0x64,
95 },
96 };
97
98 struct curve_point balanced_curve[] = {
99 {
100 .temp = 0x1C,
101 .pwm = 0x15,
102 },
103 {
104 .temp = 0x1E,
105 .pwm = 0x1B,
106 },
107 {
108 .temp = 0x20,
109 .pwm = 0x23,
110 },
111 {
112 .temp = 0x22,
113 .pwm = 0x28,
114 },
115 {
116 .temp = 0x24,
117 .pwm = 0x32,
118 },
119 {
120 .temp = 0x27,
121 .pwm = 0x48,
122 },
123 {
124 .temp = 0x29,
125 .pwm = 0x64,
126 },
127 };
128
129 struct curve_point extreme_curve[] = {
130 {
131 .temp = 0x19,
132 .pwm = 0x28,
133 },
134 {
135 .temp = 0x1B,
136 .pwm = 0x2E,
137 },
138 {
139 .temp = 0x1D,
140 .pwm = 0x37,
141 },
142 {
143 .temp = 0x1E,
144 .pwm = 0x41,
145 },
146 {
147 .temp = 0x1F,
148 .pwm = 0x4C,
149 },
150 {
151 .temp = 0x20,
152 .pwm = 0x56,
153 },
154 {
155 .temp = 0x21,
156 .pwm = 0x64,
157 },
158 };
159
160 #define default_curve quiet_curve
161
162 static const char SUCCESS[2] = { 0x12, 0x34 };
163
164 #define SUCCES_LENGTH 3
165
166 static bool check_succes(char command, char ret[SUCCES_LENGTH])
167 {
168 char success[SUCCES_LENGTH] = { command };
169
170 strncpy(&success[1], SUCCESS, SUCCES_LENGTH - 1);
171 return strncmp(ret, success, SUCCES_LENGTH - 1) == 0;
172 }
173
> 174 int set_fan_rpm_curve(struct driver *cdev, struct hwmon_fan_data *fan_data,
175 struct curve_point point[7])
176 {
177 int retval;
178 int wrote;
179 int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
180 int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
181 char *send_buf = cdev->bulk_out_buffer;
182 char *recv_buf = cdev->bulk_in_buffer;
183
184 memcpy(fan_data->curve, point, sizeof(fan_data->curve));
185
186 send_buf[0] = 0x40;
187 send_buf[1] = fan_data->fan_channel;
188 send_buf[2] = point[0].temp;
189 send_buf[3] = point[1].temp;
190 send_buf[4] = point[2].temp;
191 send_buf[5] = point[3].temp;
192 send_buf[6] = point[4].temp;
193 send_buf[7] = point[5].temp;
194 send_buf[8] = point[6].temp;
195 send_buf[9] = point[0].pwm;
196 send_buf[10] = point[1].pwm;
197 send_buf[11] = point[2].pwm;
198 send_buf[12] = point[3].pwm;
199 send_buf[13] = point[4].pwm;
200 send_buf[14] = point[5].pwm;
201 send_buf[15] = point[6].pwm;
202
203 retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 16, &wrote, 100);
204 if (retval != 0)
205 return retval;
206
207 retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
208 if (retval != 0)
209 return retval;
210
211 if (!check_succes(send_buf[0], recv_buf))
> 212 dev_info("[*] Failled setting fan curve %d,%d,%d/%d\n",
> 213 recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
214 return 0;
215 }
216
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57292 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-14 10:03 [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
2020-07-14 23:01 ` kernel test robot
@ 2020-07-15 3:07 ` Guenter Roeck
2020-07-15 8:34 ` Greg KH
` (2 more replies)
2020-07-17 12:20 ` jaap aarts
2 siblings, 3 replies; 12+ messages in thread
From: Guenter Roeck @ 2020-07-15 3:07 UTC (permalink / raw)
To: jaap aarts; +Cc: Jean Delvare, linux-hwmon, linux-usb, Marius Zachmann
On Tue, Jul 14, 2020 at 12:03:38PM +0200, jaap aarts wrote:
> Adds fan/pwm support for H1000i platinum.
> Custom temp/fan curves are not supported, however
> the presets found in the proprietary drivers are avaiable.
>
> Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
+Marius Zachmann for input.
Questions:
- Does this really have to be a different driver or can it be merged into
the corsair-cpro driver ?
- What about HID vs. USB driver ?
- is the kref complexity really needed ?
> ---
> drivers/hwmon/Kconfig | 6 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/asetek_gen6.c | 801 ++++++++++++++++++++++++++++++++++++
> 3 files changed, 808 insertions(+)
> create mode 100644 drivers/hwmon/asetek_gen6.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 288ae9f63588..521a9e0c88ca 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -378,6 +378,12 @@ config SENSORS_ARM_SCPI
> and power sensors available on ARM Ltd's SCP based platforms. The
> actual number and type of sensors exported depend on the platform.
>
> +config SENSORS_ASETEK_GEN6
> + tristate "Asetek generation 6 driver"
> + help
> + If you say yes here you get support for asetek generation 6 boards
> + found on most AIO liquid coolers with an asetek pump.
> +
> config SENSORS_ASB100
> tristate "Asus ASB100 Bach"
> depends on X86 && I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3e32c21f5efe..9683d2aae2b2 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_W83793) += w83793.o
> obj-$(CONFIG_SENSORS_W83795) += w83795.o
> obj-$(CONFIG_SENSORS_W83781D) += w83781d.o
> obj-$(CONFIG_SENSORS_W83791D) += w83791d.o
> +obj-$(CONFIG_SENSORS_ASETEK_GEN6) += asetek_gen6.o
>
> obj-$(CONFIG_SENSORS_AB8500) += abx500.o ab8500.o
> obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o
> diff --git a/drivers/hwmon/asetek_gen6.c b/drivers/hwmon/asetek_gen6.c
> new file mode 100644
> index 000000000000..4aea9ae0b944
> --- /dev/null
> +++ b/drivers/hwmon/asetek_gen6.c
> @@ -0,0 +1,801 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * A hwmon driver for most asetek gen 6 all-in-one liquid coolers.
> + * Copyright (c) Jaap Aarts 2020
> + *
> + * Protocol reverse engineered by audiohacked
> + * https://github.com/audiohacked/OpendriverLink
> + */
> +
> +/*
> + * Supports following chips:
This isn't really a chip, it is a liquid cooler.
> + * h100i platinum
> + *
> + * Other products should work with this driver but no testing has been done.
> + *
> + * Note: platinum is the codename name for pro within driver, so h100i platinum = h1ooi pro
h1ooi ? o's instead of 0 ?
> + *
> + * Note: fan curve control has not been implemented
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/usb.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/errno.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Alphabetic order, please. Also, it doesn't look like hwmon-sysfs.h is used.
> +
> +struct driver {
this is a terribly generic name for a local structure.
> + struct usb_device *udev;
> +
> + char *bulk_out_buffer;
> + char *bulk_in_buffer;
> + size_t bulk_out_size;
> + size_t bulk_in_size;
> + char bulk_in_endpointAddr;
> + char bulk_out_endpointAddr;
> +
> + struct usb_interface *interface; /* the interface for this device */
> + struct mutex io_mutex; /* synchronize I/O with disconnect */
Not used anywhere.
> + struct semaphore
> + limit_sem; /* limiting the number of writes in progress */
I don't see the need for a semaphore in this driver.
> +
> + struct kref kref;
> +};
> +
> +struct curve_point {
> + uint8_t temp;
> + uint8_t pwm;
> +};
> +
> +struct hwmon_fan_data {
> + char fan_channel;
> + long fan_target;
> + unsigned char fan_pwm_target;
> + long mode;
> + struct curve_point curve[7];
> +};
> +
> +struct hwmon_data {
> + struct driver *dev;
> + int channel_count;
> + void **channel_data;
> +};
> +
> +struct curve_point quiet_curve[] = {
> + {
> + .temp = 0x1F,
> + .pwm = 0x15,
> + },
> + {
> + .temp = 0x21,
> + .pwm = 0x1E,
> + },
> + {
> + .temp = 0x24,
> + .pwm = 0x25,
> + },
> + {
> + .temp = 0x27,
> + .pwm = 0x2D,
> + },
> + {
> + .temp = 0x29,
> + .pwm = 0x38,
> + },
> + {
> + .temp = 0x2C,
> + .pwm = 0x4A,
> + },
> + {
> + .temp = 0x2F,
> + .pwm = 0x64,
> + },
> +};
> +
> +struct curve_point balanced_curve[] = {
> + {
> + .temp = 0x1C,
> + .pwm = 0x15,
> + },
> + {
> + .temp = 0x1E,
> + .pwm = 0x1B,
> + },
> + {
> + .temp = 0x20,
> + .pwm = 0x23,
> + },
> + {
> + .temp = 0x22,
> + .pwm = 0x28,
> + },
> + {
> + .temp = 0x24,
> + .pwm = 0x32,
> + },
> + {
> + .temp = 0x27,
> + .pwm = 0x48,
> + },
> + {
> + .temp = 0x29,
> + .pwm = 0x64,
> + },
> +};
> +
> +struct curve_point extreme_curve[] = {
> + {
> + .temp = 0x19,
> + .pwm = 0x28,
> + },
> + {
> + .temp = 0x1B,
> + .pwm = 0x2E,
> + },
> + {
> + .temp = 0x1D,
> + .pwm = 0x37,
> + },
> + {
> + .temp = 0x1E,
> + .pwm = 0x41,
> + },
> + {
> + .temp = 0x1F,
> + .pwm = 0x4C,
> + },
> + {
> + .temp = 0x20,
> + .pwm = 0x56,
> + },
> + {
> + .temp = 0x21,
> + .pwm = 0x64,
> + },
> +};
We don't get into the business of deciding fan curves in the kernel.
The driver should implement sets of {pwm_auto_pointN_temp,
pwm_auto_pointN_pwm} instead and leave it up to userspace to set
actual values.
> +
> +#define default_curve quiet_curve
> +
> +static const char SUCCESS[2] = { 0x12, 0x34 };
> +
> +#define SUCCES_LENGTH 3
> +
> +static bool check_succes(char command, char ret[SUCCES_LENGTH])
> +{
> + char success[SUCCES_LENGTH] = { command };
> +
> + strncpy(&success[1], SUCCESS, SUCCES_LENGTH - 1);
> + return strncmp(ret, success, SUCCES_LENGTH - 1) == 0;
> +}
This seems terribly expensive and complicated. I have not spend time trying
to analyze what it actually does, but it seems highly unlikely that such a
complex evaluation is needed.
> +
> +int set_fan_rpm_curve(struct driver *cdev, struct hwmon_fan_data *fan_data,
> + struct curve_point point[7])
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> + char *send_buf = cdev->bulk_out_buffer;
> + char *recv_buf = cdev->bulk_in_buffer;
> +
> + memcpy(fan_data->curve, point, sizeof(fan_data->curve));
> +
> + send_buf[0] = 0x40;
> + send_buf[1] = fan_data->fan_channel;
> + send_buf[2] = point[0].temp;
> + send_buf[3] = point[1].temp;
> + send_buf[4] = point[2].temp;
> + send_buf[5] = point[3].temp;
> + send_buf[6] = point[4].temp;
> + send_buf[7] = point[5].temp;
> + send_buf[8] = point[6].temp;
> + send_buf[9] = point[0].pwm;
> + send_buf[10] = point[1].pwm;
> + send_buf[11] = point[2].pwm;
> + send_buf[12] = point[3].pwm;
> + send_buf[13] = point[4].pwm;
> + send_buf[14] = point[5].pwm;
> + send_buf[15] = point[6].pwm;
> +
> + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 16, &wrote, 100);
> + if (retval != 0)
if (retval)
> + return retval;
> +
> + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
> + if (retval != 0)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf))
> + dev_info("[*] Failled setting fan curve %d,%d,%d/%d\n",
> + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
No error return ? Then drop the check.
> + return 0;
> +}
> +
> +int set_fan_target_rpm(struct driver *cdev, struct hwmon_fan_data *fan_data,
> + long val)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> +
> + char *send_buf = cdev->bulk_out_buffer;
> + char *recv_buf = cdev->bulk_in_buffer;
> +
> + fan_data->fan_target = val;
> + fan_data->fan_pwm_target = 0;
> +
> + send_buf[0] = 0x43;
Please use defines instead of magic values.
> + send_buf[1] = fan_data->fan_channel;
> + send_buf[2] = (fan_data->fan_target >> 8);
> + send_buf[3] = fan_data->fan_target;
> +
> + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> + if (retval != 0)
> + return retval;
> +
> + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
100 seconds timeout is a bit unreal.
> + if (retval != 0)
> + return retval;
> +
> + //no error
Useless comment.
> + if (!check_succes(send_buf[0], recv_buf))
> + dev_info("[*] Failled setting fan rpm %d,%d,%d/%d\n",
> + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> + return 0;
> +}
> +
> +void get_fan_target_rpm(struct hwmon_fan_data *fan_data, long *val)
> +{
> + *val = fan_data->fan_target;
> +}
This function does not add any value. Caller can access fan_data->fan_target
directly.
> +
> +int get_fan_current_rpm(struct driver *cdev, struct hwmon_fan_data *fan_data,
> + long *val)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> +
> + char *send_buf = cdev->bulk_out_buffer;
> + char *recv_buf = cdev->bulk_in_buffer;
> +
> + send_buf[0] = 0x41;
> + send_buf[1] = fan_data->fan_channel;
> +
> + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 2, &wrote, 100);
> + if (retval != 0)
> + return retval;
> +
> + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> + if (retval != 0)
> + return retval;
> +
> + if (!check_succes(0x41, recv_buf) ||
> + recv_buf[3] != fan_data->fan_channel)
> + dev_info("[*] Failled retrieving fan rmp %d,%d,%d/%d\n",
Failed, rpm. But that message is really pointless (as is the check),
since the result is ignored and whatever is in the buffer is returned
to the caller anyway.
> + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> +
> + *val = (((uint8_t)recv_buf[4]) << 8) + (uint8_t)recv_buf[5];
Declaring send_buf and recv_buf as char pointer just to typecast recv_buf
is not really useful and just makes the code more complex.
> + return 0;
> +}
> +
> +int set_fan_target_pwm(struct driver *cdev, struct hwmon_fan_data *fan_data,
> + long val)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> +
> + unsigned char *send_buf = cdev->bulk_out_buffer;
> + unsigned char *recv_buf = cdev->bulk_in_buffer;
Now we have char, uint8_t, and unsigned char. Please be consistent.
> +
> + fan_data->fan_pwm_target = val;
> + fan_data->fan_target = 0;
> +
> + send_buf[0] = 0x42;
> + send_buf[1] = fan_data->fan_channel;
> + send_buf[3] = fan_data->fan_pwm_target;
> +
> + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> + if (retval != 0)
> + return retval;
> +
> + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
> + if (retval != 0)
> + return retval;
> +
> + //no error
> + if (!check_succes(send_buf[0], recv_buf))
> + dev_info("[*] Failled setting fan pwm %d,%d,%d/%d\n",
> + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> + return 0;
> +}
> +
> +umode_t is_visible_func(const void *d, enum hwmon_sensor_types type, u32 attr,
_func is quite pointless in function names.
> + int channel)
Maximum line length is now 100.
> +{
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + return 0444;
> + break;
> + case hwmon_fan_target:
> + return 0644;
> + break;
> + case hwmon_fan_min:
> + return 0444;
> + break;
> + default:
> + break;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + return 0200;
> + break;
> + case hwmon_pwm_mode:
> + return 0644;
> + break;
> + default:
> + break;
> + }
> + break;
> +
> + default:
> + break;
> + }
> + return 0;
> +}
> +
> +static int write_func(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + struct hwmon_data *data = dev_get_drvdata(dev);
> + struct driver *cdev = data->dev;
> + struct hwmon_fan_data *fan_data;
> + int retval = 0;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_target:
> + fan_data = data->channel_data[channel];
> + if (fan_data->mode != 1)
> + return -EINVAL;
> +
> + retval = usb_autopm_get_interface(cdev->interface);
> + if (retval)
> + goto exit;
> +
> + if (down_trylock(&cdev->limit_sem)) {
> + retval = -EAGAIN;
> + goto cleanup_interface;
> + }
> +
> + retval = set_fan_target_rpm(cdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + goto exit;
All those "goto exit;" are useless and inconsistent.
> + default:
> + return -EINVAL;
> + }
> + goto exit;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + fan_data = data->channel_data[channel];
> + if (fan_data->mode != 1)
> + return -EINVAL;
> +
> + retval = usb_autopm_get_interface(cdev->interface);
> + if (retval)
> + goto exit;
> +
> + if (down_trylock(&cdev->limit_sem)) {
> + retval = -EAGAIN;
> + goto cleanup_interface;
> + }
> +
> + retval = set_fan_target_pwm(cdev, fan_data, val);
> + if (retval)
> + return retval;
> +
> + goto cleanup;
goto are expected to be used for error cleanup, not for normal function exits.
> + case hwmon_pwm_enable:
> + fan_data = data->channel_data[channel];
> +
> + retval = usb_autopm_get_interface(cdev->interface);
> + if (retval)
> + goto exit;
> +
> + if (down_trylock(&cdev->limit_sem)) {
> + retval = -EAGAIN;
> + goto cleanup_interface;
> + }
> + fan_data->mode = val;
> +
> + switch (val) {
> + case 0:
> + set_fan_rpm_curve(cdev, fan_data,
> + default_curve);
> + break;
> + case 1:
> + if (fan_data->fan_target != 0) {
> + retval = set_fan_target_rpm(
> + cdev, fan_data,
> + fan_data->fan_target);
> + if (retval)
> + goto cleanup;
> + } else if (fan_data->fan_pwm_target != 0) {
> + retval = set_fan_target_pwm(
> + cdev, fan_data,
> + fan_data->fan_pwm_target);
> + if (retval)
> + goto cleanup;
> + }
> + break;
> + case 2:
> + set_fan_rpm_curve(cdev, fan_data, quiet_curve);
> + break;
> + case 3:
> + set_fan_rpm_curve(cdev, fan_data,
> + balanced_curve);
> + break;
> + case 4:
> + set_fan_rpm_curve(cdev, fan_data,
> + extreme_curve);
> + break;
> + }
"pwm_enable" is not supposed to be used for parameter settings as done here.
It is supposd to be used to enable/disable automatic fan control.
> + goto cleanup;
> + default:
> + return -EINVAL;
> + }
> + goto exit;
> + default:
> + return -EINVAL;
> + }
> +
> +cleanup:
> + up(&cdev->limit_sem);
> +cleanup_interface:
> + usb_autopm_put_interface(cdev->interface);
> +exit:
> + return retval;
> +}
> +
> +int read_func(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct hwmon_data *data = dev_get_drvdata(dev);
> + struct driver *cdev = data->dev;
> + struct hwmon_fan_data *fan_data;
> + int retval = 0;
> +
> + if (channel >= data->channel_count)
> + return -EAGAIN;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + fan_data = data->channel_data[channel];
> +
> + retval = usb_autopm_get_interface(cdev->interface);
> + if (retval)
> + goto exit;
> +
> + if (down_trylock(&cdev->limit_sem)) {
> + retval = -EAGAIN;
> + goto cleanup_interface;
> + }
> +
> + retval = get_fan_current_rpm(cdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + goto cleanup;
> + case hwmon_fan_target:
> + fan_data = data->channel_data[channel];
> + if (fan_data->mode != 1) {
> + *val = 0;
> + goto exit;
> + }
> +
> + get_fan_target_rpm(fan_data, val);
> + goto exit;
> + case hwmon_fan_min:
> + *val = 200;
> + goto exit;
> +
> + default:
> + return -EINVAL;
> + }
> + goto exit;
> +
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_mode:
> + fan_data = data->channel_data[channel];
> + *val = fan_data->mode;
> + goto exit;
> + default:
> + return -EINVAL;
> + }
> + goto exit;
> +
> + default:
> + return -EINVAL;
> + }
> +
> +cleanup:
> + up(&cdev->limit_sem);
> +cleanup_interface:
> + usb_autopm_put_interface(cdev->interface);
> +exit:
> + return retval;
> +}
> +
> +#define fan_config (HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN)
> +#define pwm_config (HWMON_PWM_INPUT | HWMON_PWM_ENABLE)
> +
> +static const struct hwmon_ops asetek_6_ops = {
> + .is_visible = is_visible_func,
> + .read = read_func,
> + .write = write_func,
> +};
> +
> +bool does_fan_exist(struct driver *cdev, int channel)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> +
> + char *send_buf = cdev->bulk_out_buffer;
> + char *recv_buf = cdev->bulk_in_buffer;
> +
> + send_buf[0] = 0x43;
> + send_buf[1] = channel;
> + send_buf[2] = (600 >> 8);
> + send_buf[3] = 600;
> +
> + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> + if (retval != 0)
> + return false;
> +
> + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
> + if (retval != 0)
> + return false;
> +
> + if (!check_succes(send_buf[0], recv_buf))
> + return false;
> + return true;
> +}
> +
> +int get_fan_count(struct driver *dev)
> +{
> + int fan;
> +
> + for (fan = 0; does_fan_exist(dev, fan); fan += 1)
> + ;
> + return fan;
> +}
> +
> +void hwmon_init(struct driver *dev)
> +{
> + int fan_id;
> + struct device *hwmon_dev;
> + struct hwmon_fan_data *fan;
> + struct hwmon_data *data = devm_kzalloc(
> + &dev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
> + struct hwmon_chip_info *hwmon_info = devm_kzalloc(
> + &dev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
> + //Allocate the info table
> + struct hwmon_channel_info **aio_info =
> + devm_kzalloc(&dev->udev->dev,
> + sizeof(struct hwmon_channel_info *) * 2,
> + GFP_KERNEL); //2 for each channel info.
> +
> + //Allocate the fan and PWM configuration
> + u32 *fans_config = devm_kzalloc(&dev->udev->dev,
> + sizeof(u32) * (data->channel_count + 1),
> + GFP_KERNEL);
> + u32 *pwms_config = devm_kzalloc(&dev->udev->dev,
> + sizeof(u32) * (data->channel_count + 1),
> + GFP_KERNEL);
> +
> + data->channel_count = get_fan_count(dev); //amount of fans
> + data->channel_data =
> + devm_kzalloc(&dev->udev->dev,
> + sizeof(char *) * data->channel_count, GFP_KERNEL);
> +
> + //For each fan create a data channel a fan config entry and a pwm config entry
Please no C++ comments
> + for (fan_id = 0; fan_id <= data->channel_count; fan_id++) {
> + fan = devm_kzalloc(&dev->udev->dev,
> + sizeof(struct hwmon_fan_data), GFP_KERNEL);
> + fan->fan_channel = fan_id;
> + fan->mode = 2;
> + data->channel_data[fan_id] = fan;
> + fans_config[fan_id] = fan_config;
> + pwms_config[fan_id] = pwm_config;
> + }
> + fans_config[data->channel_count] = 0;
> + pwms_config[data->channel_count] = 0;
> +
> + aio_info[0] = devm_kzalloc(
> + &dev->udev->dev, sizeof(struct hwmon_channel_info), GFP_KERNEL);
> + aio_info[0]->type = hwmon_fan;
> + aio_info[0]->config = fans_config;
> +
> + aio_info[1] = devm_kzalloc(
> + &dev->udev->dev, sizeof(struct hwmon_channel_info), GFP_KERNEL);
> + aio_info[1]->type = hwmon_pwm;
> + aio_info[1]->config = pwms_config;
> +
> + hwmon_info->ops = &asetek_6_ops;
> + hwmon_info->info = (const struct hwmon_channel_info **)aio_info;
> +
> + data->dev = dev;
> + hwmon_dev = devm_hwmon_device_register_with_info(
> + &dev->udev->dev, "driver_fan", data, hwmon_info, NULL);
> + dev_info("[*] Setup hwmon\n");
Does this even compile ?
> +}
> +
> +void hwmon_deinit(struct driver *dev)
> +{
> + hwmon_device_unregister(&dev->udev->dev);
Defeats the purpose of devm_hwmon_device_register_with_info().
> + dev_info("[*] HWMON DISCONNECT\n");
> +}
> +
> +/*
> + * Devices that work with this driver.
> + * More devices should work, however none have been tested.
> + */
> +static const struct usb_device_id astk_table[] = {
> + { USB_DEVICE(0x1b1c, 0x0c15) },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(usb, astk_table);
> +
> +int init_device(struct usb_device *udev)
> +{
> + int retval;
> +
> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
> + 0xffff, 0x0000, 0, 0, 0);
> + //this always returns error
Needs explanation why it is needed.
> + if (retval != 0)
> + ;
> +
> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> + 0x0002, 0x0000, 0, 0, 0);
> + if (retval != 0)
> + return retval;
> +
> + return 0;
return retval;
> +}
> +
> +int deinit_device(struct usb_device *udev)
> +{
> + int retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> + 0x0004, 0x0000, 0, 0, 0);
> + if (retval != 0)
> + return retval;
> +
> + return 0;
> +}
> +
> +static void astk_delete(struct kref *kref)
> +{
> + struct driver *dev = container_of(kref, struct driver, kref);
> +
> + usb_put_intf(dev->interface);
> + usb_put_dev(dev->udev);
> + kfree(dev->bulk_in_buffer);
> + kfree(dev->bulk_out_buffer);
> + kfree(dev);
> +}
> +
> +static int astk_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct driver *dev;
> + int retval = 0;
Unnecessary initialization.
> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + retval = -ENOMEM;
> + goto exit;
> + }
> +
> + retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> + &bulk_out, NULL, NULL);
> + if (retval != 0)
> + goto exit;
> +
> + dev->udev = usb_get_dev(interface_to_usbdev(interface));
> + dev->interface = usb_get_intf(interface);
> +
> + /* set up the endpoint information */
> + /* use only the first bulk-in and bulk-out endpoints */
> + dev->bulk_in_size = usb_endpoint_maxp(bulk_in);
> + dev->bulk_in_buffer = kmalloc(dev->bulk_in_size, GFP_KERNEL);
> + dev->bulk_in_endpointAddr = bulk_in->bEndpointAddress;
> + dev->bulk_out_size = usb_endpoint_maxp(bulk_out);
> + dev->bulk_out_buffer = kmalloc(dev->bulk_out_size, GFP_KERNEL);
> + dev->bulk_out_endpointAddr = bulk_out->bEndpointAddress;
> +
> + retval = init_device(dev->udev);
> + if (retval) {
> + dev_err(&interface->dev, "Failled initialising this device.\n");
> + goto exit;
> + }
> +
> + hwmon_init(dev);
> +
> + usb_set_intfdata(interface, dev);
> + kref_init(&dev->kref);
> + mutex_init(&dev->io_mutex);
> + sema_init(&dev->limit_sem, 8);
> +exit:
> + return retval;
> +}
> +
> +static void astk_disconnect(struct usb_interface *interface)
> +{
> + struct driver *dev = usb_get_intfdata(interface);
> +
> + hwmon_deinit(dev);
> + usb_set_intfdata(interface, NULL);
> + kref_put(&dev->kref, astk_delete);
> + deinit_device(dev->udev);
> +}
> +static int astk_resume(struct usb_interface *intf)
> +{
> + return 0;
> +}
> +
> +static int astk_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> + return 0;
> +}
What is the point of those functions if they don't do anything ?
> +
> +static struct usb_driver astk_driver = {
> + .name = "Asetek gen6 driver",
> + .id_table = astk_table,
> + .probe = astk_probe,
> + .disconnect = astk_disconnect,
> + .resume = astk_resume,
> + .suspend = astk_suspend,
> + .supports_autosuspend = 1,
> +};
> +
> +static int __init astk_init(void)
> +{
> + int ret = usb_register(&astk_driver);
> +
> + return ret;
return usb_register(...);
> +}
> +
> +static void __exit astk_exit(void)
> +{
> + usb_deregister(&astk_driver);
> +}
> +
> +module_init(astk_init);
> +module_exit(astk_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jaap Aarts <jaap.aarts1@gmail.com>");
> +MODULE_DESCRIPTION("Asetek gen6 driver");
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-15 3:07 ` Guenter Roeck
@ 2020-07-15 8:34 ` Greg KH
2020-07-15 11:18 ` jaap aarts
2020-07-15 12:17 ` Marius Zachmann
2 siblings, 0 replies; 12+ messages in thread
From: Greg KH @ 2020-07-15 8:34 UTC (permalink / raw)
To: Guenter Roeck
Cc: jaap aarts, Jean Delvare, linux-hwmon, linux-usb, Marius Zachmann
On Tue, Jul 14, 2020 at 08:07:40PM -0700, Guenter Roeck wrote:
> On Tue, Jul 14, 2020 at 12:03:38PM +0200, jaap aarts wrote:
> > Adds fan/pwm support for H1000i platinum.
> > Custom temp/fan curves are not supported, however
> > the presets found in the proprietary drivers are avaiable.
> >
> > Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
>
> +Marius Zachmann for input.
>
> Questions:
> - Does this really have to be a different driver or can it be merged into
> the corsair-cpro driver ?
> - What about HID vs. USB driver ?
> - is the kref complexity really needed ?
At first glance, I can't tell what the kref is trying to keep track of.
So I would say no, it's not. :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-15 3:07 ` Guenter Roeck
2020-07-15 8:34 ` Greg KH
@ 2020-07-15 11:18 ` jaap aarts
2020-07-15 14:47 ` Guenter Roeck
2020-07-15 12:17 ` Marius Zachmann
2 siblings, 1 reply; 12+ messages in thread
From: jaap aarts @ 2020-07-15 11:18 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, linux-usb, Marius Zachmann
On Wed, 15 Jul 2020 at 05:07, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Jul 14, 2020 at 12:03:38PM +0200, jaap aarts wrote:
> > Adds fan/pwm support for H1000i platinum.
> > Custom temp/fan curves are not supported, however
> > the presets found in the proprietary drivers are avaiable.
> >
> > Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
>
> +Marius Zachmann for input.
>
> Questions:
> - Does this really have to be a different driver or can it be merged into
> the corsair-cpro driver ?
I cannot find this driver at the moment, the only corsair driver I can find
is the HID driver. As far as I know all asetek gen 6 products use the same
interface. Out of curiosity I contacted asetek to confirm, but other userland
drivers have used the same code for all products of previous generations.
> - What about HID vs. USB driver ?
This is not really a HID. I asked in the kernel newbies mailing list and
I was told HWMON is probably the right place. Most of the code is
related to HWMON so this seems to be the right place to me as well.
> - is the kref complexity really needed ?
That is leftover from the usb_skeleton driver, I assumed it was
necessary. Apparently not so I will remove this
>
> > ---
> > drivers/hwmon/Kconfig | 6 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/asetek_gen6.c | 801 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 808 insertions(+)
> > create mode 100644 drivers/hwmon/asetek_gen6.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 288ae9f63588..521a9e0c88ca 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -378,6 +378,12 @@ config SENSORS_ARM_SCPI
> > and power sensors available on ARM Ltd's SCP based platforms. The
> > actual number and type of sensors exported depend on the platform.
> >
> > +config SENSORS_ASETEK_GEN6
> > + tristate "Asetek generation 6 driver"
> > + help
> > + If you say yes here you get support for asetek generation 6 boards
> > + found on most AIO liquid coolers with an asetek pump.
> > +
> > config SENSORS_ASB100
> > tristate "Asus ASB100 Bach"
> > depends on X86 && I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 3e32c21f5efe..9683d2aae2b2 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_W83793) += w83793.o
> > obj-$(CONFIG_SENSORS_W83795) += w83795.o
> > obj-$(CONFIG_SENSORS_W83781D) += w83781d.o
> > obj-$(CONFIG_SENSORS_W83791D) += w83791d.o
> > +obj-$(CONFIG_SENSORS_ASETEK_GEN6) += asetek_gen6.o
> >
> > obj-$(CONFIG_SENSORS_AB8500) += abx500.o ab8500.o
> > obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o
> > diff --git a/drivers/hwmon/asetek_gen6.c b/drivers/hwmon/asetek_gen6.c
> > new file mode 100644
> > index 000000000000..4aea9ae0b944
> > --- /dev/null
> > +++ b/drivers/hwmon/asetek_gen6.c
> > @@ -0,0 +1,801 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * A hwmon driver for most asetek gen 6 all-in-one liquid coolers.
> > + * Copyright (c) Jaap Aarts 2020
> > + *
> > + * Protocol reverse engineered by audiohacked
> > + * https://github.com/audiohacked/OpendriverLink
> > + */
> > +
> > +/*
> > + * Supports following chips:
>
> This isn't really a chip, it is a liquid cooler.
fixed
>
> > + * h100i platinum
> > + *
> > + * Other products should work with this driver but no testing has been done.
> > + *
> > + * Note: platinum is the codename name for pro within driver, so h100i platinum = h1ooi pro
>
> h1ooi ? o's instead of 0 ?
fixed
>
> > + *
> > + * Note: fan curve control has not been implemented
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/usb.h>
> > +#include <linux/slab.h>
> > +#include <linux/mutex.h>
> > +#include <linux/errno.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
>
> Alphabetic order, please. Also, it doesn't look like hwmon-sysfs.h is used.
fixed
>
> > +
> > +struct driver {
>
> this is a terribly generic name for a local structure.
DIdn't really think about this, named it asetek_gen6_device.
Also named all variables of this type to adev, previously
udev/dev (inconsistent)
>
> > + struct usb_device *udev;
> > +
> > + char *bulk_out_buffer;
> > + char *bulk_in_buffer;
> > + size_t bulk_out_size;
> > + size_t bulk_in_size;
> > + char bulk_in_endpointAddr;
> > + char bulk_out_endpointAddr;
> > +
> > + struct usb_interface *interface; /* the interface for this device */
> > + struct mutex io_mutex; /* synchronize I/O with disconnect */
>
> Not used anywhere.
Yup, I thought I caught all of those
>
> > + struct semaphore
> > + limit_sem; /* limiting the number of writes in progress */
>
> I don't see the need for a semaphore in this driver.
After each write there is always a read, I dont want another hwmon
call to write before the first call reads. This might cause incorrect
error messages.
>
> > +
> > + struct kref kref;
> > +};
> > +
> > +struct curve_point {
> > + uint8_t temp;
> > + uint8_t pwm;
> > +};
> > +
> > +struct hwmon_fan_data {
> > + char fan_channel;
> > + long fan_target;
> > + unsigned char fan_pwm_target;
> > + long mode;
> > + struct curve_point curve[7];
> > +};
> > +
> > +struct hwmon_data {
> > + struct driver *dev;
> > + int channel_count;
> > + void **channel_data;
> > +};
> > +
> > +struct curve_point quiet_curve[] = {
> > + {
> > + .temp = 0x1F,
> > + .pwm = 0x15,
> > + },
> > + {
> > + .temp = 0x21,
> > + .pwm = 0x1E,
> > + },
> > + {
> > + .temp = 0x24,
> > + .pwm = 0x25,
> > + },
> > + {
> > + .temp = 0x27,
> > + .pwm = 0x2D,
> > + },
> > + {
> > + .temp = 0x29,
> > + .pwm = 0x38,
> > + },
> > + {
> > + .temp = 0x2C,
> > + .pwm = 0x4A,
> > + },
> > + {
> > + .temp = 0x2F,
> > + .pwm = 0x64,
> > + },
> > +};
> > +
> > +struct curve_point balanced_curve[] = {
> > + {
> > + .temp = 0x1C,
> > + .pwm = 0x15,
> > + },
> > + {
> > + .temp = 0x1E,
> > + .pwm = 0x1B,
> > + },
> > + {
> > + .temp = 0x20,
> > + .pwm = 0x23,
> > + },
> > + {
> > + .temp = 0x22,
> > + .pwm = 0x28,
> > + },
> > + {
> > + .temp = 0x24,
> > + .pwm = 0x32,
> > + },
> > + {
> > + .temp = 0x27,
> > + .pwm = 0x48,
> > + },
> > + {
> > + .temp = 0x29,
> > + .pwm = 0x64,
> > + },
> > +};
> > +
> > +struct curve_point extreme_curve[] = {
> > + {
> > + .temp = 0x19,
> > + .pwm = 0x28,
> > + },
> > + {
> > + .temp = 0x1B,
> > + .pwm = 0x2E,
> > + },
> > + {
> > + .temp = 0x1D,
> > + .pwm = 0x37,
> > + },
> > + {
> > + .temp = 0x1E,
> > + .pwm = 0x41,
> > + },
> > + {
> > + .temp = 0x1F,
> > + .pwm = 0x4C,
> > + },
> > + {
> > + .temp = 0x20,
> > + .pwm = 0x56,
> > + },
> > + {
> > + .temp = 0x21,
> > + .pwm = 0x64,
> > + },
> > +};
>
> We don't get into the business of deciding fan curves in the kernel.
> The driver should implement sets of {pwm_auto_pointN_temp,
> pwm_auto_pointN_pwm} instead and leave it up to userspace to set
> actual values.
I have thought about that, I am willing to remove the non-default fan curves.
The default must be there since the asetek board doesn't have a way to reset
to the default profile AFAIK. I don't want the user to lock themselves into
manual control until the reboot.
>
> > +
> > +#define default_curve quiet_curve
> > +
> > +static const char SUCCESS[2] = { 0x12, 0x34 };
> > +
> > +#define SUCCES_LENGTH 3
> > +
> > +static bool check_succes(char command, char ret[SUCCES_LENGTH])
> > +{
> > + char success[SUCCES_LENGTH] = { command };
> > +
> > + strncpy(&success[1], SUCCESS, SUCCES_LENGTH - 1);
> > + return strncmp(ret, success, SUCCES_LENGTH - 1) == 0;
> > +}
>
> This seems terribly expensive and complicated. I have not spent time trying
> to analyze what it actually does, but it seems highly unlikely that such a
> complex evaluation is needed.
The success return values are always 0x{command}1234 this just puts them
together by creating an array with the first value set to {command} and copies
0x1234 into the other 0 bytes. This then compares that to the actual result.
>
> > +
> > +int set_fan_rpm_curve(struct driver *cdev, struct hwmon_fan_data *fan_data,
> > + struct curve_point point[7])
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> > + char *send_buf = cdev->bulk_out_buffer;
> > + char *recv_buf = cdev->bulk_in_buffer;
> > +
> > + memcpy(fan_data->curve, point, sizeof(fan_data->curve));
> > +
> > + send_buf[0] = 0x40;
> > + send_buf[1] = fan_data->fan_channel;
> > + send_buf[2] = point[0].temp;
> > + send_buf[3] = point[1].temp;
> > + send_buf[4] = point[2].temp;
> > + send_buf[5] = point[3].temp;
> > + send_buf[6] = point[4].temp;
> > + send_buf[7] = point[5].temp;
> > + send_buf[8] = point[6].temp;
> > + send_buf[9] = point[0].pwm;
> > + send_buf[10] = point[1].pwm;
> > + send_buf[11] = point[2].pwm;
> > + send_buf[12] = point[3].pwm;
> > + send_buf[13] = point[4].pwm;
> > + send_buf[14] = point[5].pwm;
> > + send_buf[15] = point[6].pwm;
> > +
> > + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 16, &wrote, 100);
> > + if (retval != 0)
>
> if (retval)
Is it okay to be more verbose? I prefer to not depend on behaviour like this.
> > + return retval;
> > +
> > + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
> > + if (retval != 0)
> > + return retval;
> > +
> > + if (!check_succes(send_buf[0], recv_buf))
> > + dev_info("[*] Failled setting fan curve %d,%d,%d/%d\n",
> > + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
>
> No error return ? Then drop the check.
Added a -EINVAL error
>
> > + return 0;
> > +}
> > +
> > +int set_fan_target_rpm(struct driver *cdev, struct hwmon_fan_data *fan_data,
> > + long val)
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> > +
> > + char *send_buf = cdev->bulk_out_buffer;
> > + char *recv_buf = cdev->bulk_in_buffer;
> > +
> > + fan_data->fan_target = val;
> > + fan_data->fan_pwm_target = 0;
> > +
> > + send_buf[0] = 0x43;
>
> Please use defines instead of magic values.
Is an enum ok as well?
>
> > + send_buf[1] = fan_data->fan_channel;
> > + send_buf[2] = (fan_data->fan_target >> 8);
> > + send_buf[3] = fan_data->fan_target;
> > +
> > + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> > + if (retval != 0)
> > + return retval;
> > +
> > + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
>
> 100 seconds timeout is a bit unreal.
Fixed
>
> > + if (retval != 0)
> > + return retval;
> > +
> > + //no error
>
> Useless comment.
Removed
>
> > + if (!check_succes(send_buf[0], recv_buf))
> > + dev_info("[*] Failled setting fan rpm %d,%d,%d/%d\n",
> > + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> > + return 0;
> > +}
> > +
> > +void get_fan_target_rpm(struct hwmon_fan_data *fan_data, long *val)
> > +{
> > + *val = fan_data->fan_target;
> > +}
>
> This function does not add any value. Caller can access fan_data->fan_target
> directly.
Yes, I initially had this because I liked everything being a function
but at some
point it did become a bit silly/
>
> > +
> > +int get_fan_current_rpm(struct driver *cdev, struct hwmon_fan_data *fan_data,
> > + long *val)
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> > +
> > + char *send_buf = cdev->bulk_out_buffer;
> > + char *recv_buf = cdev->bulk_in_buffer;
> > +
> > + send_buf[0] = 0x41;
> > + send_buf[1] = fan_data->fan_channel;
> > +
> > + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 2, &wrote, 100);
> > + if (retval != 0)
> > + return retval;
> > +
> > + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> > + if (retval != 0)
> > + return retval;
> > +
> > + if (!check_succes(0x41, recv_buf) ||
> > + recv_buf[3] != fan_data->fan_channel)
> > + dev_info("[*] Failled retrieving fan rmp %d,%d,%d/%d\n",
>
> Failed, rpm. But that message is really pointless (as is the check),
> since the result is ignored and whatever is in the buffer is returned
> to the caller anyway.
Should I remove the messages? I added error returns to all these checks.
>
> > + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> > +
> > + *val = (((uint8_t)recv_buf[4]) << 8) + (uint8_t)recv_buf[5];
>
> Declaring send_buf and recv_buf as char pointer just to typecast recv_buf
> is not really useful and just makes the code more complex.
I had problems with shifting and formatting, I changed everything over to
unsigned char, everything works with that type.
>
> > + return 0;
> > +}
> > +
> > +int set_fan_target_pwm(struct driver *cdev, struct hwmon_fan_data *fan_data,
> > + long val)
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> > +
> > + unsigned char *send_buf = cdev->bulk_out_buffer;
> > + unsigned char *recv_buf = cdev->bulk_in_buffer;
>
> Now we have char, uint8_t, and unsigned char. Please be consistent.
switched everything over to unsigned char
>
> > +
> > + fan_data->fan_pwm_target = val;
> > + fan_data->fan_target = 0;
> > +
> > + send_buf[0] = 0x42;
> > + send_buf[1] = fan_data->fan_channel;
> > + send_buf[3] = fan_data->fan_pwm_target;
> > +
> > + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> > + if (retval != 0)
> > + return retval;
> > +
> > + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
> > + if (retval != 0)
> > + return retval;
> > +
> > + //no error
> > + if (!check_succes(send_buf[0], recv_buf))
> > + dev_info("[*] Failled setting fan pwm %d,%d,%d/%d\n",
> > + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> > + return 0;
> > +}
> > +
> > +umode_t is_visible_func(const void *d, enum hwmon_sensor_types type, u32 attr,
>
> _func is quite pointless in function names.
You're right, fixed this
>
> > + int channel)
>
> Maximum line length is now 100.
wdym? My formatter automatically picks up formatting settings from somewhere.
I checked `ColumnLimit: 80` is still in .clan-format.
>
> > +{
> > + switch (type) {
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + return 0444;
> > + break;
> > + case hwmon_fan_target:
> > + return 0644;
> > + break;
> > + case hwmon_fan_min:
> > + return 0444;
> > + break;
> > + default:
> > + break;
> > + }
> > + break;
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + return 0200;
> > + break;
> > + case hwmon_pwm_mode:
> > + return 0644;
> > + break;
> > + default:
> > + break;
> > + }
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > + return 0;
> > +}
> > +
> > +static int write_func(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long val)
> > +{
> > + struct hwmon_data *data = dev_get_drvdata(dev);
> > + struct driver *cdev = data->dev;
> > + struct hwmon_fan_data *fan_data;
> > + int retval = 0;
> > +
> > + switch (type) {
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_target:
> > + fan_data = data->channel_data[channel];
> > + if (fan_data->mode != 1)
> > + return -EINVAL;
> > +
> > + retval = usb_autopm_get_interface(cdev->interface);
> > + if (retval)
> > + goto exit;
> > +
> > + if (down_trylock(&cdev->limit_sem)) {
> > + retval = -EAGAIN;
> > + goto cleanup_interface;
> > + }
> > +
> > + retval = set_fan_target_rpm(cdev, fan_data, val);
> > + if (retval)
> > + goto cleanup;
> > +
> > + goto exit;
>
> All those "goto exit;" are useless and inconsistent.
I think I made them more consistent, I am not used to using
goto for cleanup.
>
> > + default:
> > + return -EINVAL;
> > + }
> > + goto exit;
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + fan_data = data->channel_data[channel];
> > + if (fan_data->mode != 1)
> > + return -EINVAL;
> > +
> > + retval = usb_autopm_get_interface(cdev->interface);
> > + if (retval)
> > + goto exit;
> > +
> > + if (down_trylock(&cdev->limit_sem)) {
> > + retval = -EAGAIN;
> > + goto cleanup_interface;
> > + }
> > +
> > + retval = set_fan_target_pwm(cdev, fan_data, val);
> > + if (retval)
> > + return retval;
> > +
> > + goto cleanup;
>
> goto are expected to be used for error cleanup, not for normal function exits.
Agreed
>
> > + case hwmon_pwm_enable:
> > + fan_data = data->channel_data[channel];
> > +
> > + retval = usb_autopm_get_interface(cdev->interface);
> > + if (retval)
> > + goto exit;
> > +
> > + if (down_trylock(&cdev->limit_sem)) {
> > + retval = -EAGAIN;
> > + goto cleanup_interface;
> > + }
> > + fan_data->mode = val;
> > +
> > + switch (val) {
> > + case 0:
> > + set_fan_rpm_curve(cdev, fan_data,
> > + default_curve);
> > + break;
> > + case 1:
> > + if (fan_data->fan_target != 0) {
> > + retval = set_fan_target_rpm(
> > + cdev, fan_data,
> > + fan_data->fan_target);
> > + if (retval)
> > + goto cleanup;
> > + } else if (fan_data->fan_pwm_target != 0) {
> > + retval = set_fan_target_pwm(
> > + cdev, fan_data,
> > + fan_data->fan_pwm_target);
> > + if (retval)
> > + goto cleanup;
> > + }
> > + break;
> > + case 2:
> > + set_fan_rpm_curve(cdev, fan_data, quiet_curve);
> > + break;
> > + case 3:
> > + set_fan_rpm_curve(cdev, fan_data,
> > + balanced_curve);
> > + break;
> > + case 4:
> > + set_fan_rpm_curve(cdev, fan_data,
> > + extreme_curve);
> > + break;
> > + }
>
> "pwm_enable" is not supposed to be used for parameter settings as done here.
> It is supposd to be used to enable/disable automatic fan control.
https://www.kernel.org/doc/html/v5.8-rc1/hwmon/sysfs-interface.html
Says 2+ are supposed to be used for automatic control, this is also why
I added the fan curves in this driver.
>
> > + goto cleanup;
> > + default:
> > + return -EINVAL;
> > + }
> > + goto exit;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > +cleanup:
> > + up(&cdev->limit_sem);
> > +cleanup_interface:
> > + usb_autopm_put_interface(cdev->interface);
> > +exit:
> > + return retval;
> > +}
> > +
> > +int read_func(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> > + int channel, long *val)
> > +{
> > + struct hwmon_data *data = dev_get_drvdata(dev);
> > + struct driver *cdev = data->dev;
> > + struct hwmon_fan_data *fan_data;
> > + int retval = 0;
> > +
> > + if (channel >= data->channel_count)
> > + return -EAGAIN;
> > +
> > + switch (type) {
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + fan_data = data->channel_data[channel];
> > +
> > + retval = usb_autopm_get_interface(cdev->interface);
> > + if (retval)
> > + goto exit;
> > +
> > + if (down_trylock(&cdev->limit_sem)) {
> > + retval = -EAGAIN;
> > + goto cleanup_interface;
> > + }
> > +
> > + retval = get_fan_current_rpm(cdev, fan_data, val);
> > + if (retval)
> > + goto cleanup;
> > +
> > + goto cleanup;
> > + case hwmon_fan_target:
> > + fan_data = data->channel_data[channel];
> > + if (fan_data->mode != 1) {
> > + *val = 0;
> > + goto exit;
> > + }
> > +
> > + get_fan_target_rpm(fan_data, val);
> > + goto exit;
> > + case hwmon_fan_min:
> > + *val = 200;
> > + goto exit;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > + goto exit;
> > +
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_mode:
> > + fan_data = data->channel_data[channel];
> > + *val = fan_data->mode;
> > + goto exit;
> > + default:
> > + return -EINVAL;
> > + }
> > + goto exit;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > +cleanup:
> > + up(&cdev->limit_sem);
> > +cleanup_interface:
> > + usb_autopm_put_interface(cdev->interface);
> > +exit:
> > + return retval;
> > +}
> > +
> > +#define fan_config (HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN)
> > +#define pwm_config (HWMON_PWM_INPUT | HWMON_PWM_ENABLE)
> > +
> > +static const struct hwmon_ops asetek_6_ops = {
> > + .is_visible = is_visible_func,
> > + .read = read_func,
> > + .write = write_func,
> > +};
> > +
> > +bool does_fan_exist(struct driver *cdev, int channel)
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(cdev->udev, cdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(cdev->udev, cdev->bulk_in_endpointAddr);
> > +
> > + char *send_buf = cdev->bulk_out_buffer;
> > + char *recv_buf = cdev->bulk_in_buffer;
> > +
> > + send_buf[0] = 0x43;
> > + send_buf[1] = channel;
> > + send_buf[2] = (600 >> 8);
> > + send_buf[3] = 600;
> > +
> > + retval = usb_bulk_msg(cdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> > + if (retval != 0)
> > + return false;
> > +
> > + retval = usb_bulk_msg(cdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
> > + if (retval != 0)
> > + return false;
> > +
> > + if (!check_succes(send_buf[0], recv_buf))
> > + return false;
> > + return true;
> > +}
> > +
> > +int get_fan_count(struct driver *dev)
> > +{
> > + int fan;
> > +
> > + for (fan = 0; does_fan_exist(dev, fan); fan += 1)
> > + ;
> > + return fan;
> > +}
> > +
> > +void hwmon_init(struct driver *dev)
> > +{
> > + int fan_id;
> > + struct device *hwmon_dev;
> > + struct hwmon_fan_data *fan;
> > + struct hwmon_data *data = devm_kzalloc(
> > + &dev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
> > + struct hwmon_chip_info *hwmon_info = devm_kzalloc(
> > + &dev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
> > + //Allocate the info table
> > + struct hwmon_channel_info **aio_info =
> > + devm_kzalloc(&dev->udev->dev,
> > + sizeof(struct hwmon_channel_info *) * 2,
> > + GFP_KERNEL); //2 for each channel info.
> > +
> > + //Allocate the fan and PWM configuration
> > + u32 *fans_config = devm_kzalloc(&dev->udev->dev,
> > + sizeof(u32) * (data->channel_count + 1),
> > + GFP_KERNEL);
> > + u32 *pwms_config = devm_kzalloc(&dev->udev->dev,
> > + sizeof(u32) * (data->channel_count + 1),
> > + GFP_KERNEL);
> > +
> > + data->channel_count = get_fan_count(dev); //amount of fans
> > + data->channel_data =
> > + devm_kzalloc(&dev->udev->dev,
> > + sizeof(char *) * data->channel_count, GFP_KERNEL);
> > +
> > + //For each fan create a data channel a fan config entry and a pwm config entry
>
> Please no C++ comments
what are C++ comments?
I felt like this code was really confusing to look at, and wanted to
bring some clarity.
>
> > + for (fan_id = 0; fan_id <= data->channel_count; fan_id++) {
> > + fan = devm_kzalloc(&dev->udev->dev,
> > + sizeof(struct hwmon_fan_data), GFP_KERNEL);
> > + fan->fan_channel = fan_id;
> > + fan->mode = 2;
> > + data->channel_data[fan_id] = fan;
> > + fans_config[fan_id] = fan_config;
> > + pwms_config[fan_id] = pwm_config;
> > + }
> > + fans_config[data->channel_count] = 0;
> > + pwms_config[data->channel_count] = 0;
> > +
> > + aio_info[0] = devm_kzalloc(
> > + &dev->udev->dev, sizeof(struct hwmon_channel_info), GFP_KERNEL);
> > + aio_info[0]->type = hwmon_fan;
> > + aio_info[0]->config = fans_config;
> > +
> > + aio_info[1] = devm_kzalloc(
> > + &dev->udev->dev, sizeof(struct hwmon_channel_info), GFP_KERNEL);
> > + aio_info[1]->type = hwmon_pwm;
> > + aio_info[1]->config = pwms_config;
> > +
> > + hwmon_info->ops = &asetek_6_ops;
> > + hwmon_info->info = (const struct hwmon_channel_info **)aio_info;
> > +
> > + data->dev = dev;
> > + hwmon_dev = devm_hwmon_device_register_with_info(
> > + &dev->udev->dev, "driver_fan", data, hwmon_info, NULL);
> > + dev_info("[*] Setup hwmon\n");
>
> Does this even compile ?
I wish I could say no it does not, but it does. I would love to have a
vendor/device -id->configuration map, but I am honestly not sure
how I would implement that in C at the moment, let alone if that
would be allowed in the kernel.
>
> > +}
> > +
> > +void hwmon_deinit(struct driver *dev)
> > +{
> > + hwmon_device_unregister(&dev->udev->dev);
>
> Defeats the purpose of devm_hwmon_device_register_with_info().
I thought the managed part was talking about devm_kzalloc.
If not, I'm sorry I misunderstood the documentation.
Removed this.
>
> > + dev_info("[*] HWMON DISCONNECT\n");
> > +}
> > +
> > +/*
> > + * Devices that work with this driver.
> > + * More devices should work, however none have been tested.
> > + */
> > +static const struct usb_device_id astk_table[] = {
> > + { USB_DEVICE(0x1b1c, 0x0c15) },
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(usb, astk_table);
> > +
> > +int init_device(struct usb_device *udev)
> > +{
> > + int retval;
> > +
> > + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
> > + 0xffff, 0x0000, 0, 0, 0);
> > + //this always returns error
>
> Needs explanation why it is needed.
added.
>
> > + if (retval != 0)
> > + ;
> > +
> > + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> > + 0x0002, 0x0000, 0, 0, 0);
> > + if (retval != 0)
> > + return retval;
> > +
> > + return 0;
>
> return retval;
fixed
>
> > +}
> > +
> > +int deinit_device(struct usb_device *udev)
> > +{
> > + int retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> > + 0x0004, 0x0000, 0, 0, 0);
> > + if (retval != 0)
> > + return retval;
> > +
> > + return 0;
>
> > +}
> > +
> > +static void astk_delete(struct kref *kref)
> > +{
> > + struct driver *dev = container_of(kref, struct driver, kref);
> > +
> > + usb_put_intf(dev->interface);
> > + usb_put_dev(dev->udev);
> > + kfree(dev->bulk_in_buffer);
> > + kfree(dev->bulk_out_buffer);
> > + kfree(dev);
> > +}
> > +
> > +static int astk_probe(struct usb_interface *interface,
> > + const struct usb_device_id *id)
> > +{
> > + struct driver *dev;
> > + int retval = 0;
>
> Unnecessary initialization.
fixed
>
> > + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> > +
> > + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > + if (!dev) {
> > + retval = -ENOMEM;
> > + goto exit;
> > + }
> > +
> > + retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> > + &bulk_out, NULL, NULL);
> > + if (retval != 0)
> > + goto exit;
> > +
> > + dev->udev = usb_get_dev(interface_to_usbdev(interface));
> > + dev->interface = usb_get_intf(interface);
> > +
> > + /* set up the endpoint information */
> > + /* use only the first bulk-in and bulk-out endpoints */
> > + dev->bulk_in_size = usb_endpoint_maxp(bulk_in);
> > + dev->bulk_in_buffer = kmalloc(dev->bulk_in_size, GFP_KERNEL);
> > + dev->bulk_in_endpointAddr = bulk_in->bEndpointAddress;
> > + dev->bulk_out_size = usb_endpoint_maxp(bulk_out);
> > + dev->bulk_out_buffer = kmalloc(dev->bulk_out_size, GFP_KERNEL);
> > + dev->bulk_out_endpointAddr = bulk_out->bEndpointAddress;
> > +
> > + retval = init_device(dev->udev);
> > + if (retval) {
> > + dev_err(&interface->dev, "Failled initialising this device.\n");
> > + goto exit;
> > + }
> > +
> > + hwmon_init(dev);
> > +
> > + usb_set_intfdata(interface, dev);
> > + kref_init(&dev->kref);
> > + mutex_init(&dev->io_mutex);
> > + sema_init(&dev->limit_sem, 8);
> > +exit:
> > + return retval;
> > +}
> > +
> > +static void astk_disconnect(struct usb_interface *interface)
> > +{
> > + struct driver *dev = usb_get_intfdata(interface);
> > +
> > + hwmon_deinit(dev);
> > + usb_set_intfdata(interface, NULL);
> > + kref_put(&dev->kref, astk_delete);
> > + deinit_device(dev->udev);
> > +}
> > +static int astk_resume(struct usb_interface *intf)
> > +{
> > + return 0;
> > +}
> > +
> > +static int astk_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > + return 0;
> > +}
>
> What is the point of those functions if they don't do anything ?
They are needed for USB power management. If I do not provide
these functions the USB subsystem cannot use autosuspend.
>
> > +
> > +static struct usb_driver astk_driver = {
> > + .name = "Asetek gen6 driver",
> > + .id_table = astk_table,
> > + .probe = astk_probe,
> > + .disconnect = astk_disconnect,
> > + .resume = astk_resume,
> > + .suspend = astk_suspend,
> > + .supports_autosuspend = 1,
> > +};
> > +
> > +static int __init astk_init(void)
> > +{
> > + int ret = usb_register(&astk_driver);
> > +
> > + return ret;
>
> return usb_register(...);
fixed
>
> > +}
> > +
> > +static void __exit astk_exit(void)
> > +{
> > + usb_deregister(&astk_driver);
> > +}
> > +
> > +module_init(astk_init);
> > +module_exit(astk_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Jaap Aarts <jaap.aarts1@gmail.com>");
> > +MODULE_DESCRIPTION("Asetek gen6 driver");
> > --
> > 2.27.0
> >
After fixing most of these comments should I send in a new patchset?
Or should I reply with my new patch to this thread.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-15 11:18 ` jaap aarts
@ 2020-07-15 14:47 ` Guenter Roeck
2020-07-15 15:03 ` jaap aarts
0 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2020-07-15 14:47 UTC (permalink / raw)
To: jaap aarts; +Cc: Jean Delvare, linux-hwmon, linux-usb, Marius Zachmann
On Wed, Jul 15, 2020 at 01:18:58PM +0200, jaap aarts wrote:
> On Wed, 15 Jul 2020 at 05:07, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Tue, Jul 14, 2020 at 12:03:38PM +0200, jaap aarts wrote:
> > > Adds fan/pwm support for H1000i platinum.
> > > Custom temp/fan curves are not supported, however
> > > the presets found in the proprietary drivers are avaiable.
> > >
> > > Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
> >
> > +Marius Zachmann for input.
> >
> > Questions:
> > - Does this really have to be a different driver or can it be merged into
> > the corsair-cpro driver ?
> I cannot find this driver at the moment, the only corsair driver I can find
> is the HID driver. As far as I know all asetek gen 6 products use the same
> interface. Out of curiosity I contacted asetek to confirm, but other userland
> drivers have used the same code for all products of previous generations.
> > - What about HID vs. USB driver ?
> This is not really a HID. I asked in the kernel newbies mailing list and
> I was told HWMON is probably the right place. Most of the code is
> related to HWMON so this seems to be the right place to me as well.
Question is if this identifies itself as HID device. If it does,
it would either have to be blacklisted in the HID core, or it would
have to be implemented as hid driver. The latter would be preferred,
since otherwise a userspace application accessing it directly would
no longer work. Either case, the driver can and should still reside
in hwmon; that was not the question.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-15 14:47 ` Guenter Roeck
@ 2020-07-15 15:03 ` jaap aarts
2020-07-15 15:10 ` Guenter Roeck
0 siblings, 1 reply; 12+ messages in thread
From: jaap aarts @ 2020-07-15 15:03 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, linux-usb, Marius Zachmann
On Wed, 15 Jul 2020 at 16:47, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Jul 15, 2020 at 01:18:58PM +0200, jaap aarts wrote:
> > On Wed, 15 Jul 2020 at 05:07, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Tue, Jul 14, 2020 at 12:03:38PM +0200, jaap aarts wrote:
> > > > Adds fan/pwm support for H1000i platinum.
> > > > Custom temp/fan curves are not supported, however
> > > > the presets found in the proprietary drivers are avaiable.
> > > >
> > > > Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
> > >
> > > +Marius Zachmann for input.
> > >
> > > Questions:
> > > - Does this really have to be a different driver or can it be merged into
> > > the corsair-cpro driver ?
> > I cannot find this driver at the moment, the only corsair driver I can find
> > is the HID driver. As far as I know all asetek gen 6 products use the same
> > interface. Out of curiosity I contacted asetek to confirm, but other userland
> > drivers have used the same code for all products of previous generations.
> > > - What about HID vs. USB driver ?
> > This is not really a HID. I asked in the kernel newbies mailing list and
> > I was told HWMON is probably the right place. Most of the code is
> > related to HWMON so this seems to be the right place to me as well.
>
> Question is if this identifies itself as HID device. If it does,
> it would either have to be blacklisted in the HID core, or it would
> have to be implemented as hid driver. The latter would be preferred,
> since otherwise a userspace application accessing it directly would
> no longer work. Either case, the driver can and should still reside
> in hwmon; that was not the question.
Thanks for the clarification, lsusb lists bInterfaceClass as 255 or
"Vendor Specific Class", so not as a HID.
>
> Thanks,
> Guenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-15 15:03 ` jaap aarts
@ 2020-07-15 15:10 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2020-07-15 15:10 UTC (permalink / raw)
To: jaap aarts; +Cc: Jean Delvare, linux-hwmon, linux-usb, Marius Zachmann
On Wed, Jul 15, 2020 at 05:03:52PM +0200, jaap aarts wrote:
> On Wed, 15 Jul 2020 at 16:47, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Wed, Jul 15, 2020 at 01:18:58PM +0200, jaap aarts wrote:
> > > On Wed, 15 Jul 2020 at 05:07, Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > On Tue, Jul 14, 2020 at 12:03:38PM +0200, jaap aarts wrote:
> > > > > Adds fan/pwm support for H1000i platinum.
> > > > > Custom temp/fan curves are not supported, however
> > > > > the presets found in the proprietary drivers are avaiable.
> > > > >
> > > > > Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
> > > >
> > > > +Marius Zachmann for input.
> > > >
> > > > Questions:
> > > > - Does this really have to be a different driver or can it be merged into
> > > > the corsair-cpro driver ?
> > > I cannot find this driver at the moment, the only corsair driver I can find
> > > is the HID driver. As far as I know all asetek gen 6 products use the same
> > > interface. Out of curiosity I contacted asetek to confirm, but other userland
> > > drivers have used the same code for all products of previous generations.
> > > > - What about HID vs. USB driver ?
> > > This is not really a HID. I asked in the kernel newbies mailing list and
> > > I was told HWMON is probably the right place. Most of the code is
> > > related to HWMON so this seems to be the right place to me as well.
> >
> > Question is if this identifies itself as HID device. If it does,
> > it would either have to be blacklisted in the HID core, or it would
> > have to be implemented as hid driver. The latter would be preferred,
> > since otherwise a userspace application accessing it directly would
> > no longer work. Either case, the driver can and should still reside
> > in hwmon; that was not the question.
> Thanks for the clarification, lsusb lists bInterfaceClass as 255 or
> "Vendor Specific Class", so not as a HID.
Ok.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-15 3:07 ` Guenter Roeck
2020-07-15 8:34 ` Greg KH
2020-07-15 11:18 ` jaap aarts
@ 2020-07-15 12:17 ` Marius Zachmann
2020-07-15 14:20 ` Guenter Roeck
2 siblings, 1 reply; 12+ messages in thread
From: Marius Zachmann @ 2020-07-15 12:17 UTC (permalink / raw)
To: jaap aarts, Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, linux-usb
On 15.07.20 at 05:07:40 CEST, Guenter Roeck wrote
> On Tue, Jul 14, 2020 at 12:03:38PM +0200, jaap aarts wrote:
> > Adds fan/pwm support for H1000i platinum.
> > Custom temp/fan curves are not supported, however
> > the presets found in the proprietary drivers are avaiable.
> >
> > Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
>
> +Marius Zachmann for input.
>
> Questions:
> - Does this really have to be a different driver or can it be merged into
> the corsair-cpro driver ?
>
From what I can see the protocol has quite a few differences.
A merged driver would need to implement functions like e.g. set_pwm for
each device seperately. Also error handling and buffer sizes would be
seperate for each device.
If there were more usb/hid drivers in hwmon it maybe could make sense
to have an additional abstraction layer, but for now I do not see
anything which could be gained by this.
Greetings
Marius
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-15 12:17 ` Marius Zachmann
@ 2020-07-15 14:20 ` Guenter Roeck
0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2020-07-15 14:20 UTC (permalink / raw)
To: Marius Zachmann; +Cc: jaap aarts, Jean Delvare, linux-hwmon, linux-usb
On Wed, Jul 15, 2020 at 02:17:37PM +0200, Marius Zachmann wrote:
> On 15.07.20 at 05:07:40 CEST, Guenter Roeck wrote
> > On Tue, Jul 14, 2020 at 12:03:38PM +0200, jaap aarts wrote:
> > > Adds fan/pwm support for H1000i platinum.
> > > Custom temp/fan curves are not supported, however
> > > the presets found in the proprietary drivers are avaiable.
> > >
> > > Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
> >
> > +Marius Zachmann for input.
> >
> > Questions:
> > - Does this really have to be a different driver or can it be merged into
> > the corsair-cpro driver ?
> >
>
> From what I can see the protocol has quite a few differences.
> A merged driver would need to implement functions like e.g. set_pwm for
> each device seperately. Also error handling and buffer sizes would be
> seperate for each device.
> If there were more usb/hid drivers in hwmon it maybe could make sense
> to have an additional abstraction layer, but for now I do not see
> anything which could be gained by this.
>
Ok.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
2020-07-14 10:03 [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
2020-07-14 23:01 ` kernel test robot
2020-07-15 3:07 ` Guenter Roeck
@ 2020-07-17 12:20 ` jaap aarts
2 siblings, 0 replies; 12+ messages in thread
From: jaap aarts @ 2020-07-17 12:20 UTC (permalink / raw)
To: Delvare, Guenter Roeck, linux-hwmon, linux-usb
I found out that since asetek generation 5 the resellers put their own
boards in the product,
so the new patch I sent in has fixed all the things mentioned as
fixed/changed and is renamed.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum
@ 2020-07-23 9:17 jaap aarts
0 siblings, 0 replies; 12+ messages in thread
From: jaap aarts @ 2020-07-23 9:17 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb; +Cc: jaap aarts
Adds fan/pwm support for H100i platinum.
Custom temp/fan curves are not supported.
v3:
- tineout is now a #define
- made all funcions static
- removed all retval != 0
- removed horrible mess during hwmon_init using a device table
- removed C++ style comments
- added USB dependency
Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
---
drivers/hwmon/Kconfig | 7 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/corsair_hydro_i_pro.c | 718 ++++++++++++++++++++++++++++
3 files changed, 726 insertions(+)
create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 288ae9f63588..f466b72d0f67 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -378,6 +378,13 @@ config SENSORS_ARM_SCPI
and power sensors available on ARM Ltd's SCP based platforms. The
actual number and type of sensors exported depend on the platform.
+config SENSORS_CORSAIR_HYDRO_I_PRO
+ tristate "Corsair hydro HXXXi pro driver"
+ depends on USB
+ help
+ If you say yes here you get support for the corsair hydro HXXXi pro
+ range of devices.
+
config SENSORS_ASB100
tristate "Asus ASB100 Bach"
depends on X86 && I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3e32c21f5efe..ec63294b3ef1 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_W83793) += w83793.o
obj-$(CONFIG_SENSORS_W83795) += w83795.o
obj-$(CONFIG_SENSORS_W83781D) += w83781d.o
obj-$(CONFIG_SENSORS_W83791D) += w83791d.o
+obj-$(CONFIG_SENSORS_CORSAIR_HYDRO_I_PRO) += corsair_hydro_i_pro.o
obj-$(CONFIG_SENSORS_AB8500) += abx500.o ab8500.o
obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o
diff --git a/drivers/hwmon/corsair_hydro_i_pro.c b/drivers/hwmon/corsair_hydro_i_pro.c
new file mode 100644
index 000000000000..38ca2554a5c1
--- /dev/null
+++ b/drivers/hwmon/corsair_hydro_i_pro.c
@@ -0,0 +1,718 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * A hwmon driver for all corsair hyxro HXXXi pro all-in-one liquid coolers.
+ * Copyright (c) Jaap Aarts 2020
+ *
+ * Protocol reverse engineered by audiohacked
+ * https://github.com/audiohacked/OpendriverLink
+ */
+
+/*
+ * Supports following liquid coolers:
+ * H100i platinum
+ *
+ * Other products should work with this driver but no testing has been done.
+ *
+ * Note: platinum is the codename name for pro within the driver, so H100i platinum = H100i pro.
+ * But some products are actually calles platinum, these are not intended to be supported.
+ *
+ * Note: fan curve control has not been implemented
+ */
+
+#include <linux/errno.h>
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+
+struct device_config {
+ u16 vendor_id;
+ u16 product_id;
+ u8 fancount;
+ const struct hwmon_channel_info **hwmon_info;
+};
+
+struct hydro_i_pro_device {
+ struct usb_device *udev;
+
+ const struct device_config *config;
+
+ unsigned char *bulk_out_buffer;
+ char *bulk_in_buffer;
+ size_t bulk_out_size;
+ size_t bulk_in_size;
+ char bulk_in_endpointAddr;
+ char bulk_out_endpointAddr;
+
+ struct usb_interface *interface; /* the interface for this device */
+ struct semaphore
+ limit_sem; /* limiting the number of writes in progress */
+};
+
+struct hwmon_data {
+ struct hydro_i_pro_device *hdev;
+ int channel_count;
+ void **channel_data;
+};
+
+struct curve_point {
+ uint8_t temp;
+ uint8_t pwm;
+};
+
+struct hwmon_fan_data {
+ char fan_channel;
+ long fan_target;
+ unsigned char fan_pwm_target;
+ long mode;
+ struct curve_point curve[7];
+};
+
+struct curve_point quiet_curve[] = {
+ {
+ .temp = 0x1F,
+ .pwm = 0x15,
+ },
+ {
+ .temp = 0x21,
+ .pwm = 0x1E,
+ },
+ {
+ .temp = 0x24,
+ .pwm = 0x25,
+ },
+ {
+ .temp = 0x27,
+ .pwm = 0x2D,
+ },
+ {
+ .temp = 0x29,
+ .pwm = 0x38,
+ },
+ {
+ .temp = 0x2C,
+ .pwm = 0x4A,
+ },
+ {
+ .temp = 0x2F,
+ .pwm = 0x64,
+ },
+};
+
+#define default_curve quiet_curve
+
+static const struct hwmon_channel_info *dual_fan[] = {
+ HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN,
+ HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN),
+ HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
+
+ NULL
+};
+
+static const struct device_config config_table[] = {
+ {
+ .vendor_id = 0x1b1c,
+ .product_id = 0x0c15,
+ .fancount = 2,
+ .hwmon_info = dual_fan,
+ },
+};
+
+#define BULK_TIMEOUT 100
+
+enum opcodes {
+ PWM_FAN_CURVE_CMD = 0x40,
+ PWM_GET_CURRENT_CMD = 0x41,
+ PWM_FAN_TARGET_CMD = 0x42,
+ RPM_FAN_TARGET_CMD = 0x43,
+};
+
+#define SUCCES_LENGTH 3
+#define SUCCES_CODE 0x12, 0x34
+
+static bool check_succes(enum opcodes command, char ret[SUCCES_LENGTH])
+{
+ char success[SUCCES_LENGTH] = { command, SUCCES_CODE };
+ return strncmp(ret, success, SUCCES_LENGTH) == 0;
+}
+
+static const struct device_config *get_device_configuration(u16 vendor_id,
+ u16 product_id)
+{
+ const struct device_config *config;
+ int i = 0;
+ int n = ARRAY_SIZE(config_table);
+ for (i = 0; i < n; i++) {
+ config = &config_table[i];
+ if (config->vendor_id == vendor_id &&
+ config->product_id == product_id) {
+ return config;
+ }
+ }
+ return config;
+}
+
+static int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
+ struct hwmon_fan_data *fan_data,
+ struct curve_point point[7])
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
+ unsigned char *send_buf = hdev->bulk_out_buffer;
+ unsigned char *recv_buf = hdev->bulk_in_buffer;
+
+ memcpy(fan_data->curve, point, sizeof(fan_data->curve));
+
+ send_buf[0] = PWM_FAN_CURVE_CMD;
+ send_buf[1] = fan_data->fan_channel;
+ send_buf[2] = point[0].temp;
+ send_buf[3] = point[1].temp;
+ send_buf[4] = point[2].temp;
+ send_buf[5] = point[3].temp;
+ send_buf[6] = point[4].temp;
+ send_buf[7] = point[5].temp;
+ send_buf[8] = point[6].temp;
+ send_buf[9] = point[0].pwm;
+ send_buf[10] = point[1].pwm;
+ send_buf[11] = point[2].pwm;
+ send_buf[12] = point[3].pwm;
+ send_buf[13] = point[4].pwm;
+ send_buf[14] = point[5].pwm;
+ send_buf[15] = point[6].pwm;
+
+ retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 16, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf)) {
+ dev_info(&hdev->udev->dev,
+ "[*] failed setting fan curve %d,%d,%d/%d\n",
+ recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int set_fan_target_rpm(struct hydro_i_pro_device *hdev,
+ struct hwmon_fan_data *fan_data, long val)
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
+
+ unsigned char *send_buf = hdev->bulk_out_buffer;
+ unsigned char *recv_buf = hdev->bulk_in_buffer;
+
+ fan_data->fan_target = val;
+ fan_data->fan_pwm_target = 0;
+
+ send_buf[0] = RPM_FAN_TARGET_CMD;
+ send_buf[1] = fan_data->fan_channel;
+ send_buf[2] = (fan_data->fan_target >> 8);
+ send_buf[3] = fan_data->fan_target;
+
+ retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf)) {
+ dev_info(&hdev->udev->dev,
+ "[*] failed setting fan rpm %d,%d,%d/%d\n",
+ recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int get_fan_current_rpm(struct hydro_i_pro_device *hdev,
+ struct hwmon_fan_data *fan_data, long *val)
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
+
+ unsigned char *send_buf = hdev->bulk_out_buffer;
+ unsigned char *recv_buf = hdev->bulk_in_buffer;
+
+ send_buf[0] = PWM_GET_CURRENT_CMD;
+ send_buf[1] = fan_data->fan_channel;
+
+ retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 2, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf) ||
+ recv_buf[3] != fan_data->fan_channel) {
+ dev_info(&hdev->udev->dev,
+ "[*] failed retrieving fan rmp %d,%d,%d/%d\n",
+ recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
+ return -EINVAL;
+ }
+
+ *val = ((recv_buf[4]) << 8) + recv_buf[5];
+ return 0;
+}
+
+static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
+ struct hwmon_fan_data *fan_data, long val)
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
+
+ unsigned char *send_buf = hdev->bulk_out_buffer;
+ unsigned char *recv_buf = hdev->bulk_in_buffer;
+
+ fan_data->fan_pwm_target = val;
+ fan_data->fan_target = 0;
+
+ send_buf[0] = PWM_FAN_TARGET_CMD;
+ send_buf[1] = fan_data->fan_channel;
+ send_buf[3] = fan_data->fan_pwm_target;
+
+ retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf)) {
+ dev_info(&hdev->udev->dev,
+ "[*] failed setting fan pwm %d,%d,%d/%d\n",
+ recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static umode_t hwmon_is_visible(const void *d, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ return 0444;
+ break;
+ case hwmon_fan_target:
+ return 0644;
+ break;
+ case hwmon_fan_min:
+ return 0444;
+ break;
+ default:
+ break;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ return 0200;
+ break;
+ case hwmon_pwm_enable:
+ return 0644;
+ break;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+ return 0;
+}
+
+static int hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ struct hwmon_data *data = dev_get_drvdata(dev);
+ struct hydro_i_pro_device *hdev = data->hdev;
+ struct hwmon_fan_data *fan_data;
+ int retval = 0;
+
+ if (channel >= data->channel_count)
+ return -ECHRNG;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_target:
+ fan_data = data->channel_data[channel];
+ if (fan_data->mode != 1)
+ return -EINVAL;
+
+ retval = usb_autopm_get_interface(hdev->interface);
+ if (retval)
+ goto exit;
+
+ if (down_trylock(&hdev->limit_sem)) {
+ retval = -EAGAIN;
+ goto cleanup_interface;
+ }
+
+ retval = set_fan_target_rpm(hdev, fan_data, val);
+ if (retval)
+ goto cleanup;
+
+ break;
+ default:
+ return -EINVAL;
+ }
+ goto exit;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ fan_data = data->channel_data[channel];
+ if (fan_data->mode != 1)
+ return -EINVAL;
+
+ retval = usb_autopm_get_interface(hdev->interface);
+ if (retval)
+ goto exit;
+
+ if (down_trylock(&hdev->limit_sem)) {
+ retval = -EAGAIN;
+ goto cleanup_interface;
+ }
+
+ retval = set_fan_target_pwm(hdev, fan_data, val);
+ if (retval)
+ goto cleanup;
+
+ break;
+ case hwmon_pwm_enable:
+ fan_data = data->channel_data[channel];
+
+ retval = usb_autopm_get_interface(hdev->interface);
+ if (retval)
+ goto exit;
+
+ if (down_trylock(&hdev->limit_sem)) {
+ retval = -EAGAIN;
+ goto cleanup_interface;
+ }
+ fan_data->mode = val;
+
+ switch (val) {
+ case 0:
+ set_fan_pwm_curve(hdev, fan_data,
+ default_curve);
+ break;
+ case 1:
+ if (fan_data->fan_target != 0) {
+ retval = set_fan_target_rpm(
+ hdev, fan_data,
+ fan_data->fan_target);
+ if (retval)
+ goto cleanup;
+ } else if (fan_data->fan_pwm_target != 0) {
+ retval = set_fan_target_pwm(
+ hdev, fan_data,
+ fan_data->fan_pwm_target);
+ if (retval)
+ goto cleanup;
+ }
+ break;
+ case 2:
+ set_fan_pwm_curve(hdev, fan_data,
+ default_curve);
+ break;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+ goto exit;
+ default:
+ return -EINVAL;
+ }
+
+cleanup:
+ up(&hdev->limit_sem);
+cleanup_interface:
+ usb_autopm_put_interface(hdev->interface);
+exit:
+ return retval;
+}
+
+static int hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct hwmon_data *data = dev_get_drvdata(dev);
+ struct hydro_i_pro_device *hdev = data->hdev;
+ struct hwmon_fan_data *fan_data;
+ int retval = 0;
+
+ if (channel >= data->channel_count)
+ return -ECHRNG;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ fan_data = data->channel_data[channel];
+
+ retval = usb_autopm_get_interface(hdev->interface);
+ if (retval)
+ goto exit;
+
+ if (down_trylock(&hdev->limit_sem)) {
+ retval = -EAGAIN;
+ goto cleanup_interface;
+ }
+
+ retval = get_fan_current_rpm(hdev, fan_data, val);
+ if (retval)
+ goto cleanup;
+
+ goto cleanup;
+ case hwmon_fan_target:
+ fan_data = data->channel_data[channel];
+ if (fan_data->mode != 1) {
+ *val = 0;
+ goto exit;
+ }
+ *val = fan_data->fan_target;
+ goto exit;
+ case hwmon_fan_min:
+ *val = 200;
+ goto exit;
+
+ default:
+ return -EINVAL;
+ }
+ goto exit;
+
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_enable:
+ fan_data = data->channel_data[channel];
+ *val = fan_data->mode;
+ goto exit;
+ default:
+ return -EINVAL;
+ }
+ goto exit;
+
+ default:
+ return -EINVAL;
+ }
+
+cleanup:
+ up(&hdev->limit_sem);
+cleanup_interface:
+ usb_autopm_put_interface(hdev->interface);
+exit:
+ return retval;
+}
+
+static const struct hwmon_ops i_pro_ops = {
+ .is_visible = hwmon_is_visible,
+ .read = hwmon_read,
+ .write = hwmon_write,
+};
+
+static void hwmon_init(struct hydro_i_pro_device *hdev)
+{
+ int fan_id;
+ struct device *hwmon_dev;
+ struct hwmon_fan_data *fan;
+ struct hwmon_data *data = devm_kzalloc(
+ &hdev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
+ struct hwmon_chip_info *hwmon_info = devm_kzalloc(
+ &hdev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
+
+ data->channel_count = hdev->config->fancount;
+ data->channel_data =
+ devm_kzalloc(&hdev->udev->dev,
+ sizeof(char *) * data->channel_count, GFP_KERNEL);
+
+ /* For each fan create a data channel a fan config entry and a pwm config entry */
+ for (fan_id = 0; fan_id <= data->channel_count; fan_id++) {
+ fan = devm_kzalloc(&hdev->udev->dev,
+ sizeof(struct hwmon_fan_data), GFP_KERNEL);
+ fan->fan_channel = fan_id;
+ fan->mode = 2;
+ data->channel_data[fan_id] = fan;
+ }
+
+ hwmon_info->ops = &i_pro_ops;
+ hwmon_info->info = hdev->config->hwmon_info;
+
+ data->hdev = hdev;
+ hwmon_dev = devm_hwmon_device_register_with_info(
+ &hdev->udev->dev, "driver_fan", data, hwmon_info, NULL);
+ dev_info(&hdev->udev->dev, "[*] Setup hwmon\n");
+}
+
+/*
+ * Devices that work with this driver.
+ * More devices should work, however none have been tested.
+ */
+static const struct usb_device_id astk_table[] = {
+ { USB_DEVICE(0x1b1c, 0x0c15) },
+ {},
+};
+
+MODULE_DEVICE_TABLE(usb, astk_table);
+
+static int init_device(struct usb_device *udev)
+{
+ int retval;
+
+ /*
+ * This is needed because when running windows in a vm with proprietary driver
+ *and you switch to this driver, the device will not respond unless you run this.
+ */
+ retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
+ 0xffff, 0x0000, 0, 0, 0);
+ /*this always returns error*/
+ if (retval)
+ ;
+
+ retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
+ 0x0002, 0x0000, 0, 0, 0);
+ return retval;
+}
+
+static int deinit_device(struct usb_device *udev)
+{
+ return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
+ 0x0004, 0x0000, 0, 0, 0);
+}
+
+static void astk_delete(struct hydro_i_pro_device *hdev)
+{
+ usb_put_intf(hdev->interface);
+ usb_put_dev(hdev->udev);
+ kfree(hdev->bulk_in_buffer);
+ kfree(hdev->bulk_out_buffer);
+ kfree(hdev);
+}
+
+static int astk_probe(struct usb_interface *interface,
+ const struct usb_device_id *id)
+{
+ struct hydro_i_pro_device *hdev;
+ int retval;
+ struct usb_endpoint_descriptor *bulk_in, *bulk_out;
+
+ hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);
+ if (!hdev) {
+ retval = -ENOMEM;
+ goto exit;
+ }
+
+ hdev->config = get_device_configuration(id->idVendor, id->idProduct);
+ if (hdev->config == NULL) {
+ retval = -ENOMEM;
+ goto exit;
+ }
+
+ retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
+ &bulk_out, NULL, NULL);
+ if (retval)
+ goto exit;
+
+ hdev->udev = usb_get_dev(interface_to_usbdev(interface));
+ hdev->interface = usb_get_intf(interface);
+
+ /*
+ * set up the endpoint information
+ * use only the first bulk-in and bulk-out endpoints
+ */
+ hdev->bulk_in_size = usb_endpoint_maxp(bulk_in);
+ hdev->bulk_in_buffer = kmalloc(hdev->bulk_in_size, GFP_KERNEL);
+ hdev->bulk_in_endpointAddr = bulk_in->bEndpointAddress;
+ hdev->bulk_out_size = usb_endpoint_maxp(bulk_out);
+ hdev->bulk_out_buffer = kmalloc(hdev->bulk_out_size, GFP_KERNEL);
+ hdev->bulk_out_endpointAddr = bulk_out->bEndpointAddress;
+
+ retval = init_device(hdev->udev);
+ if (retval) {
+ dev_err(&interface->dev, "failed initialising this device.\n");
+ goto exit;
+ }
+
+ hwmon_init(hdev);
+
+ usb_set_intfdata(interface, hdev);
+ sema_init(&hdev->limit_sem, 8);
+exit:
+ return retval;
+}
+
+static void astk_disconnect(struct usb_interface *interface)
+{
+ struct hydro_i_pro_device *hdev = usb_get_intfdata(interface);
+
+ dev_info(&hdev->udev->dev, "[*] DEINIT DEVICE\n");
+ usb_set_intfdata(interface, NULL);
+ astk_delete(hdev);
+ deinit_device(hdev->udev);
+}
+static int astk_resume(struct usb_interface *intf)
+{
+ return 0;
+}
+
+static int astk_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ return 0;
+}
+
+static struct usb_driver hydro_i_pro_driver = {
+ .name = "hydro_i_pro_device",
+ .id_table = astk_table,
+ .probe = astk_probe,
+ .disconnect = astk_disconnect,
+ .resume = astk_resume,
+ .suspend = astk_suspend,
+ .supports_autosuspend = 1,
+};
+
+static int __init hydro_i_pro_init(void)
+{
+ return usb_register(&hydro_i_pro_driver);
+}
+
+static void __exit hydro_i_pro_exit(void)
+{
+ usb_deregister(&hydro_i_pro_driver);
+}
+
+module_init(hydro_i_pro_init);
+module_exit(hydro_i_pro_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jaap Aarts <jaap.aarts1@gmail.com>");
+MODULE_DESCRIPTION("Corsair HXXXi pro device driver");
--
2.27.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-23 9:20 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-14 10:03 [PATCH] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
2020-07-14 23:01 ` kernel test robot
2020-07-15 3:07 ` Guenter Roeck
2020-07-15 8:34 ` Greg KH
2020-07-15 11:18 ` jaap aarts
2020-07-15 14:47 ` Guenter Roeck
2020-07-15 15:03 ` jaap aarts
2020-07-15 15:10 ` Guenter Roeck
2020-07-15 12:17 ` Marius Zachmann
2020-07-15 14:20 ` Guenter Roeck
2020-07-17 12:20 ` jaap aarts
2020-07-23 9:17 jaap aarts
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).