* [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
@ 2020-08-15 21:16 jaap aarts
2020-08-15 21:22 ` jaap aarts
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: jaap aarts @ 2020-08-15 21:16 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb,
Barnabás Pőcze
Cc: jaap aarts
Adds fan/pwm support for H100i platinum.
Custom temp/fan curves are not supported.
v4:
- fixed spelling
- more consistent use of uN and other inconsistencies
- moved from semaphore to mutex, fixing 2 locking bugs allong the way
- moved to memcmp vs strncmp
- now uses driver_info for the device configuration
- check input ranges for fan rpm/pwm
- fix default case
- off-by-one loop range
- improved naming and logging messages
- fixed unfreed hdev
- use module_usb_driver
- use fixed-sized array for rpm/pwm channels independant of device.
- use common function for the USB bulk messages.
Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
---
drivers/hwmon/Kconfig | 7 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/corsair_hydro_i_pro.c | 694 ++++++++++++++++++++++++++++
3 files changed, 702 insertions(+)
create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 288ae9f63588..f466b72d0f67 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -378,6 +378,13 @@ config SENSORS_ARM_SCPI
and power sensors available on ARM Ltd's SCP based platforms. The
actual number and type of sensors exported depend on the platform.
+config SENSORS_CORSAIR_HYDRO_I_PRO
+ tristate "Corsair hydro HXXXi pro driver"
+ depends on USB
+ help
+ If you say yes here you get support for the corsair hydro HXXXi pro
+ range of devices.
+
config SENSORS_ASB100
tristate "Asus ASB100 Bach"
depends on X86 && I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3e32c21f5efe..ec63294b3ef1 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_W83793) += w83793.o
obj-$(CONFIG_SENSORS_W83795) += w83795.o
obj-$(CONFIG_SENSORS_W83781D) += w83781d.o
obj-$(CONFIG_SENSORS_W83791D) += w83791d.o
+obj-$(CONFIG_SENSORS_CORSAIR_HYDRO_I_PRO) += corsair_hydro_i_pro.o
obj-$(CONFIG_SENSORS_AB8500) += abx500.o ab8500.o
obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o
diff --git a/drivers/hwmon/corsair_hydro_i_pro.c b/drivers/hwmon/corsair_hydro_i_pro.c
new file mode 100644
index 000000000000..f4dd435be7dd
--- /dev/null
+++ b/drivers/hwmon/corsair_hydro_i_pro.c
@@ -0,0 +1,694 @@
+// 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 partially reverse engineered by audiohacked
+ * https://github.com/audiohacked/OpendriverLink
+ */
+
+/*
+ * Supports following liquid coolers:
+ * H100i platinum
+ *
+ * Other products should work with this driver with slight modification.
+ *
+ * Note: platinum is the codename name for pro within the driver, so H100i platinum = H100i pro.
+ * But some products are actually called platinum, these are not intended to be supported (yet).
+ *
+ * Note: fan curve control has not been implemented
+ */
+
+#include <linux/errno.h>
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+
+struct device_config {
+ u16 vendor_id;
+ u16 product_id;
+ u8 fancount;
+ const char *name;
+ const struct hwmon_channel_info **hwmon_info;
+};
+
+struct hydro_i_pro_device {
+ struct usb_device *udev;
+
+ const struct device_config *config;
+
+ unsigned char *bulk_out_buffer;
+ unsigned char *bulk_in_buffer;
+ size_t bulk_out_size;
+ size_t bulk_in_size;
+ char bulk_in_endpointAddr;
+ char bulk_out_endpointAddr;
+
+ struct usb_interface *interface; /* the interface for this device */
+ struct mutex io_mutex;
+};
+
+#define max_fan_count 2
+#define max_pwm_channel_count max_fan_count
+
+struct hwmon_data {
+ struct hydro_i_pro_device *hdev;
+ int channel_count;
+ void *channel_data[max_pwm_channel_count];
+};
+
+struct curve_point {
+ u8 temp;
+ u8 pwm;
+};
+
+struct hwmon_fan_data {
+ u8 fan_channel;
+ u16 fan_target;
+ u8 fan_pwm_target;
+ u8 mode;
+ struct curve_point curve[7];
+};
+
+struct curve_point quiet_curve[] = {
+ {
+ .temp = 0x1F,
+ .pwm = 0x15,
+ },
+ {
+ .temp = 0x21,
+ .pwm = 0x1E,
+ },
+ {
+ .temp = 0x24,
+ .pwm = 0x25,
+ },
+ {
+ .temp = 0x27,
+ .pwm = 0x2D,
+ },
+ {
+ .temp = 0x29,
+ .pwm = 0x38,
+ },
+ {
+ .temp = 0x2C,
+ .pwm = 0x4A,
+ },
+ {
+ .temp = 0x2F,
+ .pwm = 0x64,
+ },
+};
+
+#define default_curve quiet_curve
+
+static const struct hwmon_channel_info *dual_fan[] = {
+ HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN,
+ HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN),
+ HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
+
+ NULL
+};
+
+static const struct device_config config_table[] = {
+ {
+ .vendor_id = 0x1b1c,
+ .product_id = 0x0c15,
+ .fancount = 2,
+ .name = "corsair_H100i_pro",
+ .hwmon_info = dual_fan,
+ },
+};
+
+#define BULK_TIMEOUT 100
+
+enum opcodes {
+ PWM_FAN_CURVE_CMD = 0x40,
+ PWM_GET_CURRENT_CMD = 0x41,
+ PWM_FAN_TARGET_CMD = 0x42,
+ RPM_FAN_TARGET_CMD = 0x43,
+};
+
+#define SUCCES_LENGTH 3
+#define SUCCES_CODE 0x12, 0x34
+
+static bool check_succes(enum opcodes command, char ret[SUCCES_LENGTH])
+{
+ char success[SUCCES_LENGTH] = { command, SUCCES_CODE };
+ return memcmp(ret, success, SUCCES_LENGTH) == 0;
+}
+
+static int acquire_lock(struct hydro_i_pro_device *hdev)
+{
+ int retval = usb_autopm_get_interface(hdev->interface);
+ if (retval)
+ return retval;
+
+ mutex_lock(&hdev->io_mutex);
+ return 0;
+}
+
+static int transfer_usb(struct hydro_i_pro_device *hdev,
+ unsigned char *send_buf, unsigned char *recv_buf,
+ int send_len, int recv_len)
+{
+ int retval;
+ int wrote;
+ int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
+ int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
+
+ retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, send_len, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+
+ retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
+ BULK_TIMEOUT);
+ if (retval)
+ return retval;
+ return 0;
+}
+
+static int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
+ struct hwmon_fan_data *fan_data,
+ struct curve_point point[7])
+{
+ int retval;
+ unsigned char *send_buf = hdev->bulk_out_buffer;
+ unsigned char *recv_buf = hdev->bulk_in_buffer;
+
+ memcpy(fan_data->curve, point, sizeof(struct curve_point) * 7);
+
+ 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 = transfer_usb(hdev, send_buf, recv_buf, 16, 4);
+ if (retval)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf)) {
+ dev_warn(
+ &hdev->udev->dev,
+ "failed setting fan pwm/temp curve for %s on channel %d %d,%d,%d\n",
+ hdev->config->name, recv_buf[3], recv_buf[0],
+ recv_buf[1], recv_buf[2]);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int set_fan_target_rpm(struct hydro_i_pro_device *hdev,
+ struct hwmon_fan_data *fan_data, u16 val)
+{
+ int retval;
+ 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 = transfer_usb(hdev, send_buf, recv_buf, 4, 6);
+ if (retval)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf)) {
+ dev_warn(
+ &hdev->udev->dev,
+ "failed setting fan rpm for %s on channel %d %d,%d,%d\n",
+ hdev->config->name, recv_buf[3], recv_buf[0],
+ recv_buf[1], recv_buf[2]);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static int get_fan_current_rpm(struct hydro_i_pro_device *hdev,
+ struct hwmon_fan_data *fan_data, long *val)
+{
+ int retval;
+ 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 = transfer_usb(hdev, send_buf, recv_buf, 2, 6);
+ if (retval)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf) ||
+ recv_buf[3] != fan_data->fan_channel) {
+ dev_warn(
+ &hdev->udev->dev,
+ "failed retreiving fan pwm for %s on channel %d %d,%d,%d\n",
+ hdev->config->name, recv_buf[3], recv_buf[0],
+ recv_buf[1], recv_buf[2]);
+ return -EINVAL;
+ }
+
+ *val = ((recv_buf[4]) << 8) + recv_buf[5];
+ return 0;
+}
+
+static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
+ struct hwmon_fan_data *fan_data, u8 val)
+{
+ int retval;
+ unsigned char *send_buf = hdev->bulk_out_buffer;
+ unsigned char *recv_buf = hdev->bulk_in_buffer;
+
+ fan_data->fan_target = 0;
+ fan_data->fan_pwm_target = val;
+
+ send_buf[0] = PWM_FAN_TARGET_CMD;
+ send_buf[1] = fan_data->fan_channel;
+ send_buf[3] = fan_data->fan_pwm_target;
+
+ retval = transfer_usb(hdev, send_buf, recv_buf, 4, 6);
+ if (retval)
+ return retval;
+
+ if (!check_succes(send_buf[0], recv_buf)) {
+ dev_warn(
+ &hdev->udev->dev,
+ "failed setting fan pwm for %s on channel %d %d,%d,%d\n",
+ hdev->config->name, recv_buf[3], recv_buf[0],
+ recv_buf[1], recv_buf[2]);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static umode_t hwmon_is_visible(const void *d, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ return 0444;
+ break;
+ case hwmon_fan_target:
+ return 0644;
+ break;
+ case hwmon_fan_min:
+ return 0444;
+ break;
+ default:
+ break;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ return 0200;
+ break;
+ case hwmon_pwm_enable:
+ return 0644;
+ break;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
+ return 0;
+}
+
+static int hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ struct hwmon_data *data = dev_get_drvdata(dev);
+ struct hydro_i_pro_device *hdev = data->hdev;
+ struct hwmon_fan_data *fan_data;
+ int retval = 0;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_target:
+ fan_data = data->channel_data[channel];
+ if (fan_data->mode != 1)
+ return -EINVAL;
+
+ if (val < (2 ^ 16) - 2)
+ return -EINVAL;
+
+ retval = acquire_lock(hdev);
+ if (retval)
+ goto exit;
+
+ retval = set_fan_target_rpm(hdev, fan_data, val);
+ if (retval)
+ goto cleanup;
+
+ break;
+ default:
+ return -EINVAL;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ fan_data = data->channel_data[channel];
+ if (fan_data->mode != 1)
+ return -EINVAL;
+
+ if (val < (2 ^ 8) - 2)
+ return -EINVAL;
+
+ retval = acquire_lock(hdev);
+ if (retval)
+ goto exit;
+
+ retval = set_fan_target_pwm(hdev, fan_data, val);
+ if (retval)
+ goto cleanup;
+
+ break;
+ case hwmon_pwm_enable:
+ fan_data = data->channel_data[channel];
+
+ switch (val) {
+ case 0:
+ retval = acquire_lock(hdev);
+ if (retval)
+ goto exit;
+
+ retval = set_fan_pwm_curve(hdev, fan_data,
+ default_curve);
+ if (retval)
+ goto cleanup;
+ fan_data->mode = 0;
+ break;
+ case 1:
+ retval = acquire_lock(hdev);
+ if (retval)
+ goto exit;
+
+ 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;
+ }
+ fan_data->mode = 1;
+ break;
+ case 2:
+ retval = acquire_lock(hdev);
+ if (retval)
+ goto exit;
+
+ retval = set_fan_pwm_curve(hdev, fan_data,
+ default_curve);
+ if (retval)
+ goto cleanup;
+ fan_data->mode = 2;
+ break;
+ default:
+ return -EINVAL;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+
+cleanup:
+ mutex_unlock(&hdev->io_mutex);
+ usb_autopm_put_interface(hdev->interface);
+exit:
+ return retval;
+}
+
+static int hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct hwmon_data *data = dev_get_drvdata(dev);
+ struct hydro_i_pro_device *hdev = data->hdev;
+ struct hwmon_fan_data *fan_data;
+ int retval = 0;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ fan_data = data->channel_data[channel];
+
+ retval = acquire_lock(hdev);
+ if (retval)
+ goto exit;
+
+ 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:
+ mutex_unlock(&hdev->io_mutex);
+ usb_autopm_put_interface(hdev->interface);
+exit:
+ return retval;
+}
+
+static const struct hwmon_ops i_pro_ops = {
+ .is_visible = hwmon_is_visible,
+ .read = hwmon_read,
+ .write = hwmon_write,
+};
+
+static void hwmon_init(struct hydro_i_pro_device *hdev)
+{
+ u8 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);
+
+ /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/
+ WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
+ data->channel_count = hdev->config->fancount;
+
+ /* 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 = 0;
+ data->channel_data[fan_id] = fan;
+ }
+
+ hwmon_info->ops = &i_pro_ops;
+ hwmon_info->info = hdev->config->hwmon_info;
+
+ data->hdev = hdev;
+ hwmon_dev = devm_hwmon_device_register_with_info(
+ &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
+ dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
+}
+
+const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
+const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
+/*
+ * 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(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
+ .driver_info = (kernel_ulong_t)&config_table[0] },
+ {},
+};
+
+MODULE_DEVICE_TABLE(usb, astk_table);
+
+static int init_device(struct usb_device *udev)
+{
+ int retval;
+
+ /*
+ * This is needed because when running windows in a vm with proprietary driver
+ * and you switch to this driver, the device will not respond unless you run this.
+ */
+ retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
+ 0xffff, 0x0000, 0, 0, 0);
+ /*this always returns error*/
+ if (retval)
+ ;
+
+ retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
+ 0x0002, 0x0000, 0, 0, 0);
+ return retval;
+}
+
+static int deinit_device(struct usb_device *udev)
+{
+ return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
+ 0x0004, 0x0000, 0, 0, 0);
+}
+
+static void astk_delete(struct hydro_i_pro_device *hdev)
+{
+ usb_put_intf(hdev->interface);
+ usb_put_dev(hdev->udev);
+ kfree(hdev->bulk_in_buffer);
+ kfree(hdev->bulk_out_buffer);
+ kfree(hdev);
+}
+
+static int astk_probe(struct usb_interface *interface,
+ const struct usb_device_id *id)
+{
+ struct hydro_i_pro_device *hdev =
+ kzalloc(sizeof(struct hydro_i_pro_device), GFP_KERNEL);
+ int retval;
+ struct usb_endpoint_descriptor *bulk_in, *bulk_out;
+
+ if (!hdev) {
+ kfree(hdev);
+ retval = -ENOMEM;
+ goto exit;
+ }
+
+ hdev->config = (const struct device_config *)id->driver_info;
+ /* You should set the driver_info to a pointer to the correct configuration!!*/
+ WARN_ON(hdev->config == NULL);
+
+ retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
+ &bulk_out, NULL, NULL);
+ if (retval) {
+ kfree(hdev);
+ 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 %s\n",
+ hdev->config->name);
+ kfree(hdev);
+ goto exit;
+ }
+
+ hwmon_init(hdev);
+
+ usb_set_intfdata(interface, hdev);
+ mutex_init(&hdev->io_mutex);
+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, "removed hwmon for %s\n",
+ hdev->config->name);
+ deinit_device(hdev->udev);
+ usb_set_intfdata(interface, NULL);
+ astk_delete(hdev);
+}
+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,
+};
+
+module_usb_driver(hydro_i_pro_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jaap Aarts <jaap.aarts1@gmail.com>");
+MODULE_DESCRIPTION("Corsair HXXXi pro device driver");
--
2.28.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-15 21:16 [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
@ 2020-08-15 21:22 ` jaap aarts
2020-08-15 22:54 ` Barnabás Pőcze
2020-08-16 9:34 ` kernel test robot
2 siblings, 0 replies; 12+ messages in thread
From: jaap aarts @ 2020-08-15 21:22 UTC (permalink / raw)
To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb,
Barnabás Pőcze
errr, I am a bit confused, I didn't specify any in-reply-to headers in
`git send-mail`, and I used a different subject anyways.
For me this is being listed as "in reply to" the v3 patch in gmail.
No idea how to "fix" this, maybe a good night's sleep will help.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-15 21:16 [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
2020-08-15 21:22 ` jaap aarts
@ 2020-08-15 22:54 ` Barnabás Pőcze
2020-08-16 0:01 ` Guenter Roeck
2020-08-16 8:57 ` jaap aarts
2020-08-16 9:34 ` kernel test robot
2 siblings, 2 replies; 12+ messages in thread
From: Barnabás Pőcze @ 2020-08-15 22:54 UTC (permalink / raw)
To: jaap aarts; +Cc: Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb
Hello,
there are a couple things that I did not notice in v3, or were introduced in
this version of the patch.
> [...]
> +
> +#define max_fan_count 2
> +#define max_pwm_channel_count max_fan_count
> +
I think these should be all-caps as per
https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
> [...]
> +
> +#define default_curve quiet_curve
> +
I am inclined to say that this should be all-caps as well.
> [...]
> +static int transfer_usb(struct hydro_i_pro_device *hdev,
> + unsigned char *send_buf, unsigned char *recv_buf,
> + int send_len, int recv_len)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> +
> + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, send_len, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> + return 0;
The preceding 5 lines could be simplified to the following:
return usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
BULK_TIMEOUT);
And if you don't use 'wrote', you can simply pass 'NULL' as the 5th argument of
usb_bulk_msg(). Although you should either check the value or set 'recv_buf' to
all zeroes in the calling function to avoid the possibility of a failed transaction
being recognized as successful.
> +}
> +
> +static int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data,
> + struct curve_point point[7])
> +{
> + int retval;
> + unsigned char *send_buf = hdev->bulk_out_buffer;
> + unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> + memcpy(fan_data->curve, point, sizeof(struct curve_point) * 7);
> +
Personally, I'd use 'sizeof(fan_data->curve)' here. And consider making that
seven a named constant.
Beware that even though the size is there in 'struct curve_point point[7]',
you can still pass arrays that are smaller than that. Consider using
'struct curve_point point[static 7]'.
> + 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 = transfer_usb(hdev, send_buf, recv_buf, 16, 4);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf)) {
> + dev_warn(
> + &hdev->udev->dev,
> + "failed setting fan pwm/temp curve for %s on channel %d %d,%d,%d\n",
> + hdev->config->name, recv_buf[3], recv_buf[0],
> + recv_buf[1], recv_buf[2]);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> [...]
> +
> +static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data, u8 val)
> +{
> + int retval;
> + unsigned char *send_buf = hdev->bulk_out_buffer;
> + unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> + fan_data->fan_target = 0;
> + fan_data->fan_pwm_target = val;
> +
> + send_buf[0] = PWM_FAN_TARGET_CMD;
> + send_buf[1] = fan_data->fan_channel;
> + send_buf[3] = fan_data->fan_pwm_target;
> +
> + retval = transfer_usb(hdev, send_buf, recv_buf, 4, 6);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf)) {
Any reason why you don't check recv_buf[3] as in get_fan_current_rpm()?
(same applies to set_fan_pwm_curve() and set_fan_target_rpm())
> + dev_warn(
> + &hdev->udev->dev,
> + "failed setting fan pwm for %s on channel %d %d,%d,%d\n",
> + hdev->config->name, recv_buf[3], recv_buf[0],
> + recv_buf[1], recv_buf[2]);
> + return -EINVAL;
> + }
> + 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;
> +
> + if (val < (2 ^ 16) - 2)
Did you intend to write 2 to the power of 16? Because 2^16 is not that.
2 to the power 16 may be written as '(1 << 16)' or 'BIT(16)'
(you may need to include linux/bits.h for the latter)
But something like this is my suggestion:
if (val < 0 || 0xFFFF < val)
return -EINVAL;
Though, I suspect the fans won't go up to 60000 RPM or so.
> + return -EINVAL;
> +
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = set_fan_target_rpm(hdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + fan_data = data->channel_data[channel];
> + if (fan_data->mode != 1)
> + return -EINVAL;
> +
> + if (val < (2 ^ 8) - 2)
Same here, 2^8 != 2 to the power of 8.
I suggest you simply do something like the following:
if (val < 0 || 0xFF < val)
return -EINVAL;
> + return -EINVAL;
> +
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = set_fan_target_pwm(hdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + break;
> + case hwmon_pwm_enable:
> + fan_data = data->channel_data[channel];
> +
> + switch (val) {
> + case 0:
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = set_fan_pwm_curve(hdev, fan_data,
> + default_curve);
> + if (retval)
> + goto cleanup;
> + fan_data->mode = 0;
> + break;
> + case 1:
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + 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;
> + }
> + fan_data->mode = 1;
> + break;
> + case 2:
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = set_fan_pwm_curve(hdev, fan_data,
> + default_curve);
> + if (retval)
> + goto cleanup;
> + fan_data->mode = 2;
> + break;
If I see it correctly, case 0 and case 2 are the basically the same, no?
If so, then you could merge them.
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> +cleanup:
> + mutex_unlock(&hdev->io_mutex);
> + usb_autopm_put_interface(hdev->interface);
> +exit:
> + return retval;
> +}
> +
> +static int hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct hwmon_data *data = dev_get_drvdata(dev);
> + struct hydro_i_pro_device *hdev = data->hdev;
> + struct hwmon_fan_data *fan_data;
> + int retval = 0;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + fan_data = data->channel_data[channel];
> +
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = get_fan_current_rpm(hdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + goto cleanup;
The preceding 3 lines can be replaced by a single 'break' given that the
'goto exit' is replaced by a 'break' after the 'switch (attr)'
> + 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;
> +
I don't see why this goto is needed here. It has no effect on the control flow.
> + 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:
> + mutex_unlock(&hdev->io_mutex);
> + usb_autopm_put_interface(hdev->interface);
> +exit:
> + return retval;
> +}
> +
> +static const struct hwmon_ops i_pro_ops = {
> + .is_visible = hwmon_is_visible,
> + .read = hwmon_read,
> + .write = hwmon_write,
> +};
> +
> +static void hwmon_init(struct hydro_i_pro_device *hdev)
> +{
> + u8 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);
> +
> + /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/
> + WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
If this warning is triggered, then that leads to the overflow of 'data->channel_data' later on.
> + data->channel_count = hdev->config->fancount;
> +
> + /* 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 = 0;
> + data->channel_data[fan_id] = fan;
> + }
> +
> + hwmon_info->ops = &i_pro_ops;
> + hwmon_info->info = hdev->config->hwmon_info;
> +
> + data->hdev = hdev;
> + hwmon_dev = devm_hwmon_device_register_with_info(
> + &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
> + dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
> +}
> +
There is absolutely no error checking in hwmon_init().
> +const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
> +const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
I think these should be either - preferably - #define's or 'static' at least.
> +/*
> + * 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(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
> + .driver_info = (kernel_ulong_t)&config_table[0] },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(usb, astk_table);
> +
> +static int init_device(struct usb_device *udev)
> +{
> + int retval;
> +
> + /*
> + * This is needed because when running windows in a vm with proprietary driver
> + * and you switch to this driver, the device will not respond unless you run this.
> + */
> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
> + 0xffff, 0x0000, 0, 0, 0);
> + /*this always returns error*/
> + if (retval)
> + ;
Shouldn't you check if it's the "good" kind of error?
> +
> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> + 0x0002, 0x0000, 0, 0, 0);
> + return retval;
> +}
> +
> +static int deinit_device(struct usb_device *udev)
> +{
> + return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> + 0x0004, 0x0000, 0, 0, 0);
> +}
> +
> +static void astk_delete(struct hydro_i_pro_device *hdev)
> +{
> + usb_put_intf(hdev->interface);
> + usb_put_dev(hdev->udev);
> + kfree(hdev->bulk_in_buffer);
> + kfree(hdev->bulk_out_buffer);
> + kfree(hdev);
> +}
> +
I think you should call mutex_destroy() in astk_delete().
> +static int astk_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct hydro_i_pro_device *hdev =
> + kzalloc(sizeof(struct hydro_i_pro_device), GFP_KERNEL);
I think this should be modifed to use 'sizeof(*ptr)' as per
https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory
(and everything else that uses the same pattern)
> [...]
> + retval = init_device(hdev->udev);
> + if (retval) {
> + dev_err(&interface->dev, "failed initialising %s\n",
> + hdev->config->name);
If you print the error code, that helps immensely with troubleshooting.
> + kfree(hdev);
> + goto exit;
> + }
> +
> + hwmon_init(hdev);
> +
> + usb_set_intfdata(interface, hdev);
> + mutex_init(&hdev->io_mutex);
> +exit:
> + return retval;
> +}
> +
> [...]
Barnabás Pőcze
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-15 22:54 ` Barnabás Pőcze
@ 2020-08-16 0:01 ` Guenter Roeck
2020-08-16 0:27 ` Barnabás Pőcze
2020-08-16 8:57 ` jaap aarts
1 sibling, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2020-08-16 0:01 UTC (permalink / raw)
To: Barnabás Pőcze, jaap aarts; +Cc: Jean Delvare, linux-hwmon, linux-usb
On 8/15/20 3:54 PM, Barnabás Pőcze wrote:
> Hello,
>
> there are a couple things that I did not notice in v3, or were introduced in
> this version of the patch.
>
>
>> [...]
>> +
>> +#define max_fan_count 2
>> +#define max_pwm_channel_count max_fan_count
>> +
>
> I think these should be all-caps as per
> https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl
>
>
>> [...]
>> +
>> +#define default_curve quiet_curve
>> +
>
> I am inclined to say that this should be all-caps as well.
>
>
>> [...]
>> +static int transfer_usb(struct hydro_i_pro_device *hdev,
>> + unsigned char *send_buf, unsigned char *recv_buf,
>> + int send_len, int recv_len)
>> +{
>> + int retval;
>> + int wrote;
>> + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
>> + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
>> +
>> + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, send_len, &wrote,
>> + BULK_TIMEOUT);
>> + if (retval)
>> + return retval;
>> +
>> + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
>> + BULK_TIMEOUT);
>> + if (retval)
>> + return retval;
>> + return 0;
>
> The preceding 5 lines could be simplified to the following:
>
> return usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
> BULK_TIMEOUT);
>
> And if you don't use 'wrote', you can simply pass 'NULL' as the 5th argument of
> usb_bulk_msg(). Although you should either check the value or set 'recv_buf' to
> all zeroes in the calling function to avoid the possibility of a failed transaction
> being recognized as successful.
>
>
>> +}
>> +
>> +static int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
>> + struct hwmon_fan_data *fan_data,
>> + struct curve_point point[7])
>> +{
>> + int retval;
>> + unsigned char *send_buf = hdev->bulk_out_buffer;
>> + unsigned char *recv_buf = hdev->bulk_in_buffer;
>> +
>> + memcpy(fan_data->curve, point, sizeof(struct curve_point) * 7);
>> +
>
> Personally, I'd use 'sizeof(fan_data->curve)' here. And consider making that
> seven a named constant.
>
> Beware that even though the size is there in 'struct curve_point point[7]',
> you can still pass arrays that are smaller than that. Consider using
> 'struct curve_point point[static 7]'.
>
>
>> + 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 = transfer_usb(hdev, send_buf, recv_buf, 16, 4);
>> + if (retval)
>> + return retval;
>> +
>> + if (!check_succes(send_buf[0], recv_buf)) {
>> + dev_warn(
>> + &hdev->udev->dev,
>> + "failed setting fan pwm/temp curve for %s on channel %d %d,%d,%d\n",
>> + hdev->config->name, recv_buf[3], recv_buf[0],
>> + recv_buf[1], recv_buf[2]);
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>> +
>> [...]
>> +
>> +static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
>> + struct hwmon_fan_data *fan_data, u8 val)
>> +{
>> + int retval;
>> + unsigned char *send_buf = hdev->bulk_out_buffer;
>> + unsigned char *recv_buf = hdev->bulk_in_buffer;
>> +
>> + fan_data->fan_target = 0;
>> + fan_data->fan_pwm_target = val;
>> +
>> + send_buf[0] = PWM_FAN_TARGET_CMD;
>> + send_buf[1] = fan_data->fan_channel;
>> + send_buf[3] = fan_data->fan_pwm_target;
>> +
>> + retval = transfer_usb(hdev, send_buf, recv_buf, 4, 6);
>> + if (retval)
>> + return retval;
>> +
>> + if (!check_succes(send_buf[0], recv_buf)) {
>
> Any reason why you don't check recv_buf[3] as in get_fan_current_rpm()?
> (same applies to set_fan_pwm_curve() and set_fan_target_rpm())
>
>
>> + dev_warn(
>> + &hdev->udev->dev,
>> + "failed setting fan pwm for %s on channel %d %d,%d,%d\n",
>> + hdev->config->name, recv_buf[3], recv_buf[0],
>> + recv_buf[1], recv_buf[2]);
>> + return -EINVAL;
>> + }
>> + 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;
>> +
>> + if (val < (2 ^ 16) - 2)
>
> Did you intend to write 2 to the power of 16? Because 2^16 is not that.
> 2 to the power 16 may be written as '(1 << 16)' or 'BIT(16)'
> (you may need to include linux/bits.h for the latter)
>
> But something like this is my suggestion:
>
> if (val < 0 || 0xFFFF < val)
> return -EINVAL;
>
I would suggest to use clamp_val; we don't expect users to know device
specific limits. Also, FTR, I won't accept any Yoda programming.
Guenter
> Though, I suspect the fans won't go up to 60000 RPM or so.
>
>
>> + return -EINVAL;
>> +
>> + retval = acquire_lock(hdev);
>> + if (retval)
>> + goto exit;
>> +
>> + retval = set_fan_target_rpm(hdev, fan_data, val);
>> + if (retval)
>> + goto cleanup;
>> +
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + break;
>> + case hwmon_pwm:
>> + switch (attr) {
>> + case hwmon_pwm_input:
>> + fan_data = data->channel_data[channel];
>> + if (fan_data->mode != 1)
>> + return -EINVAL;
>> +
>> + if (val < (2 ^ 8) - 2)
>
> Same here, 2^8 != 2 to the power of 8.
>
> I suggest you simply do something like the following:
>
> if (val < 0 || 0xFF < val)
> return -EINVAL;
>
>
>> + return -EINVAL;
>> +
>> + retval = acquire_lock(hdev);
>> + if (retval)
>> + goto exit;
>> +
>> + retval = set_fan_target_pwm(hdev, fan_data, val);
>> + if (retval)
>> + goto cleanup;
>> +
>> + break;
>> + case hwmon_pwm_enable:
>> + fan_data = data->channel_data[channel];
>> +
>> + switch (val) {
>> + case 0:
>> + retval = acquire_lock(hdev);
>> + if (retval)
>> + goto exit;
>> +
>> + retval = set_fan_pwm_curve(hdev, fan_data,
>> + default_curve);
>> + if (retval)
>> + goto cleanup;
>> + fan_data->mode = 0;
>> + break;
>> + case 1:
>> + retval = acquire_lock(hdev);
>> + if (retval)
>> + goto exit;
>> +
>> + 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;
>> + }
>> + fan_data->mode = 1;
>> + break;
>> + case 2:
>> + retval = acquire_lock(hdev);
>> + if (retval)
>> + goto exit;
>> +
>> + retval = set_fan_pwm_curve(hdev, fan_data,
>> + default_curve);
>> + if (retval)
>> + goto cleanup;
>> + fan_data->mode = 2;
>> + break;
>
> If I see it correctly, case 0 and case 2 are the basically the same, no?
> If so, then you could merge them.
>
>
>> + default:
>> + return -EINVAL;
>> + }
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> +cleanup:
>> + mutex_unlock(&hdev->io_mutex);
>> + usb_autopm_put_interface(hdev->interface);
>> +exit:
>> + return retval;
>> +}
>> +
>> +static int hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int channel, long *val)
>> +{
>> + struct hwmon_data *data = dev_get_drvdata(dev);
>> + struct hydro_i_pro_device *hdev = data->hdev;
>> + struct hwmon_fan_data *fan_data;
>> + int retval = 0;
>> +
>> + switch (type) {
>> + case hwmon_fan:
>> + switch (attr) {
>> + case hwmon_fan_input:
>> + fan_data = data->channel_data[channel];
>> +
>> + retval = acquire_lock(hdev);
>> + if (retval)
>> + goto exit;
>> +
>> + retval = get_fan_current_rpm(hdev, fan_data, val);
>> + if (retval)
>> + goto cleanup;
>> +
>> + goto cleanup;
>
> The preceding 3 lines can be replaced by a single 'break' given that the
> 'goto exit' is replaced by a 'break' after the 'switch (attr)'
>
>
>> + 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;
>> +
>
> I don't see why this goto is needed here. It has no effect on the control flow.
>
>
>> + 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:
>> + mutex_unlock(&hdev->io_mutex);
>> + usb_autopm_put_interface(hdev->interface);
>> +exit:
>> + return retval;
>> +}
>> +
>> +static const struct hwmon_ops i_pro_ops = {
>> + .is_visible = hwmon_is_visible,
>> + .read = hwmon_read,
>> + .write = hwmon_write,
>> +};
>> +
>> +static void hwmon_init(struct hydro_i_pro_device *hdev)
>> +{
>> + u8 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);
>> +
>> + /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/
>> + WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
>
> If this warning is triggered, then that leads to the overflow of 'data->channel_data' later on.
>
>
>> + data->channel_count = hdev->config->fancount;
>> +
>> + /* 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 = 0;
>> + data->channel_data[fan_id] = fan;
>> + }
>> +
>> + hwmon_info->ops = &i_pro_ops;
>> + hwmon_info->info = hdev->config->hwmon_info;
>> +
>> + data->hdev = hdev;
>> + hwmon_dev = devm_hwmon_device_register_with_info(
>> + &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
>> + dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
>> +}
>> +
>
> There is absolutely no error checking in hwmon_init().
>
>
>> +const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
>> +const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
>
> I think these should be either - preferably - #define's or 'static' at least.
>
>
>> +/*
>> + * 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(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
>> + .driver_info = (kernel_ulong_t)&config_table[0] },
>> + {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(usb, astk_table);
>> +
>> +static int init_device(struct usb_device *udev)
>> +{
>> + int retval;
>> +
>> + /*
>> + * This is needed because when running windows in a vm with proprietary driver
>> + * and you switch to this driver, the device will not respond unless you run this.
>> + */
>> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
>> + 0xffff, 0x0000, 0, 0, 0);
>> + /*this always returns error*/
>> + if (retval)
>> + ;
>
> Shouldn't you check if it's the "good" kind of error?
>
>
>> +
>> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
>> + 0x0002, 0x0000, 0, 0, 0);
>> + return retval;
>> +}
>> +
>> +static int deinit_device(struct usb_device *udev)
>> +{
>> + return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
>> + 0x0004, 0x0000, 0, 0, 0);
>> +}
>> +
>> +static void astk_delete(struct hydro_i_pro_device *hdev)
>> +{
>> + usb_put_intf(hdev->interface);
>> + usb_put_dev(hdev->udev);
>> + kfree(hdev->bulk_in_buffer);
>> + kfree(hdev->bulk_out_buffer);
>> + kfree(hdev);
>> +}
>> +
>
> I think you should call mutex_destroy() in astk_delete().
>
>
>> +static int astk_probe(struct usb_interface *interface,
>> + const struct usb_device_id *id)
>> +{
>> + struct hydro_i_pro_device *hdev =
>> + kzalloc(sizeof(struct hydro_i_pro_device), GFP_KERNEL);
>
> I think this should be modifed to use 'sizeof(*ptr)' as per
> https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory
> (and everything else that uses the same pattern)
>
>
>
>> [...]
>> + retval = init_device(hdev->udev);
>> + if (retval) {
>> + dev_err(&interface->dev, "failed initialising %s\n",
>> + hdev->config->name);
>
> If you print the error code, that helps immensely with troubleshooting.
>
>
>> + kfree(hdev);
>> + goto exit;
>> + }
>> +
>> + hwmon_init(hdev);
>> +
>> + usb_set_intfdata(interface, hdev);
>> + mutex_init(&hdev->io_mutex);
>> +exit:
>> + return retval;
>> +}
>> +
>> [...]
>
>
> Barnabás Pőcze
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-16 0:01 ` Guenter Roeck
@ 2020-08-16 0:27 ` Barnabás Pőcze
2020-08-16 0:51 ` Guenter Roeck
0 siblings, 1 reply; 12+ messages in thread
From: Barnabás Pőcze @ 2020-08-16 0:27 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jaap aarts, Jean Delvare, linux-hwmon, linux-usb
> [...]
> >> + if (val < (2 ^ 16) - 2)
> >
> > Did you intend to write 2 to the power of 16? Because 2^16 is not that.
> > 2 to the power 16 may be written as '(1 << 16)' or 'BIT(16)'
> > (you may need to include linux/bits.h for the latter)
> >
> > But something like this is my suggestion:
> >
> > if (val < 0 || 0xFFFF < val)
> > return -EINVAL;
> >
>
> I would suggest to use clamp_val; we don't expect users to know device
> specific limits. Also, FTR, I won't accept any Yoda programming.
>
> Guenter
> [...]
If you don't mind me asking, why is that? I, too, don't like Yoda conditions,
but I've always thought that in this specific case - when checking if a value
lies within/outside a certain range - it makes the code more easily readable
if it is written like this: (lo < val && val < hi).
Maybe I'm just too accustomed to it?
(Don't get me wrong, I'm not trying to argue with your decision about not
accepting such code, neither am I trying to convince you otherwise.)
Barnabás Pőcze
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-16 0:27 ` Barnabás Pőcze
@ 2020-08-16 0:51 ` Guenter Roeck
2020-08-16 1:35 ` Barnabás Pőcze
0 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2020-08-16 0:51 UTC (permalink / raw)
To: Barnabás Pőcze; +Cc: jaap aarts, Jean Delvare, linux-hwmon, linux-usb
On 8/15/20 5:27 PM, Barnabás Pőcze wrote:
>> [...]
>>>> + if (val < (2 ^ 16) - 2)
>>>
>>> Did you intend to write 2 to the power of 16? Because 2^16 is not that.
>>> 2 to the power 16 may be written as '(1 << 16)' or 'BIT(16)'
>>> (you may need to include linux/bits.h for the latter)
>>>
>>> But something like this is my suggestion:
>>>
>>> if (val < 0 || 0xFFFF < val)
>>> return -EINVAL;
>>>
>>
>> I would suggest to use clamp_val; we don't expect users to know device
>> specific limits. Also, FTR, I won't accept any Yoda programming.
>>
>> Guenter
>> [...]
>
>
> If you don't mind me asking, why is that? I, too, don't like Yoda conditions,
> but I've always thought that in this specific case - when checking if a value
> lies within/outside a certain range - it makes the code more easily readable
> if it is written like this: (lo < val && val < hi).
>
You are suggesting (val < lo || hi < val) here, which is a bit different.
Anyway, good for you. If you are signing up as code reviewer for a Linux kernel
subsystem, please feel free to accept and promote such code. For me, it is just
confusing.
Guenter
> Maybe I'm just too accustomed to it?
>
> (Don't get me wrong, I'm not trying to argue with your decision about not
> accepting such code, neither am I trying to convince you otherwise.)
> >
> Barnabás Pőcze
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-16 0:51 ` Guenter Roeck
@ 2020-08-16 1:35 ` Barnabás Pőcze
0 siblings, 0 replies; 12+ messages in thread
From: Barnabás Pőcze @ 2020-08-16 1:35 UTC (permalink / raw)
To: Guenter Roeck; +Cc: jaap aarts, Jean Delvare, linux-hwmon, linux-usb
2020. augusztus 16., vasárnap 2:51 keltezéssel, Guenter Roeck írta:
> On 8/15/20 5:27 PM, Barnabás Pőcze wrote:
> >> [...]
> >>>> + if (val < (2 ^ 16) - 2)
> >>>
> >>> Did you intend to write 2 to the power of 16? Because 2^16 is not that.
> >>> 2 to the power 16 may be written as '(1 << 16)' or 'BIT(16)'
> >>> (you may need to include linux/bits.h for the latter)
> >>>
> >>> But something like this is my suggestion:
> >>>
> >>> if (val < 0 || 0xFFFF < val)
> >>> return -EINVAL;
> >>>
> >>
> >> I would suggest to use clamp_val; we don't expect users to know device
> >> specific limits. Also, FTR, I won't accept any Yoda programming.
> >>
> >> Guenter
> >> [...]
> >
> >
> > If you don't mind me asking, why is that? I, too, don't like Yoda conditions,
> > but I've always thought that in this specific case - when checking if a value
> > lies within/outside a certain range - it makes the code more easily readable
> > if it is written like this: (lo < val && val < hi).
> >
>
> You are suggesting (val < lo || hi < val) here, which is a bit different.
>
> Anyway, good for you. If you are signing up as code reviewer for a Linux kernel
> subsystem, please feel free to accept and promote such code. For me, it is just
> confusing.
>
> Guenter
>
Thank you for the answer.
Barnabás Pőcze
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-15 22:54 ` Barnabás Pőcze
2020-08-16 0:01 ` Guenter Roeck
@ 2020-08-16 8:57 ` jaap aarts
1 sibling, 0 replies; 12+ messages in thread
From: jaap aarts @ 2020-08-16 8:57 UTC (permalink / raw)
To: Barnabás Pőcze
Cc: Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb
On Sun, 16 Aug 2020 at 00:54, Barnabás Pőcze <pobrn@protonmail.com> wrote:
> [...]
> > +static int transfer_usb(struct hydro_i_pro_device *hdev,
> > + unsigned char *send_buf, unsigned char *recv_buf,
> > + int send_len, int recv_len)
> > +{
> > + int retval;
> > + int wrote;
> > + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> > + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> > +
> > + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, send_len, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > +
> > + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
> > + BULK_TIMEOUT);
> > + if (retval)
> > + return retval;
> > + return 0;
>
> The preceding 5 lines could be simplified to the following:
>
> return usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
> BULK_TIMEOUT);
>
> And if you don't use 'wrote', you can simply pass 'NULL' as the 5th argument of
> usb_bulk_msg(). Although you should either check the value or set 'recv_buf' to
> all zeroes in the calling function to avoid the possibility of a failed transaction
> being recognized as successful.
I ended up using `wrote`, so I still need those lines now.
> > +}
> > +
> > +static int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
> > + struct hwmon_fan_data *fan_data,
> > + struct curve_point point[7])
> > +{
> > + int retval;
> > + unsigned char *send_buf = hdev->bulk_out_buffer;
> > + unsigned char *recv_buf = hdev->bulk_in_buffer;
> > +
> > + memcpy(fan_data->curve, point, sizeof(struct curve_point) * 7);
> > +
>
> Personally, I'd use 'sizeof(fan_data->curve)' here. And consider making that
> seven a named constant.
You told me I should be consistent with `sizeof` using variables vs types?
I agree that it should be `sizeof(fan_data->curve)`, that's what I used before.
> [...]
> > + if (!check_succes(send_buf[0], recv_buf)) {
>
> Any reason why you don't check recv_buf[3] as in get_fan_current_rpm()?
> (same applies to set_fan_pwm_curve() and set_fan_target_rpm())
Yes, the device simply doesnt return them, I had the expected bytes returned
wrong as well, they should be 3.
> [...]
> > +
> > +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;
> > +
> > + if (val < (2 ^ 16) - 2)
>
> Did you intend to write 2 to the power of 16? Because 2^16 is not that.
> 2 to the power 16 may be written as '(1 << 16)' or 'BIT(16)'
> (you may need to include linux/bits.h for the latter)
I should not do things at night, I did mean 1<<16, although BIT(16) is
probably nicer. I thought about casting it to u8/u16 and checking if it
is still the same, but just comparing it to `BIT(16)-1` gives clearer
intentions.
> But something like this is my suggestion:
>
> if (val < 0 || 0xFFFF < val)
> return -EINVAL;
>
> Though, I suspect the fans won't go up to 60000 RPM or so.
Just tried, the device has failsafes for this :) invalid argument is returned.
> [...]
> > +static int hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + struct hwmon_data *data = dev_get_drvdata(dev);
> > + struct hydro_i_pro_device *hdev = data->hdev;
> > + struct hwmon_fan_data *fan_data;
> > + int retval = 0;
> > +
> > + switch (type) {
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + fan_data = data->channel_data[channel];
> > +
> > + retval = acquire_lock(hdev);
> > + if (retval)
> > + goto exit;
> > +
> > + retval = get_fan_current_rpm(hdev, fan_data, val);
> > + if (retval)
> > + goto cleanup;
> > +
> > + goto cleanup;
>
> The preceding 3 lines can be replaced by a single 'break' given that the
> 'goto exit' is replaced by a 'break' after the 'switch (attr)'
That sounds alright.
> [...]
> > + /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/
> > + WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
>
> If this warning is triggered, then that leads to the overflow of 'data->channel_data' later on.
I am just going to clamp this just like the rpm/pwm values.
> > + data->channel_count = hdev->config->fancount;
> > +
> > + /* 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 = 0;
> > + data->channel_data[fan_id] = fan;
> > + }
> > +
> > + hwmon_info->ops = &i_pro_ops;
> > + hwmon_info->info = hdev->config->hwmon_info;
> > +
> > + data->hdev = hdev;
> > + hwmon_dev = devm_hwmon_device_register_with_info(
> > + &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
> > + dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
> > +}
> > +
>
> There is absolutely no error checking in hwmon_init().
>
>
> > +const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
> > +const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
>
> I think these should be either - preferably - #define's or 'static' at least.
>
>
> > +/*
> > + * 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(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
> > + .driver_info = (kernel_ulong_t)&config_table[0] },
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(usb, astk_table);
> > +
> > +static int init_device(struct usb_device *udev)
> > +{
> > + int retval;
> > +
> > + /*
> > + * This is needed because when running windows in a vm with proprietary driver
> > + * and you switch to this driver, the device will not respond unless you run this.
> > + */
> > + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
> > + 0xffff, 0x0000, 0, 0, 0);
> > + /*this always returns error*/
> > + if (retval)
> > + ;
>
> Shouldn't you check if it's the "good" kind of error?
probably yeah, I still have no idea why the error occurs,but it is required when
switching from windows driver to linux.
> > +
> > + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> > + 0x0002, 0x0000, 0, 0, 0);
> > + return retval;
> > +}
> > +
> > +static int deinit_device(struct usb_device *udev)
> > +{
> > + return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> > + 0x0004, 0x0000, 0, 0, 0);
> > +}
> > +
> > +static void astk_delete(struct hydro_i_pro_device *hdev)
> > +{
> > + usb_put_intf(hdev->interface);
> > + usb_put_dev(hdev->udev);
> > + kfree(hdev->bulk_in_buffer);
> > + kfree(hdev->bulk_out_buffer);
> > + kfree(hdev);
> > +}
> > +
>
> I think you should call mutex_destroy() in astk_delete().
>
>
> > +static int astk_probe(struct usb_interface *interface,
> > + const struct usb_device_id *id)
> > +{
> > + struct hydro_i_pro_device *hdev =
> > + kzalloc(sizeof(struct hydro_i_pro_device), GFP_KERNEL);
>
> I think this should be modifed to use 'sizeof(*ptr)' as per
> https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory
> (and everything else that uses the same pattern)
Aah ok, you just told me to be more consistent so I moved everything
to sizeof(type)
while it should have been all to sizeof(var).
> [...]
Ok, I fixed all of the issues, I also made the input prm/pwm clamped
by min/max values
I manually tested.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-15 21:16 [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
@ 2020-08-16 9:34 ` kernel test robot
2020-08-15 22:54 ` Barnabás Pőcze
2020-08-16 9:34 ` kernel test robot
2 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2020-08-16 9:34 UTC (permalink / raw)
To: jaap aarts, Jean Delvare, Guenter Roeck, linux-hwmon, linux-usb,
Barnabás Pőcze
Cc: kbuild-all, jaap aarts
[-- Attachment #1: Type: text/plain, Size: 4320 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 next-20200814]
[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/20200816-085929
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: nios2-randconfig-c003-20200816 (attached as .config)
compiler: nios2-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=nios2
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 'hwmon_init':
>> drivers/hwmon/corsair_hydro_i_pro.c:529:17: warning: variable 'hwmon_dev' set but not used [-Wunused-but-set-variable]
529 | struct device *hwmon_dev;
| ^~~~~~~~~
drivers/hwmon/corsair_hydro_i_pro.c: In function 'init_device':
>> drivers/hwmon/corsair_hydro_i_pro.c:584:3: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
584 | ;
| ^
vim +/hwmon_dev +529 drivers/hwmon/corsair_hydro_i_pro.c
525
526 static void hwmon_init(struct hydro_i_pro_device *hdev)
527 {
528 u8 fan_id;
> 529 struct device *hwmon_dev;
530 struct hwmon_fan_data *fan;
531 struct hwmon_data *data = devm_kzalloc(
532 &hdev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
533 struct hwmon_chip_info *hwmon_info = devm_kzalloc(
534 &hdev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
535
536 /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/
537 WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
538 data->channel_count = hdev->config->fancount;
539
540 /* For each fan create a data channel a fan config entry and a pwm config entry */
541 for (fan_id = 0; fan_id < data->channel_count; fan_id++) {
542 fan = devm_kzalloc(&hdev->udev->dev,
543 sizeof(struct hwmon_fan_data), GFP_KERNEL);
544 fan->fan_channel = fan_id;
545 fan->mode = 0;
546 data->channel_data[fan_id] = fan;
547 }
548
549 hwmon_info->ops = &i_pro_ops;
550 hwmon_info->info = hdev->config->hwmon_info;
551
552 data->hdev = hdev;
553 hwmon_dev = devm_hwmon_device_register_with_info(
554 &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
555 dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
556 }
557
558 const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
559 const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
560 /*
561 * Devices that work with this driver.
562 * More devices should work, however none have been tested.
563 */
564 static const struct usb_device_id astk_table[] = {
565 { USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
566 .driver_info = (kernel_ulong_t)&config_table[0] },
567 {},
568 };
569
570 MODULE_DEVICE_TABLE(usb, astk_table);
571
572 static int init_device(struct usb_device *udev)
573 {
574 int retval;
575
576 /*
577 * This is needed because when running windows in a vm with proprietary driver
578 * and you switch to this driver, the device will not respond unless you run this.
579 */
580 retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
581 0xffff, 0x0000, 0, 0, 0);
582 /*this always returns error*/
583 if (retval)
> 584 ;
585
586 retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
587 0x0002, 0x0000, 0, 0, 0);
588 return retval;
589 }
590
---
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: 24911 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
@ 2020-08-16 9:34 ` kernel test robot
0 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2020-08-16 9:34 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4428 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 next-20200814]
[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/20200816-085929
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: nios2-randconfig-c003-20200816 (attached as .config)
compiler: nios2-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=nios2
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 'hwmon_init':
>> drivers/hwmon/corsair_hydro_i_pro.c:529:17: warning: variable 'hwmon_dev' set but not used [-Wunused-but-set-variable]
529 | struct device *hwmon_dev;
| ^~~~~~~~~
drivers/hwmon/corsair_hydro_i_pro.c: In function 'init_device':
>> drivers/hwmon/corsair_hydro_i_pro.c:584:3: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
584 | ;
| ^
vim +/hwmon_dev +529 drivers/hwmon/corsair_hydro_i_pro.c
525
526 static void hwmon_init(struct hydro_i_pro_device *hdev)
527 {
528 u8 fan_id;
> 529 struct device *hwmon_dev;
530 struct hwmon_fan_data *fan;
531 struct hwmon_data *data = devm_kzalloc(
532 &hdev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
533 struct hwmon_chip_info *hwmon_info = devm_kzalloc(
534 &hdev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
535
536 /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/
537 WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
538 data->channel_count = hdev->config->fancount;
539
540 /* For each fan create a data channel a fan config entry and a pwm config entry */
541 for (fan_id = 0; fan_id < data->channel_count; fan_id++) {
542 fan = devm_kzalloc(&hdev->udev->dev,
543 sizeof(struct hwmon_fan_data), GFP_KERNEL);
544 fan->fan_channel = fan_id;
545 fan->mode = 0;
546 data->channel_data[fan_id] = fan;
547 }
548
549 hwmon_info->ops = &i_pro_ops;
550 hwmon_info->info = hdev->config->hwmon_info;
551
552 data->hdev = hdev;
553 hwmon_dev = devm_hwmon_device_register_with_info(
554 &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
555 dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
556 }
557
558 const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
559 const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
560 /*
561 * Devices that work with this driver.
562 * More devices should work, however none have been tested.
563 */
564 static const struct usb_device_id astk_table[] = {
565 { USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
566 .driver_info = (kernel_ulong_t)&config_table[0] },
567 {},
568 };
569
570 MODULE_DEVICE_TABLE(usb, astk_table);
571
572 static int init_device(struct usb_device *udev)
573 {
574 int retval;
575
576 /*
577 * This is needed because when running windows in a vm with proprietary driver
578 * and you switch to this driver, the device will not respond unless you run this.
579 */
580 retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
581 0xffff, 0x0000, 0, 0, 0);
582 /*this always returns error*/
583 if (retval)
> 584 ;
585
586 retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
587 0x0002, 0x0000, 0, 0, 0);
588 return retval;
589 }
590
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 24911 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
@ 2020-08-16 0:04 Guenter Roeck
2020-08-16 9:05 ` jaap aarts
0 siblings, 1 reply; 12+ messages in thread
From: Guenter Roeck @ 2020-08-16 0:04 UTC (permalink / raw)
To: jaap aarts; +Cc: Jean Delvare, linux-hwmon, linux-usb, Barnabás Pőcze
On Sat, Aug 15, 2020 at 11:16:17PM +0200, jaap aarts wrote:
> Adds fan/pwm support for H100i platinum.
> Custom temp/fan curves are not supported.
>
> v4:
> - fixed spelling
> - more consistent use of uN and other inconsistencies
> - moved from semaphore to mutex, fixing 2 locking bugs allong the way
> - moved to memcmp vs strncmp
> - now uses driver_info for the device configuration
> - check input ranges for fan rpm/pwm
> - fix default case
> - off-by-one loop range
> - improved naming and logging messages
> - fixed unfreed hdev
> - use module_usb_driver
> - use fixed-sized array for rpm/pwm channels independant of device.
> - use common function for the USB bulk messages.
>
> Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com>
checkpatch --strict says:
total: 7 errors, 12 warnings, 13 checks, 714 lines checked
Guenter
> ---
> drivers/hwmon/Kconfig | 7 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/corsair_hydro_i_pro.c | 694 ++++++++++++++++++++++++++++
> 3 files changed, 702 insertions(+)
> create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 288ae9f63588..f466b72d0f67 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -378,6 +378,13 @@ config SENSORS_ARM_SCPI
> and power sensors available on ARM Ltd's SCP based platforms. The
> actual number and type of sensors exported depend on the platform.
>
> +config SENSORS_CORSAIR_HYDRO_I_PRO
> + tristate "Corsair hydro HXXXi pro driver"
> + depends on USB
> + help
> + If you say yes here you get support for the corsair hydro HXXXi pro
> + range of devices.
> +
> config SENSORS_ASB100
> tristate "Asus ASB100 Bach"
> depends on X86 && I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3e32c21f5efe..ec63294b3ef1 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_W83793) += w83793.o
> obj-$(CONFIG_SENSORS_W83795) += w83795.o
> obj-$(CONFIG_SENSORS_W83781D) += w83781d.o
> obj-$(CONFIG_SENSORS_W83791D) += w83791d.o
> +obj-$(CONFIG_SENSORS_CORSAIR_HYDRO_I_PRO) += corsair_hydro_i_pro.o
>
> obj-$(CONFIG_SENSORS_AB8500) += abx500.o ab8500.o
> obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o
> diff --git a/drivers/hwmon/corsair_hydro_i_pro.c b/drivers/hwmon/corsair_hydro_i_pro.c
> new file mode 100644
> index 000000000000..f4dd435be7dd
> --- /dev/null
> +++ b/drivers/hwmon/corsair_hydro_i_pro.c
> @@ -0,0 +1,694 @@
> +// 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 partially reverse engineered by audiohacked
> + * https://github.com/audiohacked/OpendriverLink
> + */
> +
> +/*
> + * Supports following liquid coolers:
> + * H100i platinum
> + *
> + * Other products should work with this driver with slight modification.
> + *
> + * Note: platinum is the codename name for pro within the driver, so H100i platinum = H100i pro.
> + * But some products are actually called platinum, these are not intended to be supported (yet).
> + *
> + * Note: fan curve control has not been implemented
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +struct device_config {
> + u16 vendor_id;
> + u16 product_id;
> + u8 fancount;
> + const char *name;
> + const struct hwmon_channel_info **hwmon_info;
> +};
> +
> +struct hydro_i_pro_device {
> + struct usb_device *udev;
> +
> + const struct device_config *config;
> +
> + unsigned char *bulk_out_buffer;
> + unsigned char *bulk_in_buffer;
> + size_t bulk_out_size;
> + size_t bulk_in_size;
> + char bulk_in_endpointAddr;
> + char bulk_out_endpointAddr;
> +
> + struct usb_interface *interface; /* the interface for this device */
> + struct mutex io_mutex;
> +};
> +
> +#define max_fan_count 2
> +#define max_pwm_channel_count max_fan_count
> +
> +struct hwmon_data {
> + struct hydro_i_pro_device *hdev;
> + int channel_count;
> + void *channel_data[max_pwm_channel_count];
> +};
> +
> +struct curve_point {
> + u8 temp;
> + u8 pwm;
> +};
> +
> +struct hwmon_fan_data {
> + u8 fan_channel;
> + u16 fan_target;
> + u8 fan_pwm_target;
> + u8 mode;
> + struct curve_point curve[7];
> +};
> +
> +struct curve_point quiet_curve[] = {
> + {
> + .temp = 0x1F,
> + .pwm = 0x15,
> + },
> + {
> + .temp = 0x21,
> + .pwm = 0x1E,
> + },
> + {
> + .temp = 0x24,
> + .pwm = 0x25,
> + },
> + {
> + .temp = 0x27,
> + .pwm = 0x2D,
> + },
> + {
> + .temp = 0x29,
> + .pwm = 0x38,
> + },
> + {
> + .temp = 0x2C,
> + .pwm = 0x4A,
> + },
> + {
> + .temp = 0x2F,
> + .pwm = 0x64,
> + },
> +};
> +
> +#define default_curve quiet_curve
> +
> +static const struct hwmon_channel_info *dual_fan[] = {
> + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN,
> + HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN),
> + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> +
> + NULL
> +};
> +
> +static const struct device_config config_table[] = {
> + {
> + .vendor_id = 0x1b1c,
> + .product_id = 0x0c15,
> + .fancount = 2,
> + .name = "corsair_H100i_pro",
> + .hwmon_info = dual_fan,
> + },
> +};
> +
> +#define BULK_TIMEOUT 100
> +
> +enum opcodes {
> + PWM_FAN_CURVE_CMD = 0x40,
> + PWM_GET_CURRENT_CMD = 0x41,
> + PWM_FAN_TARGET_CMD = 0x42,
> + RPM_FAN_TARGET_CMD = 0x43,
> +};
> +
> +#define SUCCES_LENGTH 3
> +#define SUCCES_CODE 0x12, 0x34
> +
> +static bool check_succes(enum opcodes command, char ret[SUCCES_LENGTH])
> +{
> + char success[SUCCES_LENGTH] = { command, SUCCES_CODE };
> + return memcmp(ret, success, SUCCES_LENGTH) == 0;
> +}
> +
> +static int acquire_lock(struct hydro_i_pro_device *hdev)
> +{
> + int retval = usb_autopm_get_interface(hdev->interface);
> + if (retval)
> + return retval;
> +
> + mutex_lock(&hdev->io_mutex);
> + return 0;
> +}
> +
> +static int transfer_usb(struct hydro_i_pro_device *hdev,
> + unsigned char *send_buf, unsigned char *recv_buf,
> + int send_len, int recv_len)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> +
> + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, send_len, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> + return 0;
> +}
> +
> +static int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data,
> + struct curve_point point[7])
> +{
> + int retval;
> + unsigned char *send_buf = hdev->bulk_out_buffer;
> + unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> + memcpy(fan_data->curve, point, sizeof(struct curve_point) * 7);
> +
> + 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 = transfer_usb(hdev, send_buf, recv_buf, 16, 4);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf)) {
> + dev_warn(
> + &hdev->udev->dev,
> + "failed setting fan pwm/temp curve for %s on channel %d %d,%d,%d\n",
> + hdev->config->name, recv_buf[3], recv_buf[0],
> + recv_buf[1], recv_buf[2]);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int set_fan_target_rpm(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data, u16 val)
> +{
> + int retval;
> + 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 = transfer_usb(hdev, send_buf, recv_buf, 4, 6);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf)) {
> + dev_warn(
> + &hdev->udev->dev,
> + "failed setting fan rpm for %s on channel %d %d,%d,%d\n",
> + hdev->config->name, recv_buf[3], recv_buf[0],
> + recv_buf[1], recv_buf[2]);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int get_fan_current_rpm(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data, long *val)
> +{
> + int retval;
> + 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 = transfer_usb(hdev, send_buf, recv_buf, 2, 6);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf) ||
> + recv_buf[3] != fan_data->fan_channel) {
> + dev_warn(
> + &hdev->udev->dev,
> + "failed retreiving fan pwm for %s on channel %d %d,%d,%d\n",
> + hdev->config->name, recv_buf[3], recv_buf[0],
> + recv_buf[1], recv_buf[2]);
> + return -EINVAL;
> + }
> +
> + *val = ((recv_buf[4]) << 8) + recv_buf[5];
> + return 0;
> +}
> +
> +static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data, u8 val)
> +{
> + int retval;
> + unsigned char *send_buf = hdev->bulk_out_buffer;
> + unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> + fan_data->fan_target = 0;
> + fan_data->fan_pwm_target = val;
> +
> + send_buf[0] = PWM_FAN_TARGET_CMD;
> + send_buf[1] = fan_data->fan_channel;
> + send_buf[3] = fan_data->fan_pwm_target;
> +
> + retval = transfer_usb(hdev, send_buf, recv_buf, 4, 6);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf)) {
> + dev_warn(
> + &hdev->udev->dev,
> + "failed setting fan pwm for %s on channel %d %d,%d,%d\n",
> + hdev->config->name, recv_buf[3], recv_buf[0],
> + recv_buf[1], recv_buf[2]);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static umode_t hwmon_is_visible(const void *d, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + return 0444;
> + break;
> + case hwmon_fan_target:
> + return 0644;
> + break;
> + case hwmon_fan_min:
> + return 0444;
> + break;
> + default:
> + break;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + return 0200;
> + break;
> + case hwmon_pwm_enable:
> + return 0644;
> + break;
> + default:
> + break;
> + }
> + break;
> + default:
> + break;
> + }
> + return 0;
> +}
> +
> +static int hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + struct hwmon_data *data = dev_get_drvdata(dev);
> + struct hydro_i_pro_device *hdev = data->hdev;
> + struct hwmon_fan_data *fan_data;
> + int retval = 0;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_target:
> + fan_data = data->channel_data[channel];
> + if (fan_data->mode != 1)
> + return -EINVAL;
> +
> + if (val < (2 ^ 16) - 2)
> + return -EINVAL;
> +
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = set_fan_target_rpm(hdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + fan_data = data->channel_data[channel];
> + if (fan_data->mode != 1)
> + return -EINVAL;
> +
> + if (val < (2 ^ 8) - 2)
> + return -EINVAL;
> +
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = set_fan_target_pwm(hdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + break;
> + case hwmon_pwm_enable:
> + fan_data = data->channel_data[channel];
> +
> + switch (val) {
> + case 0:
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = set_fan_pwm_curve(hdev, fan_data,
> + default_curve);
> + if (retval)
> + goto cleanup;
> + fan_data->mode = 0;
> + break;
> + case 1:
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + 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;
> + }
> + fan_data->mode = 1;
> + break;
> + case 2:
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + retval = set_fan_pwm_curve(hdev, fan_data,
> + default_curve);
> + if (retval)
> + goto cleanup;
> + fan_data->mode = 2;
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> +cleanup:
> + mutex_unlock(&hdev->io_mutex);
> + usb_autopm_put_interface(hdev->interface);
> +exit:
> + return retval;
> +}
> +
> +static int hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct hwmon_data *data = dev_get_drvdata(dev);
> + struct hydro_i_pro_device *hdev = data->hdev;
> + struct hwmon_fan_data *fan_data;
> + int retval = 0;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + fan_data = data->channel_data[channel];
> +
> + retval = acquire_lock(hdev);
> + if (retval)
> + goto exit;
> +
> + 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:
> + mutex_unlock(&hdev->io_mutex);
> + usb_autopm_put_interface(hdev->interface);
> +exit:
> + return retval;
> +}
> +
> +static const struct hwmon_ops i_pro_ops = {
> + .is_visible = hwmon_is_visible,
> + .read = hwmon_read,
> + .write = hwmon_write,
> +};
> +
> +static void hwmon_init(struct hydro_i_pro_device *hdev)
> +{
> + u8 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);
> +
> + /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/
> + WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
> + data->channel_count = hdev->config->fancount;
> +
> + /* 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 = 0;
> + data->channel_data[fan_id] = fan;
> + }
> +
> + hwmon_info->ops = &i_pro_ops;
> + hwmon_info->info = hdev->config->hwmon_info;
> +
> + data->hdev = hdev;
> + hwmon_dev = devm_hwmon_device_register_with_info(
> + &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
> + dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
> +}
> +
> +const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
> +const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
> +/*
> + * 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(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
> + .driver_info = (kernel_ulong_t)&config_table[0] },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(usb, astk_table);
> +
> +static int init_device(struct usb_device *udev)
> +{
> + int retval;
> +
> + /*
> + * This is needed because when running windows in a vm with proprietary driver
> + * and you switch to this driver, the device will not respond unless you run this.
> + */
> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
> + 0xffff, 0x0000, 0, 0, 0);
> + /*this always returns error*/
> + if (retval)
> + ;
> +
> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> + 0x0002, 0x0000, 0, 0, 0);
> + return retval;
> +}
> +
> +static int deinit_device(struct usb_device *udev)
> +{
> + return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> + 0x0004, 0x0000, 0, 0, 0);
> +}
> +
> +static void astk_delete(struct hydro_i_pro_device *hdev)
> +{
> + usb_put_intf(hdev->interface);
> + usb_put_dev(hdev->udev);
> + kfree(hdev->bulk_in_buffer);
> + kfree(hdev->bulk_out_buffer);
> + kfree(hdev);
> +}
> +
> +static int astk_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct hydro_i_pro_device *hdev =
> + kzalloc(sizeof(struct hydro_i_pro_device), GFP_KERNEL);
> + int retval;
> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> +
> + if (!hdev) {
> + kfree(hdev);
> + retval = -ENOMEM;
> + goto exit;
> + }
> +
> + hdev->config = (const struct device_config *)id->driver_info;
> + /* You should set the driver_info to a pointer to the correct configuration!!*/
> + WARN_ON(hdev->config == NULL);
> +
> + retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> + &bulk_out, NULL, NULL);
> + if (retval) {
> + kfree(hdev);
> + 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 %s\n",
> + hdev->config->name);
> + kfree(hdev);
> + goto exit;
> + }
> +
> + hwmon_init(hdev);
> +
> + usb_set_intfdata(interface, hdev);
> + mutex_init(&hdev->io_mutex);
> +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, "removed hwmon for %s\n",
> + hdev->config->name);
> + deinit_device(hdev->udev);
> + usb_set_intfdata(interface, NULL);
> + astk_delete(hdev);
> +}
> +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,
> +};
> +
> +module_usb_driver(hydro_i_pro_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jaap Aarts <jaap.aarts1@gmail.com>");
> +MODULE_DESCRIPTION("Corsair HXXXi pro device driver");
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum
2020-08-16 0:04 Guenter Roeck
@ 2020-08-16 9:05 ` jaap aarts
0 siblings, 0 replies; 12+ messages in thread
From: jaap aarts @ 2020-08-16 9:05 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, linux-hwmon, linux-usb, Barnabás Pőcze
On Sun, 16 Aug 2020 at 02:04, Guenter Roeck <linux@roeck-us.net> wrote:
> checkpatch --strict says:
>
> total: 7 errors, 12 warnings, 13 checks, 714 lines checked
>
> Guenter
I fixed all of the issues I knew how to fix, one error still occurs
but I have no idea how to fix it. I tried but it always ended up
broken some way or another.
If you could tell me whether or not/how I should add myself
to the maintainers file, that would be useful. And I have a
signed-off-by line but it is not recognised.
Kind regards,
Jaap Aarts
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-08-16 9:35 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-15 21:16 [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum jaap aarts
2020-08-15 21:22 ` jaap aarts
2020-08-15 22:54 ` Barnabás Pőcze
2020-08-16 0:01 ` Guenter Roeck
2020-08-16 0:27 ` Barnabás Pőcze
2020-08-16 0:51 ` Guenter Roeck
2020-08-16 1:35 ` Barnabás Pőcze
2020-08-16 8:57 ` jaap aarts
2020-08-16 9:34 ` kernel test robot
2020-08-16 9:34 ` kernel test robot
2020-08-16 0:04 Guenter Roeck
2020-08-16 9:05 ` jaap aarts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.