Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
@ 2020-07-17 12:16 jaap aarts
  2020-07-17 21:41 ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: jaap aarts @ 2020-07-17 12:16 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, however
the presets found in the proprietary drivers are available.

Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
---
 drivers/hwmon/Kconfig               |   6 +
 drivers/hwmon/Makefile              |   1 +
 drivers/hwmon/corsair_hydro_i_pro.c | 791 ++++++++++++++++++++++++++++
 3 files changed, 798 insertions(+)
 create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 288ae9f63588..9831a40fb05f 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_CORSAIR_HYDRO_I_PRO
+	tristate "Corsair hydro HXXXi pro driver"
+	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..43bf52d8d365
--- /dev/null
+++ b/drivers/hwmon/corsair_hydro_i_pro.c
@@ -0,0 +1,791 @@
+// 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 hydro_i_pro_device {
+	struct usb_device *udev;
+
+	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 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 hydro_i_pro_device *hdev;
+	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
+
+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 const char SUCCESS[SUCCES_LENGTH - 1] = { 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;
+}
+
+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, 100);
+	if (retval != 0)
+		return retval;
+
+	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
+	if (retval != 0)
+		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;
+}
+
+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, 100);
+	if (retval != 0)
+		return retval;
+
+	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
+	if (retval != 0)
+		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;
+}
+
+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, 100);
+	if (retval != 0)
+		return retval;
+
+	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
+	if (retval != 0)
+		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;
+}
+
+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, 100);
+	if (retval != 0)
+		return retval;
+
+	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
+	if (retval != 0)
+		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;
+}
+
+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;
+
+	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, quiet_curve);
+				break;
+			case 3:
+				set_fan_pwm_curve(hdev, fan_data,
+						  balanced_curve);
+				break;
+			case 4:
+				set_fan_pwm_curve(hdev, fan_data,
+						  extreme_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;
+}
+
+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 -EAGAIN;
+
+	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;
+}
+
+#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 i_pro_ops = {
+	.is_visible = hwmon_is_visible,
+	.read = hwmon_read,
+	.write = hwmon_write,
+};
+
+bool does_fan_exist(struct hydro_i_pro_device *hdev, int channel)
+{
+	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] = RPM_FAN_TARGET_CMD;
+	send_buf[1] = channel;
+	send_buf[2] = (600 >> 8);
+	send_buf[3] = (unsigned char)600;
+
+	retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote, 100);
+	if (retval != 0)
+		return false;
+
+	retval = usb_bulk_msg(hdev->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 hydro_i_pro_device *hdev)
+{
+	int fan;
+
+	for (fan = 0; does_fan_exist(hdev, fan); fan += 1)
+		;
+	return fan;
+}
+
+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);
+	//Allocate the info table
+	struct hwmon_channel_info **aio_info =
+		devm_kzalloc(&hdev->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(&hdev->udev->dev,
+					sizeof(u32) * (data->channel_count + 1),
+					GFP_KERNEL);
+	u32 *pwms_config = devm_kzalloc(&hdev->udev->dev,
+					sizeof(u32) * (data->channel_count + 1),
+					GFP_KERNEL);
+
+	data->channel_count = get_fan_count(hdev); //amount of fans
+	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;
+		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(&hdev->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(&hdev->udev->dev,
+			     sizeof(struct hwmon_channel_info), GFP_KERNEL);
+	aio_info[1]->type = hwmon_pwm;
+	aio_info[1]->config = pwms_config;
+
+	hwmon_info->ops = &i_pro_ops;
+	hwmon_info->info = (const struct hwmon_channel_info **)aio_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);
+
+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 != 0)
+		;
+
+	retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
+				 0x0002, 0x0000, 0, 0, 0);
+	return retval;
+}
+
+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;
+	}
+
+	retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
+					   &bulk_out, NULL, NULL);
+	if (retval != 0)
+		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	[flat|nested] 10+ messages in thread

* Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-17 12:16 [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
@ 2020-07-17 21:41 ` kernel test robot
  2020-07-17 23:15 ` Guenter Roeck
  2020-07-19  0:30 ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2020-07-17 21:41 UTC (permalink / raw)
  To: jaap aarts, Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb
  Cc: kbuild-all, jaap aarts


[-- Attachment #1: Type: text/plain, Size: 11342 bytes --]

Hi jaap,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' 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/20200717-201923
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: h8300-allyesconfig (attached as .config)
compiler: h8300-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=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/hwmon/corsair_hydro_i_pro.c: In function 'check_succes':
   drivers/hwmon/corsair_hydro_i_pro.c:165:26: warning: left-hand operand of comma expression has no effect [-Wunused-value]
     165 | #define SUCCES_CODE (0x12, 0x34)
         |                          ^
   drivers/hwmon/corsair_hydro_i_pro.c:170:43: note: in expansion of macro 'SUCCES_CODE'
     170 |  char success[SUCCES_LENGTH] = { command, SUCCES_CODE };
         |                                           ^~~~~~~~~~~
   drivers/hwmon/corsair_hydro_i_pro.c: At top level:
>> drivers/hwmon/corsair_hydro_i_pro.c:175:5: warning: no previous prototype for 'set_fan_pwm_curve' [-Wmissing-prototypes]
     175 | int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
         |     ^~~~~~~~~~~~~~~~~
>> drivers/hwmon/corsair_hydro_i_pro.c:222:5: warning: no previous prototype for 'set_fan_target_rpm' [-Wmissing-prototypes]
     222 | int set_fan_target_rpm(struct hydro_i_pro_device *hdev,
         |     ^~~~~~~~~~~~~~~~~~
>> drivers/hwmon/corsair_hydro_i_pro.c:258:5: warning: no previous prototype for 'get_fan_current_rpm' [-Wmissing-prototypes]
     258 | int get_fan_current_rpm(struct hydro_i_pro_device *hdev,
         |     ^~~~~~~~~~~~~~~~~~~
>> drivers/hwmon/corsair_hydro_i_pro.c:292:5: warning: no previous prototype for 'set_fan_target_pwm' [-Wmissing-prototypes]
     292 | int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
         |     ^~~~~~~~~~~~~~~~~~
>> drivers/hwmon/corsair_hydro_i_pro.c:327:9: warning: no previous prototype for 'hwmon_is_visible' [-Wmissing-prototypes]
     327 | umode_t hwmon_is_visible(const void *d, enum hwmon_sensor_types type, u32 attr,
         |         ^~~~~~~~~~~~~~~~
>> drivers/hwmon/corsair_hydro_i_pro.c:481:5: warning: no previous prototype for 'hwmon_read' [-Wmissing-prototypes]
     481 | int hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
         |     ^~~~~~~~~~
>> drivers/hwmon/corsair_hydro_i_pro.c:561:6: warning: no previous prototype for 'does_fan_exist' [-Wmissing-prototypes]
     561 | bool does_fan_exist(struct hydro_i_pro_device *hdev, int channel)
         |      ^~~~~~~~~~~~~~
>> drivers/hwmon/corsair_hydro_i_pro.c:589:5: warning: no previous prototype for 'get_fan_count' [-Wmissing-prototypes]
     589 | int get_fan_count(struct hydro_i_pro_device *hdev)
         |     ^~~~~~~~~~~~~
>> drivers/hwmon/corsair_hydro_i_pro.c:598:6: warning: no previous prototype for 'hwmon_init' [-Wmissing-prototypes]
     598 | void hwmon_init(struct hydro_i_pro_device *hdev)
         |      ^~~~~~~~~~
   drivers/hwmon/corsair_hydro_i_pro.c: In function 'hwmon_init':
>> drivers/hwmon/corsair_hydro_i_pro.c:601:17: warning: variable 'hwmon_dev' set but not used [-Wunused-but-set-variable]
     601 |  struct device *hwmon_dev;
         |                 ^~~~~~~~~
   drivers/hwmon/corsair_hydro_i_pro.c: At top level:
>> drivers/hwmon/corsair_hydro_i_pro.c:671:5: warning: no previous prototype for 'init_device' [-Wmissing-prototypes]
     671 | int init_device(struct usb_device *udev)
         |     ^~~~~~~~~~~
   drivers/hwmon/corsair_hydro_i_pro.c: In function 'init_device':
>> drivers/hwmon/corsair_hydro_i_pro.c:681:3: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
     681 |   ;
         |   ^
   drivers/hwmon/corsair_hydro_i_pro.c: At top level:
>> drivers/hwmon/corsair_hydro_i_pro.c:688:5: warning: no previous prototype for 'deinit_device' [-Wmissing-prototypes]
     688 | int deinit_device(struct usb_device *udev)
         |     ^~~~~~~~~~~~~
   drivers/hwmon/corsair_hydro_i_pro.c:166:19: warning: 'SUCCESS' defined but not used [-Wunused-const-variable=]
     166 | static const char SUCCESS[SUCCES_LENGTH - 1] = { 0x12, 0x34 };
         |                   ^~~~~~~

vim +/set_fan_pwm_curve +175 drivers/hwmon/corsair_hydro_i_pro.c

   174	
 > 175	int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
   176			      struct hwmon_fan_data *fan_data,
   177			      struct curve_point point[7])
   178	{
   179		int retval;
   180		int wrote;
   181		int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
   182		int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
   183		unsigned char *send_buf = hdev->bulk_out_buffer;
   184		unsigned char *recv_buf = hdev->bulk_in_buffer;
   185	
   186		memcpy(fan_data->curve, point, sizeof(fan_data->curve));
   187	
   188		send_buf[0] = PWM_FAN_CURVE_CMD;
   189		send_buf[1] = fan_data->fan_channel;
   190		send_buf[2] = point[0].temp;
   191		send_buf[3] = point[1].temp;
   192		send_buf[4] = point[2].temp;
   193		send_buf[5] = point[3].temp;
   194		send_buf[6] = point[4].temp;
   195		send_buf[7] = point[5].temp;
   196		send_buf[8] = point[6].temp;
   197		send_buf[9] = point[0].pwm;
   198		send_buf[10] = point[1].pwm;
   199		send_buf[11] = point[2].pwm;
   200		send_buf[12] = point[3].pwm;
   201		send_buf[13] = point[4].pwm;
   202		send_buf[14] = point[5].pwm;
   203		send_buf[15] = point[6].pwm;
   204	
   205		retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 16, &wrote, 100);
   206		if (retval != 0)
   207			return retval;
   208	
   209		retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
   210		if (retval != 0)
   211			return retval;
   212	
   213		if (!check_succes(send_buf[0], recv_buf)) {
   214			dev_info(&hdev->udev->dev,
   215				 "[*] failed setting fan curve %d,%d,%d/%d\n",
   216				 recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
   217			return -EINVAL;
   218		}
   219		return 0;
   220	}
   221	
 > 222	int set_fan_target_rpm(struct hydro_i_pro_device *hdev,
   223			       struct hwmon_fan_data *fan_data, long val)
   224	{
   225		int retval;
   226		int wrote;
   227		int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
   228		int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
   229	
   230		unsigned char *send_buf = hdev->bulk_out_buffer;
   231		unsigned char *recv_buf = hdev->bulk_in_buffer;
   232	
   233		fan_data->fan_target = val;
   234		fan_data->fan_pwm_target = 0;
   235	
   236		send_buf[0] = RPM_FAN_TARGET_CMD;
   237		send_buf[1] = fan_data->fan_channel;
   238		send_buf[2] = (fan_data->fan_target >> 8);
   239		send_buf[3] = fan_data->fan_target;
   240	
   241		retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote, 100);
   242		if (retval != 0)
   243			return retval;
   244	
   245		retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
   246		if (retval != 0)
   247			return retval;
   248	
   249		if (!check_succes(send_buf[0], recv_buf)) {
   250			dev_info(&hdev->udev->dev,
   251				 "[*] failed setting fan rpm %d,%d,%d/%d\n",
   252				 recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
   253			return -EINVAL;
   254		}
   255		return 0;
   256	}
   257	
 > 258	int get_fan_current_rpm(struct hydro_i_pro_device *hdev,
   259				struct hwmon_fan_data *fan_data, long *val)
   260	{
   261		int retval;
   262		int wrote;
   263		int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
   264		int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
   265	
   266		unsigned char *send_buf = hdev->bulk_out_buffer;
   267		unsigned char *recv_buf = hdev->bulk_in_buffer;
   268	
   269		send_buf[0] = PWM_GET_CURRENT_CMD;
   270		send_buf[1] = fan_data->fan_channel;
   271	
   272		retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 2, &wrote, 100);
   273		if (retval != 0)
   274			return retval;
   275	
   276		retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
   277		if (retval != 0)
   278			return retval;
   279	
   280		if (!check_succes(send_buf[0], recv_buf) ||
   281		    recv_buf[3] != fan_data->fan_channel) {
   282			dev_info(&hdev->udev->dev,
   283				 "[*] failed retrieving fan rmp %d,%d,%d/%d\n",
   284				 recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
   285			return -EINVAL;
   286		}
   287	
   288		*val = ((recv_buf[4]) << 8) + recv_buf[5];
   289		return 0;
   290	}
   291	
 > 292	int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
   293			       struct hwmon_fan_data *fan_data, long val)
   294	{
   295		int retval;
   296		int wrote;
   297		int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
   298		int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
   299	
   300		unsigned char *send_buf = hdev->bulk_out_buffer;
   301		unsigned char *recv_buf = hdev->bulk_in_buffer;
   302	
   303		fan_data->fan_pwm_target = val;
   304		fan_data->fan_target = 0;
   305	
   306		send_buf[0] = PWM_FAN_TARGET_CMD;
   307		send_buf[1] = fan_data->fan_channel;
   308		send_buf[3] = fan_data->fan_pwm_target;
   309	
   310		retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote, 100);
   311		if (retval != 0)
   312			return retval;
   313	
   314		retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
   315		if (retval != 0)
   316			return retval;
   317	
   318		if (!check_succes(send_buf[0], recv_buf)) {
   319			dev_info(&hdev->udev->dev,
   320				 "[*] failed setting fan pwm %d,%d,%d/%d\n",
   321				 recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
   322			return -EINVAL;
   323		}
   324		return 0;
   325	}
   326	
 > 327	umode_t hwmon_is_visible(const void *d, enum hwmon_sensor_types type, u32 attr,
   328				 int channel)
   329	{
   330		switch (type) {
   331		case hwmon_fan:
   332			switch (attr) {
   333			case hwmon_fan_input:
   334				return 0444;
   335				break;
   336			case hwmon_fan_target:
   337				return 0644;
   338				break;
   339			case hwmon_fan_min:
   340				return 0444;
   341				break;
   342			default:
   343				break;
   344			}
   345			break;
   346		case hwmon_pwm:
   347			switch (attr) {
   348			case hwmon_pwm_input:
   349				return 0200;
   350				break;
   351			case hwmon_pwm_enable:
   352				return 0644;
   353				break;
   354			default:
   355				break;
   356			}
   357			break;
   358		default:
   359			break;
   360		}
   361		return 0;
   362	}
   363	

---
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: 55765 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-17 12:16 [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
  2020-07-17 21:41 ` kernel test robot
@ 2020-07-17 23:15 ` Guenter Roeck
  2020-07-18  9:33   ` jaap aarts
  2020-07-19  0:30 ` kernel test robot
  2 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2020-07-17 23:15 UTC (permalink / raw)
  To: jaap aarts, Jean Delvare, linux-hwmon, linux-usb

On 7/17/20 5:16 AM, jaap aarts wrote:
> Adds fan/pwm support for H100i platinum.
> Custom temp/fan curves are not supported, however
> the presets found in the proprietary drivers are available.
> 
> Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>

Most of my comments have not been addressed.
Change log is missing.
0-day feedback has not been adressed.

Guenter

> ---
>  drivers/hwmon/Kconfig               |   6 +
>  drivers/hwmon/Makefile              |   1 +
>  drivers/hwmon/corsair_hydro_i_pro.c | 791 ++++++++++++++++++++++++++++
>  3 files changed, 798 insertions(+)
>  create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 288ae9f63588..9831a40fb05f 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_CORSAIR_HYDRO_I_PRO
> +	tristate "Corsair hydro HXXXi pro driver"
> +	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..43bf52d8d365
> --- /dev/null
> +++ b/drivers/hwmon/corsair_hydro_i_pro.c
> @@ -0,0 +1,791 @@
> +// 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 hydro_i_pro_device {
> +	struct usb_device *udev;
> +
> +	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 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 hydro_i_pro_device *hdev;
> +	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
> +
> +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 const char SUCCESS[SUCCES_LENGTH - 1] = { 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;
> +}
> +
> +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, 100);
> +	if (retval != 0)
> +		return retval;
> +
> +	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
> +	if (retval != 0)
> +		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;
> +}
> +
> +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, 100);
> +	if (retval != 0)
> +		return retval;
> +
> +	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> +	if (retval != 0)
> +		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;
> +}
> +
> +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, 100);
> +	if (retval != 0)
> +		return retval;
> +
> +	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> +	if (retval != 0)
> +		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;
> +}
> +
> +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, 100);
> +	if (retval != 0)
> +		return retval;
> +
> +	retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
> +	if (retval != 0)
> +		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;
> +}
> +
> +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;
> +
> +	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, quiet_curve);
> +				break;
> +			case 3:
> +				set_fan_pwm_curve(hdev, fan_data,
> +						  balanced_curve);
> +				break;
> +			case 4:
> +				set_fan_pwm_curve(hdev, fan_data,
> +						  extreme_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;
> +}
> +
> +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 -EAGAIN;
> +
> +	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;
> +}
> +
> +#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 i_pro_ops = {
> +	.is_visible = hwmon_is_visible,
> +	.read = hwmon_read,
> +	.write = hwmon_write,
> +};
> +
> +bool does_fan_exist(struct hydro_i_pro_device *hdev, int channel)
> +{
> +	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] = RPM_FAN_TARGET_CMD;
> +	send_buf[1] = channel;
> +	send_buf[2] = (600 >> 8);
> +	send_buf[3] = (unsigned char)600;
> +
> +	retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> +	if (retval != 0)
> +		return false;
> +
> +	retval = usb_bulk_msg(hdev->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 hydro_i_pro_device *hdev)
> +{
> +	int fan;
> +
> +	for (fan = 0; does_fan_exist(hdev, fan); fan += 1)
> +		;
> +	return fan;
> +}
> +
> +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);
> +	//Allocate the info table
> +	struct hwmon_channel_info **aio_info =
> +		devm_kzalloc(&hdev->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(&hdev->udev->dev,
> +					sizeof(u32) * (data->channel_count + 1),
> +					GFP_KERNEL);
> +	u32 *pwms_config = devm_kzalloc(&hdev->udev->dev,
> +					sizeof(u32) * (data->channel_count + 1),
> +					GFP_KERNEL);
> +
> +	data->channel_count = get_fan_count(hdev); //amount of fans
> +	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;
> +		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(&hdev->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(&hdev->udev->dev,
> +			     sizeof(struct hwmon_channel_info), GFP_KERNEL);
> +	aio_info[1]->type = hwmon_pwm;
> +	aio_info[1]->config = pwms_config;
> +
> +	hwmon_info->ops = &i_pro_ops;
> +	hwmon_info->info = (const struct hwmon_channel_info **)aio_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);
> +
> +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 != 0)
> +		;
> +
> +	retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> +				 0x0002, 0x0000, 0, 0, 0);
> +	return retval;
> +}
> +
> +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;
> +	}
> +
> +	retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> +					   &bulk_out, NULL, NULL);
> +	if (retval != 0)
> +		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");
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-17 23:15 ` Guenter Roeck
@ 2020-07-18  9:33   ` jaap aarts
  2020-07-18 15:10     ` Guenter Roeck
  0 siblings, 1 reply; 10+ messages in thread
From: jaap aarts @ 2020-07-18  9:33 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, linux-usb

On Sat, 18 Jul 2020 at 01:15, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/17/20 5:16 AM, jaap aarts wrote:
> > Adds fan/pwm support for H100i platinum.
> > Custom temp/fan curves are not supported, however
> > the presets found in the proprietary drivers are available.
> >
> > Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
>
> Most of my comments have not been addressed.

I replied to your comments, everything I mentioned as fixed/changed
has been addressed in this new patch. You didn't respond to that any
further even though you did reply to my other email. So I thought I
was supposed to just send in v2.

> Change log is missing.

I don't know what you mean, I have a from line, I have a one-line
description of the patch, the email subject just like some other
patches, I have a multi-line description, and a signed-off-by line.
According to the linux docs
(https://www.kernel.org/doc/html/latest/process/5.Posting.html?highlight=changelog#patch-formatting-and-changelogs)
This is what a changelog should be.
If you mean changelog from v1, I mailed about all the fixed/changed
things in this patch. I also send a follow-up email noting that after
some research I found out that this driver should NOT work with
all asetek gen6 based coolers, and that I changed the scope of
the driver to just a single line of corsair products.

> 0-day feedback has not been adressed.

True, I did not fix all of those, I wasn't sure how to take all of them
since it was a long list.
The rest of the 0-day feedback will be fixed next round.

>
> Guenter


>
> > ---
> >  drivers/hwmon/Kconfig               |   6 +
> >  drivers/hwmon/Makefile              |   1 +
> >  drivers/hwmon/corsair_hydro_i_pro.c | 791 ++++++++++++++++++++++++++++
> >  3 files changed, 798 insertions(+)
> >  create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 288ae9f63588..9831a40fb05f 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_CORSAIR_HYDRO_I_PRO
> > +     tristate "Corsair hydro HXXXi pro driver"
> > +     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..43bf52d8d365
> > --- /dev/null
> > +++ b/drivers/hwmon/corsair_hydro_i_pro.c
> > @@ -0,0 +1,791 @@
> > +// 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 hydro_i_pro_device {
> > +     struct usb_device *udev;
> > +
> > +     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 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 hydro_i_pro_device *hdev;
> > +     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
> > +
> > +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 const char SUCCESS[SUCCES_LENGTH - 1] = { 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;
> > +}
> > +
> > +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, 100);
> > +     if (retval != 0)
> > +             return retval;
> > +
> > +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
> > +     if (retval != 0)
> > +             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;
> > +}
> > +
> > +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, 100);
> > +     if (retval != 0)
> > +             return retval;
> > +
> > +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> > +     if (retval != 0)
> > +             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;
> > +}
> > +
> > +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, 100);
> > +     if (retval != 0)
> > +             return retval;
> > +
> > +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> > +     if (retval != 0)
> > +             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;
> > +}
> > +
> > +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, 100);
> > +     if (retval != 0)
> > +             return retval;
> > +
> > +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
> > +     if (retval != 0)
> > +             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;
> > +}
> > +
> > +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;
> > +
> > +     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, quiet_curve);
> > +                             break;
> > +                     case 3:
> > +                             set_fan_pwm_curve(hdev, fan_data,
> > +                                               balanced_curve);
> > +                             break;
> > +                     case 4:
> > +                             set_fan_pwm_curve(hdev, fan_data,
> > +                                               extreme_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;
> > +}
> > +
> > +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 -EAGAIN;
> > +
> > +     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;
> > +}
> > +
> > +#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 i_pro_ops = {
> > +     .is_visible = hwmon_is_visible,
> > +     .read = hwmon_read,
> > +     .write = hwmon_write,
> > +};
> > +
> > +bool does_fan_exist(struct hydro_i_pro_device *hdev, int channel)
> > +{
> > +     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] = RPM_FAN_TARGET_CMD;
> > +     send_buf[1] = channel;
> > +     send_buf[2] = (600 >> 8);
> > +     send_buf[3] = (unsigned char)600;
> > +
> > +     retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> > +     if (retval != 0)
> > +             return false;
> > +
> > +     retval = usb_bulk_msg(hdev->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 hydro_i_pro_device *hdev)
> > +{
> > +     int fan;
> > +
> > +     for (fan = 0; does_fan_exist(hdev, fan); fan += 1)
> > +             ;
> > +     return fan;
> > +}
> > +
> > +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);
> > +     //Allocate the info table
> > +     struct hwmon_channel_info **aio_info =
> > +             devm_kzalloc(&hdev->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(&hdev->udev->dev,
> > +                                     sizeof(u32) * (data->channel_count + 1),
> > +                                     GFP_KERNEL);
> > +     u32 *pwms_config = devm_kzalloc(&hdev->udev->dev,
> > +                                     sizeof(u32) * (data->channel_count + 1),
> > +                                     GFP_KERNEL);
> > +
> > +     data->channel_count = get_fan_count(hdev); //amount of fans
> > +     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;
> > +             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(&hdev->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(&hdev->udev->dev,
> > +                          sizeof(struct hwmon_channel_info), GFP_KERNEL);
> > +     aio_info[1]->type = hwmon_pwm;
> > +     aio_info[1]->config = pwms_config;
> > +
> > +     hwmon_info->ops = &i_pro_ops;
> > +     hwmon_info->info = (const struct hwmon_channel_info **)aio_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);
> > +
> > +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 != 0)
> > +             ;
> > +
> > +     retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> > +                              0x0002, 0x0000, 0, 0, 0);
> > +     return retval;
> > +}
> > +
> > +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;
> > +     }
> > +
> > +     retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> > +                                        &bulk_out, NULL, NULL);
> > +     if (retval != 0)
> > +             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");
> >
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-18  9:33   ` jaap aarts
@ 2020-07-18 15:10     ` Guenter Roeck
  2020-07-18 19:51       ` jaap aarts
  2020-07-21 10:10       ` jaap aarts
  0 siblings, 2 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-07-18 15:10 UTC (permalink / raw)
  To: jaap aarts; +Cc: Jean Delvare, linux-hwmon, linux-usb

On 7/18/20 2:33 AM, jaap aarts wrote:
> On Sat, 18 Jul 2020 at 01:15, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 7/17/20 5:16 AM, jaap aarts wrote:
>>> Adds fan/pwm support for H100i platinum.
>>> Custom temp/fan curves are not supported, however
>>> the presets found in the proprietary drivers are available.
>>>
>>> Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
>>
>> Most of my comments have not been addressed.
> 
> I replied to your comments, everything I mentioned as fixed/changed
> has been addressed in this new patch. You didn't respond to that any
> further even though you did reply to my other email. So I thought I
> was supposed to just send in v2.
> 

This is incorrect. I did say, for example, that I won't accept things
like setting fan curves, and that the driver should provide sets of
{temperature, pwm} attributes instead. There are several other comments
which were not addressed, such as not using C++ comments and using
"if (retval)" instead of "if (retval != 0)". There is still a
100-second timeout. I didn't check any further, but I think that
is sufficient to say that several of my comments were not addressed.

>> Change log is missing.
> 
> I don't know what you mean, I have a from line, I have a one-line
> description of the patch, the email subject just like some other
> patches, I have a multi-line description, and a signed-off-by line.
> According to the linux docs
> (https://www.kernel.org/doc/html/latest/process/5.Posting.html?highlight=changelog#patch-formatting-and-changelogs)
> This is what a changelog should be.
> If you mean changelog from v1, I mailed about all the fixed/changed
> things in this patch. I also send a follow-up email noting that after
> some research I found out that this driver should NOT work with
> all asetek gen6 based coolers, and that I changed the scope of
> the driver to just a single line of corsair products.
> 

I don't see anything along the line of

v2:
- made this change
- made that change

in this patch.

>> 0-day feedback has not been adressed.
> 
> True, I did not fix all of those, I wasn't sure how to take all of them
> since it was a long list.

I am not entirely sure how to respond to that. What would be the point
of reviewing the code if a simple compile and sanity check reveals
several errors, and you state yourself that you did not fix them all ?

Guenter

> The rest of the 0-day feedback will be fixed next round.
> 
>>
>> Guenter
> 
> 
>>
>>> ---
>>>  drivers/hwmon/Kconfig               |   6 +
>>>  drivers/hwmon/Makefile              |   1 +
>>>  drivers/hwmon/corsair_hydro_i_pro.c | 791 ++++++++++++++++++++++++++++
>>>  3 files changed, 798 insertions(+)
>>>  create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 288ae9f63588..9831a40fb05f 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_CORSAIR_HYDRO_I_PRO
>>> +     tristate "Corsair hydro HXXXi pro driver"
>>> +     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..43bf52d8d365
>>> --- /dev/null
>>> +++ b/drivers/hwmon/corsair_hydro_i_pro.c
>>> @@ -0,0 +1,791 @@
>>> +// 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 hydro_i_pro_device {
>>> +     struct usb_device *udev;
>>> +
>>> +     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 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 hydro_i_pro_device *hdev;
>>> +     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
>>> +
>>> +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 const char SUCCESS[SUCCES_LENGTH - 1] = { 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;
>>> +}
>>> +
>>> +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, 100);
>>> +     if (retval != 0)
>>> +             return retval;
>>> +
>>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
>>> +     if (retval != 0)
>>> +             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;
>>> +}
>>> +
>>> +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, 100);
>>> +     if (retval != 0)
>>> +             return retval;
>>> +
>>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
>>> +     if (retval != 0)
>>> +             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;
>>> +}
>>> +
>>> +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, 100);
>>> +     if (retval != 0)
>>> +             return retval;
>>> +
>>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
>>> +     if (retval != 0)
>>> +             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;
>>> +}
>>> +
>>> +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, 100);
>>> +     if (retval != 0)
>>> +             return retval;
>>> +
>>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
>>> +     if (retval != 0)
>>> +             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;
>>> +}
>>> +
>>> +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;
>>> +
>>> +     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, quiet_curve);
>>> +                             break;
>>> +                     case 3:
>>> +                             set_fan_pwm_curve(hdev, fan_data,
>>> +                                               balanced_curve);
>>> +                             break;
>>> +                     case 4:
>>> +                             set_fan_pwm_curve(hdev, fan_data,
>>> +                                               extreme_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;
>>> +}
>>> +
>>> +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 -EAGAIN;
>>> +
>>> +     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;
>>> +}
>>> +
>>> +#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 i_pro_ops = {
>>> +     .is_visible = hwmon_is_visible,
>>> +     .read = hwmon_read,
>>> +     .write = hwmon_write,
>>> +};
>>> +
>>> +bool does_fan_exist(struct hydro_i_pro_device *hdev, int channel)
>>> +{
>>> +     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] = RPM_FAN_TARGET_CMD;
>>> +     send_buf[1] = channel;
>>> +     send_buf[2] = (600 >> 8);
>>> +     send_buf[3] = (unsigned char)600;
>>> +
>>> +     retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote, 100);
>>> +     if (retval != 0)
>>> +             return false;
>>> +
>>> +     retval = usb_bulk_msg(hdev->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 hydro_i_pro_device *hdev)
>>> +{
>>> +     int fan;
>>> +
>>> +     for (fan = 0; does_fan_exist(hdev, fan); fan += 1)
>>> +             ;
>>> +     return fan;
>>> +}
>>> +
>>> +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);
>>> +     //Allocate the info table
>>> +     struct hwmon_channel_info **aio_info =
>>> +             devm_kzalloc(&hdev->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(&hdev->udev->dev,
>>> +                                     sizeof(u32) * (data->channel_count + 1),
>>> +                                     GFP_KERNEL);
>>> +     u32 *pwms_config = devm_kzalloc(&hdev->udev->dev,
>>> +                                     sizeof(u32) * (data->channel_count + 1),
>>> +                                     GFP_KERNEL);
>>> +
>>> +     data->channel_count = get_fan_count(hdev); //amount of fans
>>> +     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;
>>> +             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(&hdev->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(&hdev->udev->dev,
>>> +                          sizeof(struct hwmon_channel_info), GFP_KERNEL);
>>> +     aio_info[1]->type = hwmon_pwm;
>>> +     aio_info[1]->config = pwms_config;
>>> +
>>> +     hwmon_info->ops = &i_pro_ops;
>>> +     hwmon_info->info = (const struct hwmon_channel_info **)aio_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);
>>> +
>>> +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 != 0)
>>> +             ;
>>> +
>>> +     retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
>>> +                              0x0002, 0x0000, 0, 0, 0);
>>> +     return retval;
>>> +}
>>> +
>>> +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;
>>> +     }
>>> +
>>> +     retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
>>> +                                        &bulk_out, NULL, NULL);
>>> +     if (retval != 0)
>>> +             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");
>>>
>>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-18 15:10     ` Guenter Roeck
@ 2020-07-18 19:51       ` jaap aarts
  2020-07-21 10:10       ` jaap aarts
  1 sibling, 0 replies; 10+ messages in thread
From: jaap aarts @ 2020-07-18 19:51 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, linux-usb

On Sat, 18 Jul 2020 at 17:10, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/18/20 2:33 AM, jaap aarts wrote:
> > On Sat, 18 Jul 2020 at 01:15, Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 7/17/20 5:16 AM, jaap aarts wrote:
> >>> Adds fan/pwm support for H100i platinum.
> >>> Custom temp/fan curves are not supported, however
> >>> the presets found in the proprietary drivers are available.
> >>>
> >>> Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
> >>
> >> Most of my comments have not been addressed.
> >
> > I replied to your comments, everything I mentioned as fixed/changed
> > has been addressed in this new patch. You didn't respond to that any
> > further even though you did reply to my other email. So I thought I
> > was supposed to just send in v2.
> >
>
> This is incorrect. I did say, for example, that I won't accept things
> like setting fan curves, and that the driver should provide sets of
> {temperature, pwm} attributes instead.
I replied to this comment saying at least a default was needed for
setting pwm{1-*}_enable to 0, I didnt get any response on that so I could
not make a decision on what to so about this.
> There are several other comments
> which were not addressed, such as not using C++ comments
I replied asking what exactly C++ comments are.
I think you mean C++ style comments, which i can easily find out what
they are, I tried googling C++ comments and that does not give any
useful information.
> and using  "if (retval)" instead of "if (retval != 0)".
I asked if it was ok to be more verbose, I personally prefer to not treat
ints as boolean values. Asyou had not replied yet I did not see purpose
in changing this.
> There is still a 100-second timeout.
This is indeed my bad,  there were a lot small changes im sorry I
missed one.

> I didn't check any further, but I think that
> is sufficient to say that several of my comments were not addressed.
>
> >> Change log is missing.
> >
> > I don't know what you mean, I have a from line, I have a one-line
> > description of the patch, the email subject just like some other
> > patches, I have a multi-line description, and a signed-off-by line.
> > According to the linux docs
> > (https://www.kernel.org/doc/html/latest/process/5.Posting.html?highlight=changelog#patch-formatting-and-changelogs)
> > This is what a changelog should be.
> > If you mean changelog from v1, I mailed about all the fixed/changed
> > things in this patch. I also send a follow-up email noting that after
> > some research I found out that this driver should NOT work with
> > all asetek gen6 based coolers, and that I changed the scope of
> > the driver to just a single line of corsair products.
> >
>
> I don't see anything along the line of
>
> v2:
> - made this change
> - made that change
>
> in this patch.

I have not read anything about that in the documentation, for V3 I
will add a changelog.

> >> 0-day feedback has not been adressed.
> >
> > True, I did not fix all of those, I wasn't sure how to take all of them
> > since it was a long list.
>
> I am not entirely sure how to respond to that. What would be the point
> of reviewing the code if a simple compile and sanity check reveals
> several errors, and you state yourself that you did not fix them all ?

My code compiled without warnings, the linux format checker told me
to put "()" around my `#define SUCCES_CODE 0x12, 0x34`. I had no
idea this would cause anything to change, I was under the
understanding this was just nice syntax-wise so I didn't try to
recompile. (it's a format checker after all, format doesn't change
compiler output.)

I can send in V3 with all the 0-day problems fixed and remove `()`
around the define? That would be a pretty insignificant change but
if you prefer that I am ok with that.

> Guenter
>
> > The rest of the 0-day feedback will be fixed next round.
> >
> >>
> >> Guenter
> >
> >
> >>
> >>> ---
> >>>  drivers/hwmon/Kconfig               |   6 +
> >>>  drivers/hwmon/Makefile              |   1 +
> >>>  drivers/hwmon/corsair_hydro_i_pro.c | 791 ++++++++++++++++++++++++++++
> >>>  3 files changed, 798 insertions(+)
> >>>  create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c
> >>>
> >>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >>> index 288ae9f63588..9831a40fb05f 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_CORSAIR_HYDRO_I_PRO
> >>> +     tristate "Corsair hydro HXXXi pro driver"
> >>> +     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..43bf52d8d365
> >>> --- /dev/null
> >>> +++ b/drivers/hwmon/corsair_hydro_i_pro.c
> >>> @@ -0,0 +1,791 @@
> >>> +// 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 hydro_i_pro_device {
> >>> +     struct usb_device *udev;
> >>> +
> >>> +     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 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 hydro_i_pro_device *hdev;
> >>> +     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
> >>> +
> >>> +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 const char SUCCESS[SUCCES_LENGTH - 1] = { 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;
> >>> +}
> >>> +
> >>> +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, 100);
> >>> +     if (retval != 0)
> >>> +             return retval;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
> >>> +     if (retval != 0)
> >>> +             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;
> >>> +}
> >>> +
> >>> +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, 100);
> >>> +     if (retval != 0)
> >>> +             return retval;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> >>> +     if (retval != 0)
> >>> +             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;
> >>> +}
> >>> +
> >>> +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, 100);
> >>> +     if (retval != 0)
> >>> +             return retval;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> >>> +     if (retval != 0)
> >>> +             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;
> >>> +}
> >>> +
> >>> +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, 100);
> >>> +     if (retval != 0)
> >>> +             return retval;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
> >>> +     if (retval != 0)
> >>> +             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;
> >>> +}
> >>> +
> >>> +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;
> >>> +
> >>> +     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, quiet_curve);
> >>> +                             break;
> >>> +                     case 3:
> >>> +                             set_fan_pwm_curve(hdev, fan_data,
> >>> +                                               balanced_curve);
> >>> +                             break;
> >>> +                     case 4:
> >>> +                             set_fan_pwm_curve(hdev, fan_data,
> >>> +                                               extreme_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;
> >>> +}
> >>> +
> >>> +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 -EAGAIN;
> >>> +
> >>> +     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;
> >>> +}
> >>> +
> >>> +#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 i_pro_ops = {
> >>> +     .is_visible = hwmon_is_visible,
> >>> +     .read = hwmon_read,
> >>> +     .write = hwmon_write,
> >>> +};
> >>> +
> >>> +bool does_fan_exist(struct hydro_i_pro_device *hdev, int channel)
> >>> +{
> >>> +     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] = RPM_FAN_TARGET_CMD;
> >>> +     send_buf[1] = channel;
> >>> +     send_buf[2] = (600 >> 8);
> >>> +     send_buf[3] = (unsigned char)600;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> >>> +     if (retval != 0)
> >>> +             return false;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->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 hydro_i_pro_device *hdev)
> >>> +{
> >>> +     int fan;
> >>> +
> >>> +     for (fan = 0; does_fan_exist(hdev, fan); fan += 1)
> >>> +             ;
> >>> +     return fan;
> >>> +}
> >>> +
> >>> +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);
> >>> +     //Allocate the info table
> >>> +     struct hwmon_channel_info **aio_info =
> >>> +             devm_kzalloc(&hdev->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(&hdev->udev->dev,
> >>> +                                     sizeof(u32) * (data->channel_count + 1),
> >>> +                                     GFP_KERNEL);
> >>> +     u32 *pwms_config = devm_kzalloc(&hdev->udev->dev,
> >>> +                                     sizeof(u32) * (data->channel_count + 1),
> >>> +                                     GFP_KERNEL);
> >>> +
> >>> +     data->channel_count = get_fan_count(hdev); //amount of fans
> >>> +     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;
> >>> +             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(&hdev->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(&hdev->udev->dev,
> >>> +                          sizeof(struct hwmon_channel_info), GFP_KERNEL);
> >>> +     aio_info[1]->type = hwmon_pwm;
> >>> +     aio_info[1]->config = pwms_config;
> >>> +
> >>> +     hwmon_info->ops = &i_pro_ops;
> >>> +     hwmon_info->info = (const struct hwmon_channel_info **)aio_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);
> >>> +
> >>> +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 != 0)
> >>> +             ;
> >>> +
> >>> +     retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> >>> +                              0x0002, 0x0000, 0, 0, 0);
> >>> +     return retval;
> >>> +}
> >>> +
> >>> +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;
> >>> +     }
> >>> +
> >>> +     retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> >>> +                                        &bulk_out, NULL, NULL);
> >>> +     if (retval != 0)
> >>> +             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");
> >>>
> >>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-17 12:16 [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
  2020-07-17 21:41 ` kernel test robot
  2020-07-17 23:15 ` Guenter Roeck
@ 2020-07-19  0:30 ` kernel test robot
  2020-07-19  9:38   ` jaap aarts
  2 siblings, 1 reply; 10+ messages in thread
From: kernel test robot @ 2020-07-19  0:30 UTC (permalink / raw)
  To: jaap aarts, Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb
  Cc: kbuild-all, jaap aarts


[-- Attachment #1: Type: text/plain, Size: 2237 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-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' 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/20200717-201923
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: xtensa-randconfig-r011-20200719 (attached as .config)
compiler: xtensa-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=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "usb_deregister" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_register_driver" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_put_dev" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_put_intf" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_get_intf" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_get_dev" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_find_common_endpoints" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_control_msg" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_autopm_put_interface" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_autopm_get_interface" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> ERROR: modpost: "usb_bulk_msg" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!

---
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: 29196 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-19  0:30 ` kernel test robot
@ 2020-07-19  9:38   ` jaap aarts
  2020-07-22  8:16     ` Xia, Hui
  0 siblings, 1 reply; 10+ messages in thread
From: jaap aarts @ 2020-07-19  9:38 UTC (permalink / raw)
  To: kernel test robot
  Cc: Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb, kbuild-all

I tried to reproduce this, but I got the error:
xtensa-linux-gcc: error: missing argument to '-Wframe-larger-than='
at the last step using the provided config.

Jaap Aarts

On Sun, 19 Jul 2020 at 02:31, kernel test robot <lkp@intel.com> wrote:
>
> 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-20200717]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' 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/20200717-201923
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
> config: xtensa-randconfig-r011-20200719 (attached as .config)
> compiler: xtensa-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=xtensa
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>, old ones prefixed by <<):
>
> >> ERROR: modpost: "usb_deregister" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_register_driver" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_put_dev" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_put_intf" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_get_intf" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_get_dev" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_find_common_endpoints" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_control_msg" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_autopm_put_interface" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_autopm_get_interface" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
> >> ERROR: modpost: "usb_bulk_msg" [drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-18 15:10     ` Guenter Roeck
  2020-07-18 19:51       ` jaap aarts
@ 2020-07-21 10:10       ` jaap aarts
  1 sibling, 0 replies; 10+ messages in thread
From: jaap aarts @ 2020-07-21 10:10 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Jean Delvare, linux-hwmon, linux-usb

Hello Guenter,

Is there still interest in this driver? I got a way of not needing the
dynamic clusterfuck in `hwmon_init` and could send in v3
with that, the 0day issues fixed and remove the 100s timeout.

I send this message in private by accident, sorry about that.

Jaap Aarts

On Sat, 18 Jul 2020 at 17:10, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/18/20 2:33 AM, jaap aarts wrote:
> > On Sat, 18 Jul 2020 at 01:15, Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 7/17/20 5:16 AM, jaap aarts wrote:
> >>> Adds fan/pwm support for H100i platinum.
> >>> Custom temp/fan curves are not supported, however
> >>> the presets found in the proprietary drivers are available.
> >>>
> >>> Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
> >>
> >> Most of my comments have not been addressed.
> >
> > I replied to your comments, everything I mentioned as fixed/changed
> > has been addressed in this new patch. You didn't respond to that any
> > further even though you did reply to my other email. So I thought I
> > was supposed to just send in v2.
> >
>
> This is incorrect. I did say, for example, that I won't accept things
> like setting fan curves, and that the driver should provide sets of
> {temperature, pwm} attributes instead. There are several other comments
> which were not addressed, such as not using C++ comments and using
> "if (retval)" instead of "if (retval != 0)". There is still a
> 100-second timeout. I didn't check any further, but I think that
> is sufficient to say that several of my comments were not addressed.
>
> >> Change log is missing.
> >
> > I don't know what you mean, I have a from line, I have a one-line
> > description of the patch, the email subject just like some other
> > patches, I have a multi-line description, and a signed-off-by line.
> > According to the linux docs
> > (https://www.kernel.org/doc/html/latest/process/5.Posting.html?highlight=changelog#patch-formatting-and-changelogs)
> > This is what a changelog should be.
> > If you mean changelog from v1, I mailed about all the fixed/changed
> > things in this patch. I also send a follow-up email noting that after
> > some research I found out that this driver should NOT work with
> > all asetek gen6 based coolers, and that I changed the scope of
> > the driver to just a single line of corsair products.
> >
>
> I don't see anything along the line of
>
> v2:
> - made this change
> - made that change
>
> in this patch.
>
> >> 0-day feedback has not been adressed.
> >
> > True, I did not fix all of those, I wasn't sure how to take all of them
> > since it was a long list.
>
> I am not entirely sure how to respond to that. What would be the point
> of reviewing the code if a simple compile and sanity check reveals
> several errors, and you state yourself that you did not fix them all ?
>
> Guenter
>
> > The rest of the 0-day feedback will be fixed next round.
> >
> >>
> >> Guenter
> >
> >
> >>
> >>> ---
> >>>  drivers/hwmon/Kconfig               |   6 +
> >>>  drivers/hwmon/Makefile              |   1 +
> >>>  drivers/hwmon/corsair_hydro_i_pro.c | 791 ++++++++++++++++++++++++++++
> >>>  3 files changed, 798 insertions(+)
> >>>  create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c
> >>>
> >>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >>> index 288ae9f63588..9831a40fb05f 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_CORSAIR_HYDRO_I_PRO
> >>> +     tristate "Corsair hydro HXXXi pro driver"
> >>> +     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..43bf52d8d365
> >>> --- /dev/null
> >>> +++ b/drivers/hwmon/corsair_hydro_i_pro.c
> >>> @@ -0,0 +1,791 @@
> >>> +// 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 hydro_i_pro_device {
> >>> +     struct usb_device *udev;
> >>> +
> >>> +     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 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 hydro_i_pro_device *hdev;
> >>> +     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
> >>> +
> >>> +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 const char SUCCESS[SUCCES_LENGTH - 1] = { 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;
> >>> +}
> >>> +
> >>> +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, 100);
> >>> +     if (retval != 0)
> >>> +             return retval;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote, 100);
> >>> +     if (retval != 0)
> >>> +             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;
> >>> +}
> >>> +
> >>> +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, 100);
> >>> +     if (retval != 0)
> >>> +             return retval;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> >>> +     if (retval != 0)
> >>> +             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;
> >>> +}
> >>> +
> >>> +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, 100);
> >>> +     if (retval != 0)
> >>> +             return retval;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100);
> >>> +     if (retval != 0)
> >>> +             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;
> >>> +}
> >>> +
> >>> +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, 100);
> >>> +     if (retval != 0)
> >>> +             return retval;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote, 100000);
> >>> +     if (retval != 0)
> >>> +             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;
> >>> +}
> >>> +
> >>> +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;
> >>> +
> >>> +     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, quiet_curve);
> >>> +                             break;
> >>> +                     case 3:
> >>> +                             set_fan_pwm_curve(hdev, fan_data,
> >>> +                                               balanced_curve);
> >>> +                             break;
> >>> +                     case 4:
> >>> +                             set_fan_pwm_curve(hdev, fan_data,
> >>> +                                               extreme_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;
> >>> +}
> >>> +
> >>> +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 -EAGAIN;
> >>> +
> >>> +     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;
> >>> +}
> >>> +
> >>> +#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 i_pro_ops = {
> >>> +     .is_visible = hwmon_is_visible,
> >>> +     .read = hwmon_read,
> >>> +     .write = hwmon_write,
> >>> +};
> >>> +
> >>> +bool does_fan_exist(struct hydro_i_pro_device *hdev, int channel)
> >>> +{
> >>> +     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] = RPM_FAN_TARGET_CMD;
> >>> +     send_buf[1] = channel;
> >>> +     send_buf[2] = (600 >> 8);
> >>> +     send_buf[3] = (unsigned char)600;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote, 100);
> >>> +     if (retval != 0)
> >>> +             return false;
> >>> +
> >>> +     retval = usb_bulk_msg(hdev->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 hydro_i_pro_device *hdev)
> >>> +{
> >>> +     int fan;
> >>> +
> >>> +     for (fan = 0; does_fan_exist(hdev, fan); fan += 1)
> >>> +             ;
> >>> +     return fan;
> >>> +}
> >>> +
> >>> +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);
> >>> +     //Allocate the info table
> >>> +     struct hwmon_channel_info **aio_info =
> >>> +             devm_kzalloc(&hdev->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(&hdev->udev->dev,
> >>> +                                     sizeof(u32) * (data->channel_count + 1),
> >>> +                                     GFP_KERNEL);
> >>> +     u32 *pwms_config = devm_kzalloc(&hdev->udev->dev,
> >>> +                                     sizeof(u32) * (data->channel_count + 1),
> >>> +                                     GFP_KERNEL);
> >>> +
> >>> +     data->channel_count = get_fan_count(hdev); //amount of fans
> >>> +     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;
> >>> +             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(&hdev->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(&hdev->udev->dev,
> >>> +                          sizeof(struct hwmon_channel_info), GFP_KERNEL);
> >>> +     aio_info[1]->type = hwmon_pwm;
> >>> +     aio_info[1]->config = pwms_config;
> >>> +
> >>> +     hwmon_info->ops = &i_pro_ops;
> >>> +     hwmon_info->info = (const struct hwmon_channel_info **)aio_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);
> >>> +
> >>> +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 != 0)
> >>> +             ;
> >>> +
> >>> +     retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> >>> +                              0x0002, 0x0000, 0, 0, 0);
> >>> +     return retval;
> >>> +}
> >>> +
> >>> +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;
> >>> +     }
> >>> +
> >>> +     retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> >>> +                                        &bulk_out, NULL, NULL);
> >>> +     if (retval != 0)
> >>> +             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");
> >>>
> >>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
  2020-07-19  9:38   ` jaap aarts
@ 2020-07-22  8:16     ` Xia, Hui
  0 siblings, 0 replies; 10+ messages in thread
From: Xia, Hui @ 2020-07-22  8:16 UTC (permalink / raw)
  To: jaap aarts, lkp
  Cc: Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb, kbuild-all



>-----Original Message-----
>From: jaap aarts <jaap.aarts1@gmail.com>
>Sent: 2020年7月19日 17:38
>To: kernel test robot <lkp@intel.com>
>Cc: Jean Delvare <jdelvare@suse.com>; Guenter Roeck <linux@roeck-us.net>;
>linux-hwmon@vger.kernel.org; linux-usb@vger.kernel.org; kbuild-
>all@lists.01.org
>Subject: Re: [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum
>
>I tried to reproduce this, but I got the error:
>xtensa-linux-gcc: error: missing argument to '-Wframe-larger-than='
>at the last step using the provided config.
Hi Jaap Aarts,

Sorry to not reproduce the error you met. You can try to reproduce the modpost error
With any kernel config that enable SENSORS_CORSAIR_HYDRO_I_PRO and disable USB.
The driver supposed to need add "depends on USB" in kconfig since it use usb API.

--Hui

>
>Jaap Aarts
>
>On Sun, 19 Jul 2020 at 02:31, kernel test robot <lkp@intel.com> wrote:
>>
>> 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-20200717] [If your patch is applied to the wrong git
>> tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' 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/20200717-201923
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
>hwmon-next
>> config: xtensa-randconfig-r011-20200719 (attached as .config)
>> compiler: xtensa-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=xtensa
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>, old ones prefixed by <<):
>>
>> >> ERROR: modpost: "usb_deregister" [drivers/hwmon/corsair_hydro_i_pro.ko]
>undefined!
>> >> ERROR: modpost: "usb_register_driver"
>[drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> >> ERROR: modpost: "usb_put_dev" [drivers/hwmon/corsair_hydro_i_pro.ko]
>undefined!
>> >> ERROR: modpost: "usb_put_intf" [drivers/hwmon/corsair_hydro_i_pro.ko]
>undefined!
>> >> ERROR: modpost: "usb_get_intf" [drivers/hwmon/corsair_hydro_i_pro.ko]
>undefined!
>> >> ERROR: modpost: "usb_get_dev" [drivers/hwmon/corsair_hydro_i_pro.ko]
>undefined!
>> >> ERROR: modpost: "usb_find_common_endpoints"
>[drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> >> ERROR: modpost: "usb_control_msg"
>[drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> >> ERROR: modpost: "usb_autopm_put_interface"
>[drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> >> ERROR: modpost: "usb_autopm_get_interface"
>[drivers/hwmon/corsair_hydro_i_pro.ko] undefined!
>> >> ERROR: modpost: "usb_bulk_msg" [drivers/hwmon/corsair_hydro_i_pro.ko]
>undefined!
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-17 12:16 [PATCH V2] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
2020-07-17 21:41 ` kernel test robot
2020-07-17 23:15 ` Guenter Roeck
2020-07-18  9:33   ` jaap aarts
2020-07-18 15:10     ` Guenter Roeck
2020-07-18 19:51       ` jaap aarts
2020-07-21 10:10       ` jaap aarts
2020-07-19  0:30 ` kernel test robot
2020-07-19  9:38   ` jaap aarts
2020-07-22  8:16     ` Xia, Hui

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org
	public-inbox-index linux-hwmon

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git