This is a driver for the Corsair Commander Pro.
Since this is my first addition to the kernel I would be happy, if you would take a look at it.
I am using this driver on my computer at home and I had a few people test it.
The device:
The Corsair Commander Pro is a USB device, which is usually connected on the internal USB headers on the mainboard.
It has 6 fan connectors, 4 thermal sensor connectors and 2 RGB connectors.
It also reads the voltages on the SATA connector, which it needs for power.
The driver:
I hope hwmon is the right place.
The driver is a HID driver, but it uses usb_bulk_msg for communication.
The device registers as a HID device and there are userspace tools, which use hidraw to access it. I think, it would be good to maintain compatibility with these tools, but the driver can easily be rewritten to be a pure USB driver.
For now it can read temp sensors, fan speeds, voltage values and set pwm values.
It also reads the connection status on the fan headers.
There are a few more things, which I would like to add in the near future:
* Fan curves (not yet sure about the nicest way to provide sysfs access)
* Force 3pin or 4pin mode. (Sometimes the device doesn't detect the fans correctly)
* Setting fixed RPM
I do not work for Corsair and I intend to keep the driver maintainted as long as I use the device privately.
Signed-off-by: Marius Zachmann <[email protected]>
---
Documentation/hwmon/corsair-cpro.rst | 42 +++
MAINTAINERS | 6 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/corsair-cpro.c | 481 +++++++++++++++++++++++++++
5 files changed, 540 insertions(+)
create mode 100644 Documentation/hwmon/corsair-cpro.rst
create mode 100644 drivers/hwmon/corsair-cpro.c
diff --git a/Documentation/hwmon/corsair-cpro.rst b/Documentation/hwmon/corsair-cpro.rst
new file mode 100644
index 000000000000..d4ea1b6b9336
--- /dev/null
+++ b/Documentation/hwmon/corsair-cpro.rst
@@ -0,0 +1,42 @@
+Kernel driver corsair-cpro
+==========================
+
+Supported devices:
+
+ * Corsair Commander Pro
+ * Corsair Commander Pro (1000D)
+
+Author: Marius Zachmann
+
+Description
+-----------
+
+This driver implements the sysfs interface for the Corsair Commander Pro.
+The Corsair Commander Pro is a USB device with 6 fan connectors,
+4 temperature sensor connectors and 2 Corsair LED connectors.
+It can read the voltage levels on the SATA power connector.
+
+Usage Notes
+-----------
+
+Since it is a USB device, hotswapping is possible. The device is autodetected.
+
+Sysfs entries
+-------------
+
+in0_input Voltage on SATA 12v
+in1_input Voltage on SATA 5v
+in2_input Voltage on SATA 3.3v
+
+temp[0-3]_input Connected temperature sensors
+
+fan[0-5]_input Connected fan rpm.
+fan[0-5]_label Shows connection status of the fan as detected by the
+ device.
+ "fanX nc" no connection
+ "fanX 3pin" 3-pin fan detected
+ "fanX 4pin" 4-pin fan detected
+fan[0-5]_enable the driver only reports fan speeds when 1
+pwm[0-5] Sets the fan speed. Values from 0-255.
+ When reading, it reports the last value, which
+ was set by the driver.
diff --git a/MAINTAINERS b/MAINTAINERS
index f08f290df174..169530c7eede 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4386,6 +4386,12 @@ S: Maintained
F: Documentation/hwmon/coretemp.rst
F: drivers/hwmon/coretemp.c
+CORSAIR-CPRO HARDWARE MONITOR DRIVER
+M: Marius Zachmann <[email protected]>
+L: [email protected]
+S: Maintained
+F: drivers/hwmon/corsair-cpro.c
+
COSA/SRP SYNC SERIAL DRIVER
M: Jan "Yenya" Kasprzak <[email protected]>
S: Maintained
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 288ae9f63588..9f5a808768ca 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -439,6 +439,16 @@ config SENSORS_BT1_PVT_ALARMS
the data conversion will be periodically performed and the data will be
saved in the internal driver cache.
+config SENSORS_CORSAIR_CPRO
+ tristate "Corsair Commander Pro controller"
+ depends on USB_HID
+ help
+ If you say yes here you get support for the Corsair Commander Pro
+ controller.
+
+ This driver can also be built as a module. If so, the module
+ will be called corsair-cpro.
+
config SENSORS_DRIVETEMP
tristate "Hard disk drives with temperature sensors"
depends on SCSI && ATA
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3e32c21f5efe..18e1ef74ade7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o
obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
obj-$(CONFIG_SENSORS_BT1_PVT) += bt1-pvt.o
obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
+obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
obj-$(CONFIG_SENSORS_DELL_SMM) += dell-smm-hwmon.o
diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
new file mode 100644
index 000000000000..8ba7fb061184
--- /dev/null
+++ b/drivers/hwmon/corsair-cpro.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * corsair-cpro.c - Linux driver for Corsair Commander Pro
+ * Copyright (C) 2020 Marius Zachmann <[email protected]>
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/hid.h>
+#include <linux/hwmon.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+
+#define hid_to_usb_dev(hid_dev) \
+ to_usb_device(hid_dev->dev.parent->parent)
+
+#define USB_VENDOR_ID_CORSAIR 0x1b1c
+#define USB_PRODUCT_ID_CORSAIR_COMMANDERPRO 0x0c10
+#define USB_PRODUCT_ID_CORSAIR_1000D 0x1d00
+
+#define OUT_BUFFER_SIZE 63
+#define IN_BUFFER_SIZE 16
+#define LABEL_LENGTH 10
+
+#define CTL_GET_TMP 0x11 /* byte 1 is channel, rest zero */
+ /* returns temp for channel in bytes 1 and 2 */
+#define CTL_GET_VOLT 0x12 /* byte 1 = rail number 12, 5, 3.3 */
+ /* returns volt in bytes 1,2 */
+#define CTL_GET_FAN_CNCT 0x20 /* returns in bytes 1-6 */
+ /* 0 for no connect */
+ /* 1 for 3pin, 2 for 4pin */
+#define CTL_GET_FAN_RPM 0x21 /* works like CTL_GET_TMP */
+#define CTL_SET_FAN_FPWM 0x23 /* byte 1 is fan number */
+ /* byte 2 is percentage from 0 - 100 */
+
+struct ccp_device {
+ struct hid_device *hdev;
+ struct device *hwmondev;
+ struct mutex mutex;
+ int pwm[6];
+ int fan_enable[6];
+ char fan_label[6][LABEL_LENGTH];
+
+};
+
+/* send 63 byte buffer and receive response in same buffer */
+static int send_usb_cmd(struct ccp_device *ccp, u8 *buffer)
+{
+ int ret;
+ struct usb_device *udev = hid_to_usb_dev(ccp->hdev);
+ int actual_length;
+
+
+ mutex_lock(&ccp->mutex);
+
+ ret = usb_bulk_msg(udev,
+ usb_sndintpipe(udev, 2),
+ buffer,
+ OUT_BUFFER_SIZE,
+ &actual_length,
+ 1000);
+ if (ret < 0) {
+ hid_err(ccp->hdev,
+ "usb_bulk_msg send failed: %d", ret);
+ goto exit;
+ }
+
+ ret = usb_bulk_msg(udev,
+ usb_rcvintpipe(udev, 1),
+ buffer,
+ IN_BUFFER_SIZE,
+ &actual_length,
+ 1000);
+ if (ret) {
+ hid_err(ccp->hdev,
+ "usb_bulk_msg receive failed: %d", ret);
+ goto exit;
+ }
+
+exit:
+ mutex_unlock(&ccp->mutex);
+ return ret;
+}
+
+/* for commands, which return just a number depending on a channel: */
+/* get_temp, get_volt, get_fan_rpm */
+static int get_data(struct ccp_device *ccp, int command, int channel, long *val)
+{
+ int ret = 0;
+ u8 *buffer;
+
+ buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
+ if (buffer == 0)
+ return -ENOMEM;
+
+ buffer[0] = command;
+ buffer[1] = channel;
+ ret = send_usb_cmd(ccp, buffer);
+ if (ret)
+ return -EIO;
+
+ *val = (buffer[1] << 8) + buffer[2];
+
+ kfree(buffer);
+ return ret;
+}
+
+static int set_pwm(struct ccp_device *ccp, int channel, long val)
+{
+ int ret = 0;
+ u8 *buffer;
+
+ if (val > 255)
+ return -EINVAL;
+
+ ccp->pwm[channel] = val;
+
+ /* The Corsair Commander Pro uses values from 0-100 */
+ val = val << 8;
+ val = val / 255;
+ val = val * 100;
+ val = val >> 8;
+
+ buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
+ if (buffer == 0)
+ return -ENOMEM;
+
+ buffer[0] = CTL_SET_FAN_FPWM;
+ buffer[1] = channel;
+ buffer[2] = val;
+ ret = send_usb_cmd(ccp, buffer);
+
+ kfree(buffer);
+ return ret == 0 ? 0 : -EIO;
+}
+
+static int get_fan_mode_label(struct ccp_device *ccp, int channel)
+{
+ int ret = 0;
+ int mode;
+ u8 *buffer;
+
+ buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
+ if (buffer == 0)
+ return -ENOMEM;
+
+ buffer[0] = CTL_GET_FAN_CNCT;
+ ret = send_usb_cmd(ccp, buffer);
+ if (ret)
+ goto exit;
+
+ mode = buffer[channel+1];
+
+ switch (mode) {
+ case 0:
+ scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
+ "fan%d nc", channel+1);
+ break;
+ case 1:
+ scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
+ "fan%d 3pin", channel+1);
+ break;
+ case 2:
+ scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
+ "fan%d 4pin", channel+1);
+ break;
+ default:
+ dev_err(&ccp->hdev->dev,
+ "Mode Description %d not implemented", mode);
+ break;
+ }
+
+exit:
+ kfree(buffer);
+ return ret == 0 ? 0 : -EIO;
+}
+
+static int get_voltages(struct ccp_device *ccp, int channel, long *val)
+{
+ int ret = 0;
+
+ ret = get_data(ccp, CTL_GET_VOLT, channel, val);
+
+ return ret == 0 ? 0 : -EIO;
+}
+
+static int get_temp(struct ccp_device *ccp, int channel, long *val)
+{
+ int ret = 0;
+
+ ret = get_data(ccp, CTL_GET_TMP, channel, val);
+ *val = *val * 10;
+
+ return ret == 0 ? 0 : -EIO;
+}
+
+static int get_rpm(struct ccp_device *ccp, int channel, long *val)
+{
+ int ret = 0;
+
+ if (ccp->fan_enable[channel] != 1)
+ return -ENODATA;
+
+ ret = get_data(ccp, CTL_GET_FAN_RPM, channel, val);
+
+ return ret == 0 ? 0 : -EIO;
+}
+
+static int ccp_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **str)
+{
+ int ret = 0;
+ struct ccp_device *ccp = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_label:
+ ret = get_fan_mode_label(ccp, channel);
+ *str = ccp->fan_label[channel];
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ int ret = 0;
+ struct ccp_device *ccp = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_input:
+ ret = get_temp(ccp, channel, val);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ ret = get_rpm(ccp, channel, val);
+ break;
+ case hwmon_fan_enable:
+ *val = ccp->fan_enable[channel];
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ *val = ccp->pwm[channel];
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ case hwmon_in:
+ switch (attr) {
+ case hwmon_in_input:
+ ret = get_voltages(ccp, channel, val);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ return ret;
+};
+
+static int ccp_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ int ret = 0;
+ struct ccp_device *ccp = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_enable:
+ ccp->fan_enable[channel] = val;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ set_pwm(ccp, channel, val);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ return ret;
+};
+
+static umode_t ccp_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ switch (type) {
+ case hwmon_chip:
+ switch (attr) {
+ case hwmon_chip_update_interval:
+ return 0644;
+ }
+ break;
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_input:
+ return 0444;
+ }
+ break;
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ return 0444;
+ case hwmon_fan_label:
+ return 0444;
+ case hwmon_fan_enable:
+ return 0644;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ return 0644;
+ }
+ break;
+ case hwmon_in:
+ switch (attr) {
+ case hwmon_in_input:
+ return 0444;
+ }
+ break;
+ default:
+ break;
+ }
+ return 0;
+};
+
+static const struct hwmon_ops ccp_hwmon_ops = {
+ .is_visible = ccp_is_visible,
+ .read = ccp_read,
+ .write = ccp_write,
+ .read_string = ccp_read_string,
+};
+
+static const struct hwmon_channel_info *ccp_info[] = {
+ HWMON_CHANNEL_INFO(chip,
+ HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_MAX,
+ HWMON_T_INPUT | HWMON_T_MAX,
+ HWMON_T_INPUT | HWMON_T_MAX,
+ HWMON_T_INPUT | HWMON_T_MAX
+ ),
+ HWMON_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
+ HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
+ HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
+ HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
+ HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
+ HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL
+ ),
+ HWMON_CHANNEL_INFO(pwm,
+ HWMON_PWM_INPUT,
+ HWMON_PWM_INPUT,
+ HWMON_PWM_INPUT,
+ HWMON_PWM_INPUT,
+ HWMON_PWM_INPUT,
+ HWMON_PWM_INPUT
+ ),
+ HWMON_CHANNEL_INFO(in,
+ HWMON_I_INPUT,
+ HWMON_I_INPUT,
+ HWMON_I_INPUT
+ ),
+ NULL
+};
+
+static const struct hwmon_chip_info ccp_chip_info = {
+ .ops = &ccp_hwmon_ops,
+ .info = ccp_info,
+
+};
+
+static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ struct ccp_device *ccp;
+ int ret = 0;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "hid_parse failed\n");
+ goto exit;
+ }
+
+ ccp = devm_kzalloc(&hdev->dev, sizeof(struct ccp_device), GFP_KERNEL);
+ if (ccp == NULL)
+ goto exit;
+
+ mutex_init(&ccp->mutex);
+
+ ccp->fan_enable[0] = 1;
+ ccp->fan_enable[1] = 1;
+ ccp->fan_enable[2] = 1;
+ ccp->fan_enable[3] = 1;
+ ccp->fan_enable[4] = 1;
+ ccp->fan_enable[5] = 1;
+
+ hid_set_drvdata(hdev, ccp);
+
+ ccp->hdev = hdev;
+ ccp->hwmondev = devm_hwmon_device_register_with_info(&hdev->dev,
+ "corsaircpro",
+ ccp,
+ &ccp_chip_info,
+ 0);
+
+exit:
+ return ret;
+}
+
+static void ccp_remove(struct hid_device *hdev)
+{
+ struct ccp_device *ccp;
+
+ ccp = hid_get_drvdata(hdev);
+ mutex_destroy(&ccp->mutex);
+}
+
+static const struct hid_device_id ccp_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR,
+ USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR,
+ USB_PRODUCT_ID_CORSAIR_1000D) },
+ { }
+};
+
+static struct hid_driver ccp_driver = {
+ .name = "corsair-cpro",
+ .id_table = ccp_devices,
+ .probe = ccp_probe,
+ .remove = ccp_remove
+};
+
+MODULE_DEVICE_TABLE(hid, ccp_devices);
+MODULE_LICENSE("GPL v2");
+
+module_hid_driver(ccp_driver);
--
2.27.0
On Wed, Jun 10, 2020 at 12:26:40AM +0200, Marius Zachmann wrote:
> This is a driver for the Corsair Commander Pro.
> Since this is my first addition to the kernel I would be happy, if you would take a look at it.
> I am using this driver on my computer at home and I had a few people test it.
>
> The device:
>
> The Corsair Commander Pro is a USB device, which is usually connected on the internal USB headers on the mainboard.
> It has 6 fan connectors, 4 thermal sensor connectors and 2 RGB connectors.
> It also reads the voltages on the SATA connector, which it needs for power.
>
> The driver:
>
> I hope hwmon is the right place.
> The driver is a HID driver, but it uses usb_bulk_msg for communication.
> The device registers as a HID device and there are userspace tools, which use hidraw to access it. I think, it would be good to maintain compatibility with these tools, but the driver can easily be rewritten to be a pure USB driver.
> For now it can read temp sensors, fan speeds, voltage values and set pwm values.
> It also reads the connection status on the fan headers.
>
> There are a few more things, which I would like to add in the near future:
> * Fan curves (not yet sure about the nicest way to provide sysfs access)
> * Force 3pin or 4pin mode. (Sometimes the device doesn't detect the fans correctly)
> * Setting fixed RPM
>
> I do not work for Corsair and I intend to keep the driver maintainted as long as I use the device privately.
>
The above should be limited to the driver description. Discussion should be
below the '---'. Either case, make sure to split your lines.
> Signed-off-by: Marius Zachmann <[email protected]>
> ---
> Documentation/hwmon/corsair-cpro.rst | 42 +++
Needs to be added to Documentation/hwmon/index.rst.
> MAINTAINERS | 6 +
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/corsair-cpro.c | 481 +++++++++++++++++++++++++++
> 5 files changed, 540 insertions(+)
> create mode 100644 Documentation/hwmon/corsair-cpro.rst
> create mode 100644 drivers/hwmon/corsair-cpro.c
>
> diff --git a/Documentation/hwmon/corsair-cpro.rst b/Documentation/hwmon/corsair-cpro.rst
> new file mode 100644
> index 000000000000..d4ea1b6b9336
> --- /dev/null
> +++ b/Documentation/hwmon/corsair-cpro.rst
> @@ -0,0 +1,42 @@
> +Kernel driver corsair-cpro
> +==========================
> +
> +Supported devices:
> +
> + * Corsair Commander Pro
> + * Corsair Commander Pro (1000D)
> +
> +Author: Marius Zachmann
> +
> +Description
> +-----------
> +
> +This driver implements the sysfs interface for the Corsair Commander Pro.
> +The Corsair Commander Pro is a USB device with 6 fan connectors,
> +4 temperature sensor connectors and 2 Corsair LED connectors.
> +It can read the voltage levels on the SATA power connector.
> +
> +Usage Notes
> +-----------
> +
> +Since it is a USB device, hotswapping is possible. The device is autodetected.
> +
> +Sysfs entries
> +-------------
> +
> +in0_input Voltage on SATA 12v
> +in1_input Voltage on SATA 5v
> +in2_input Voltage on SATA 3.3v
> +
> +temp[0-3]_input Connected temperature sensors
> +
Index starts with 1 for everything except inX.
> +fan[0-5]_input Connected fan rpm.
> +fan[0-5]_label Shows connection status of the fan as detected by the
> + device.
> + "fanX nc" no connection
> + "fanX 3pin" 3-pin fan detected
> + "fanX 4pin" 4-pin fan detected
> +fan[0-5]_enable the driver only reports fan speeds when 1
> +pwm[0-5] Sets the fan speed. Values from 0-255.
> + When reading, it reports the last value, which
> + was set by the driver.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f08f290df174..169530c7eede 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4386,6 +4386,12 @@ S: Maintained
> F: Documentation/hwmon/coretemp.rst
> F: drivers/hwmon/coretemp.c
>
> +CORSAIR-CPRO HARDWARE MONITOR DRIVER
> +M: Marius Zachmann <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/hwmon/corsair-cpro.c
> +
> COSA/SRP SYNC SERIAL DRIVER
> M: Jan "Yenya" Kasprzak <[email protected]>
> S: Maintained
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 288ae9f63588..9f5a808768ca 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -439,6 +439,16 @@ config SENSORS_BT1_PVT_ALARMS
> the data conversion will be periodically performed and the data will be
> saved in the internal driver cache.
>
> +config SENSORS_CORSAIR_CPRO
> + tristate "Corsair Commander Pro controller"
> + depends on USB_HID
> + help
> + If you say yes here you get support for the Corsair Commander Pro
> + controller.
> +
> + This driver can also be built as a module. If so, the module
> + will be called corsair-cpro.
> +
> config SENSORS_DRIVETEMP
> tristate "Hard disk drives with temperature sensors"
> depends on SCSI && ATA
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3e32c21f5efe..18e1ef74ade7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -56,6 +56,7 @@ obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o
> obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> obj-$(CONFIG_SENSORS_BT1_PVT) += bt1-pvt.o
> obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
> +obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
> obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> obj-$(CONFIG_SENSORS_DELL_SMM) += dell-smm-hwmon.o
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> new file mode 100644
> index 000000000000..8ba7fb061184
> --- /dev/null
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -0,0 +1,481 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
This conflicts with MODULE_LICENSE which explicitly states GPL v2.
> +/*
> + * corsair-cpro.c - Linux driver for Corsair Commander Pro
> + * Copyright (C) 2020 Marius Zachmann <[email protected]>
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/hid.h>
> +#include <linux/hwmon.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +#define hid_to_usb_dev(hid_dev) \
> + to_usb_device(hid_dev->dev.parent->parent)
Unnecessary line break.
> +
> +#define USB_VENDOR_ID_CORSAIR 0x1b1c
> +#define USB_PRODUCT_ID_CORSAIR_COMMANDERPRO 0x0c10
> +#define USB_PRODUCT_ID_CORSAIR_1000D 0x1d00
> +
> +#define OUT_BUFFER_SIZE 63
> +#define IN_BUFFER_SIZE 16
> +#define LABEL_LENGTH 10
> +
> +#define CTL_GET_TMP 0x11 /* byte 1 is channel, rest zero */
> + /* returns temp for channel in bytes 1 and 2 */
> +#define CTL_GET_VOLT 0x12 /* byte 1 = rail number 12, 5, 3.3 */
> + /* returns volt in bytes 1,2 */
> +#define CTL_GET_FAN_CNCT 0x20 /* returns in bytes 1-6 */
> + /* 0 for no connect */
> + /* 1 for 3pin, 2 for 4pin */
> +#define CTL_GET_FAN_RPM 0x21 /* works like CTL_GET_TMP */
> +#define CTL_SET_FAN_FPWM 0x23 /* byte 1 is fan number */
> + /* byte 2 is percentage from 0 - 100 */
> +
Please use tabs consistently after the defined name, eg
#define OUT_BUFFER_SIZE 63
#define IN_BUFFER_SIZE 16
#define LABEL_LENGTH 10
> +struct ccp_device {
> + struct hid_device *hdev;
> + struct device *hwmondev;
Not used anywhere.
> + struct mutex mutex;
> + int pwm[6];
> + int fan_enable[6];
> + char fan_label[6][LABEL_LENGTH];
> +
Unnecessary empty line.
> +};
> +
> +/* send 63 byte buffer and receive response in same buffer */
> +static int send_usb_cmd(struct ccp_device *ccp, u8 *buffer)
> +{
> + int ret;
> + struct usb_device *udev = hid_to_usb_dev(ccp->hdev);
> + int actual_length;
> +
> +
Unnecessary empty line
> + mutex_lock(&ccp->mutex);
> +
> + ret = usb_bulk_msg(udev,
> + usb_sndintpipe(udev, 2),
> + buffer,
> + OUT_BUFFER_SIZE,
> + &actual_length,
> + 1000);
Too many line breaks. All over the place. Please only split lines where
necessary (line length limit is now 100).
> + if (ret < 0) {
> + hid_err(ccp->hdev,
> + "usb_bulk_msg send failed: %d", ret);
> + goto exit;
> + }
> +
> + ret = usb_bulk_msg(udev,
> + usb_rcvintpipe(udev, 1),
> + buffer,
> + IN_BUFFER_SIZE,
> + &actual_length,
> + 1000);
> + if (ret) {
> + hid_err(ccp->hdev,
> + "usb_bulk_msg receive failed: %d", ret);
> + goto exit;
> + }
> +
> +exit:
> + mutex_unlock(&ccp->mutex);
> + return ret;
> +}
> +
> +/* for commands, which return just a number depending on a channel: */
> +/* get_temp, get_volt, get_fan_rpm */
> +static int get_data(struct ccp_device *ccp, int command, int channel, long *val)
> +{
> + int ret = 0;
Unnecessary initialization (almost everywhere).
> + u8 *buffer;
> +
> + buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> + if (buffer == 0)
> + return -ENOMEM;
AFAICS all those commands end up locking anyway. Might as well
move the buffer into struct ccp_device and use it from there
(ie lock while using it).
> +
> + buffer[0] = command;
> + buffer[1] = channel;
> + ret = send_usb_cmd(ccp, buffer);
> + if (ret)
> + return -EIO;
Please do not replace error codes - not just here, everywhere.
> +
> + *val = (buffer[1] << 8) + buffer[2];
> +
> + kfree(buffer);
> + return ret;
Just return 0;
> +}
> +
> +static int set_pwm(struct ccp_device *ccp, int channel, long val)
> +{
> + int ret = 0;
> + u8 *buffer;
> +
> + if (val > 255)
> + return -EINVAL;
How about val < 0 ?
> +
> + ccp->pwm[channel] = val;
> +
> + /* The Corsair Commander Pro uses values from 0-100 */
> + val = val << 8;
> + val = val / 255;
> + val = val * 100;
> + val = val >> 8;
val = DIV_ROUND_CLOSEST(val * 100, 255);
would be much easier to read.
> +
> + buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> + if (buffer == 0)
if (!buffer)
> + return -ENOMEM;
> +
> + buffer[0] = CTL_SET_FAN_FPWM;
> + buffer[1] = channel;
> + buffer[2] = val;
> + ret = send_usb_cmd(ccp, buffer);
> +
> + kfree(buffer);
> + return ret == 0 ? 0 : -EIO;
> +}
> +
> +static int get_fan_mode_label(struct ccp_device *ccp, int channel)
> +{
> + int ret = 0;
> + int mode;
> + u8 *buffer;
> +
> + buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> + if (buffer == 0)
> + return -ENOMEM;
> +
> + buffer[0] = CTL_GET_FAN_CNCT;
> + ret = send_usb_cmd(ccp, buffer);
> + if (ret)
> + goto exit;
> +
> + mode = buffer[channel+1];
Space around operators (use checkpatch --strict to find all locations).
> +
> + switch (mode) {
> + case 0:
> + scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> + "fan%d nc", channel+1);
> + break;
> + case 1:
> + scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> + "fan%d 3pin", channel+1);
> + break;
> + case 2:
> + scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> + "fan%d 4pin", channel+1);
> + break;
> + default:
> + dev_err(&ccp->hdev->dev,
> + "Mode Description %d not implemented", mode);
If this is an error that needs to be logged you have to return an error.
I would suggest to name the label something like "fanX other" instead
in this situation.
> + break;
> + }
> +
> +exit:
> + kfree(buffer);
> + return ret == 0 ? 0 : -EIO;
While it is unacceptable, I am curious why you override error codes everywhere.
> +}
> +
> +static int get_voltages(struct ccp_device *ccp, int channel, long *val)
> +{
> + int ret = 0;
> +
> + ret = get_data(ccp, CTL_GET_VOLT, channel, val);
> +
> + return ret == 0 ? 0 : -EIO;
> +}
> +
> +static int get_temp(struct ccp_device *ccp, int channel, long *val)
> +{
> + int ret = 0;
> +
> + ret = get_data(ccp, CTL_GET_TMP, channel, val);
> + *val = *val * 10;
> +
> + return ret == 0 ? 0 : -EIO;
> +}
> +
> +static int get_rpm(struct ccp_device *ccp, int channel, long *val)
> +{
> + int ret = 0;
> +
> + if (ccp->fan_enable[channel] != 1)
Valid values are 0 and 1, so != 1 is unnecessary. Use
if (!ccp->fan_enable[channel])
instead.
> + return -ENODATA;
> +
> + ret = get_data(ccp, CTL_GET_FAN_RPM, channel, val);
> +
> + return ret == 0 ? 0 : -EIO;
> +}
> +
> +static int ccp_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **str)
> +{
> + int ret = 0;
> + struct ccp_device *ccp = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_label:
> + ret = get_fan_mode_label(ccp, channel);
> + *str = ccp->fan_label[channel];
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + int ret = 0;
> + struct ccp_device *ccp = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_input:
> + ret = get_temp(ccp, channel, val);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + ret = get_rpm(ccp, channel, val);
> + break;
> + case hwmon_fan_enable:
> + *val = ccp->fan_enable[channel];
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + *val = ccp->pwm[channel];
This returns 0 if pwm wasn't set. Is this indeed not readable from the
device ?
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> + case hwmon_in:
> + switch (attr) {
> + case hwmon_in_input:
> + ret = get_voltages(ccp, channel, val);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + return ret;
> +};
> +
> +static int ccp_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + int ret = 0;
> + struct ccp_device *ccp = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_enable:
> + ccp->fan_enable[channel] = val;
Needs to be validated. Accepting random values does not make sense here.
> + break;
> + default:
> + ret = -EINVAL;
-EOPNOTSUPP for those cases.
> + break;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + set_pwm(ccp, channel, val);
Do not ignore errors.
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + return ret;
> +};
> +
> +static umode_t ccp_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + switch (type) {
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + return 0644;
default:
missing everywhere here.
> + }
> + break;
> + case hwmon_temp:
> + switch (attr) {
> + case hwmon_temp_input:
> + return 0444;
> + }
> + break;
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_input:
> + return 0444;
> + case hwmon_fan_label:
> + return 0444;
> + case hwmon_fan_enable:
> + return 0644;
> + }
> + break;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + return 0644;
> + }
> + break;
> + case hwmon_in:
> + switch (attr) {
> + case hwmon_in_input:
> + return 0444;
> + }
> + break;
> + default:
> + break;
> + }
> + return 0;
> +};
> +
> +static const struct hwmon_ops ccp_hwmon_ops = {
> + .is_visible = ccp_is_visible,
> + .read = ccp_read,
> + .write = ccp_write,
> + .read_string = ccp_read_string,
> +};
> +
> +static const struct hwmon_channel_info *ccp_info[] = {
> + HWMON_CHANNEL_INFO(chip,
> + HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
> + HWMON_CHANNEL_INFO(temp,
> + HWMON_T_INPUT | HWMON_T_MAX,
> + HWMON_T_INPUT | HWMON_T_MAX,
> + HWMON_T_INPUT | HWMON_T_MAX,
> + HWMON_T_INPUT | HWMON_T_MAX
> + ),
> + HWMON_CHANNEL_INFO(fan,
> + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL
> + ),
> + HWMON_CHANNEL_INFO(pwm,
> + HWMON_PWM_INPUT,
> + HWMON_PWM_INPUT,
> + HWMON_PWM_INPUT,
> + HWMON_PWM_INPUT,
> + HWMON_PWM_INPUT,
> + HWMON_PWM_INPUT
> + ),
> + HWMON_CHANNEL_INFO(in,
> + HWMON_I_INPUT,
> + HWMON_I_INPUT,
> + HWMON_I_INPUT
> + ),
> + NULL
> +};
> +
> +static const struct hwmon_chip_info ccp_chip_info = {
> + .ops = &ccp_hwmon_ops,
> + .info = ccp_info,
> +
Another unnecessary empty line.
> +};
> +
> +static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> + struct ccp_device *ccp;
> + int ret = 0;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "hid_parse failed\n");
> + goto exit;
return ret;
> + }
> +
> + ccp = devm_kzalloc(&hdev->dev, sizeof(struct ccp_device), GFP_KERNEL);
> + if (ccp == NULL)
> + goto exit;
return -ENOMEM;
> +
> + mutex_init(&ccp->mutex);
> +
> + ccp->fan_enable[0] = 1;
> + ccp->fan_enable[1] = 1;
> + ccp->fan_enable[2] = 1;
> + ccp->fan_enable[3] = 1;
> + ccp->fan_enable[4] = 1;
> + ccp->fan_enable[5] = 1;
> +
> + hid_set_drvdata(hdev, ccp);
> +
> + ccp->hdev = hdev;
> + ccp->hwmondev = devm_hwmon_device_register_with_info(&hdev->dev,
> + "corsaircpro",
> + ccp,
> + &ccp_chip_info,
> + 0);
This can return an ERR_PTR.
> +
> +exit:
> + return ret;
> +}
> +
> +static void ccp_remove(struct hid_device *hdev)
> +{
> + struct ccp_device *ccp;
> +
> + ccp = hid_get_drvdata(hdev);
> + mutex_destroy(&ccp->mutex);
The memory is about to be released, so this doesn't really add value.
Just drop the remove function and forget about mutex_destroy().
> +}
> +
> +static const struct hid_device_id ccp_devices[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR,
> + USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR,
> + USB_PRODUCT_ID_CORSAIR_1000D) },
> + { }
> +};
> +
> +static struct hid_driver ccp_driver = {
> + .name = "corsair-cpro",
> + .id_table = ccp_devices,
> + .probe = ccp_probe,
> + .remove = ccp_remove
> +};
> +
> +MODULE_DEVICE_TABLE(hid, ccp_devices);
> +MODULE_LICENSE("GPL v2");
> +
> +module_hid_driver(ccp_driver);
> --
> 2.27.0
>
>
On 6/10/20 5:03 AM, Marius Zachmann wrote:
[ ... ]
>>> +Kernel driver corsair-cpro
>>> +==========================
>>> +
>>> +Supported devices:
>>> +
>>> + * Corsair Commander Pro
>>> + * Corsair Commander Pro (1000D)
>>> +
>>> +Author: Marius Zachmann
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +This driver implements the sysfs interface for the Corsair Commander Pro.
>>> +The Corsair Commander Pro is a USB device with 6 fan connectors,
>>> +4 temperature sensor connectors and 2 Corsair LED connectors.
>>> +It can read the voltage levels on the SATA power connector.
>>> +
>>> +Usage Notes
>>> +-----------
>>> +
>>> +Since it is a USB device, hotswapping is possible. The device is autodetected.
>>> +
>>> +Sysfs entries
>>> +-------------
>>> +
>>> +in0_input Voltage on SATA 12v
>>> +in1_input Voltage on SATA 5v
>>> +in2_input Voltage on SATA 3.3v
>>> +
>>> +temp[0-3]_input Connected temperature sensors
>>> +
>> Index starts with 1 for everything except inX.
>>
>>> +fan[0-5]_input Connected fan rpm.
>>> +fan[0-5]_label Shows connection status of the fan as detected by the
>>> + device.
>>> + "fanX nc" no connection
>>> + "fanX 3pin" 3-pin fan detected
>>> + "fanX 4pin" 4-pin fan detected
>>> +fan[0-5]_enable the driver only reports fan speeds when 1
>>> +pwm[0-5] Sets the fan speed. Values from 0-255.
>>> + When reading, it reports the last value, which
>>> + was set by the driver.
Change to:
When reading, it reports the last value if it was set by the driver.
Otherwise returns 0.
[ ... ]
>>> + case hwmon_pwm:
>>> + switch (attr) {
>>> + case hwmon_pwm_input:
>>> + *val = ccp->pwm[channel];
>>
>> This returns 0 if pwm wasn't set. Is this indeed not readable from the
>> device ?
>
> I could not find any possibility, because the official Corsair software does not use it.
> I am not sure, whether it would be better to return an error, if it was not set.
>
Bummer. Then change the documentation as suggested above, and add
a comment here explaining that it is unknown how to read pwm values
from the device.
Thanks,
Guenter
On 10.06.20 at 06:01:45 CEST, Guenter Roeck wrote:
> On Wed, Jun 10, 2020 at 12:26:40AM +0200, Marius Zachmann wrote:
> > This is a driver for the Corsair Commander Pro.
> > Since this is my first addition to the kernel I would be happy, if you would take a look at it.
> > I am using this driver on my computer at home and I had a few people test it.
> >
> > The device:
> >
> > The Corsair Commander Pro is a USB device, which is usually connected on the internal USB headers on the mainboard.
> > It has 6 fan connectors, 4 thermal sensor connectors and 2 RGB connectors.
> > It also reads the voltages on the SATA connector, which it needs for power.
> >
> > The driver:
> >
> > I hope hwmon is the right place.
> > The driver is a HID driver, but it uses usb_bulk_msg for communication.
> > The device registers as a HID device and there are userspace tools, which use hidraw to access it. I think, it would be good to maintain compatibility with these tools, but the driver can easily be rewritten to be a pure USB driver.
> > For now it can read temp sensors, fan speeds, voltage values and set pwm values.
> > It also reads the connection status on the fan headers.
> >
> > There are a few more things, which I would like to add in the near future:
> > * Fan curves (not yet sure about the nicest way to provide sysfs access)
> > * Force 3pin or 4pin mode. (Sometimes the device doesn't detect the fans correctly)
> > * Setting fixed RPM
> >
> > I do not work for Corsair and I intend to keep the driver maintainted as long as I use the device privately.
> >
> The above should be limited to the driver description. Discussion should be
> below the '---'. Either case, make sure to split your lines.
>
> > Signed-off-by: Marius Zachmann <[email protected]>
> > ---
> > Documentation/hwmon/corsair-cpro.rst | 42 +++
>
> Needs to be added to Documentation/hwmon/index.rst.
>
> > MAINTAINERS | 6 +
> > drivers/hwmon/Kconfig | 10 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/corsair-cpro.c | 481 +++++++++++++++++++++++++++
> > 5 files changed, 540 insertions(+)
> > create mode 100644 Documentation/hwmon/corsair-cpro.rst
> > create mode 100644 drivers/hwmon/corsair-cpro.c
> >
> > diff --git a/Documentation/hwmon/corsair-cpro.rst b/Documentation/hwmon/corsair-cpro.rst
> > new file mode 100644
> > index 000000000000..d4ea1b6b9336
> > --- /dev/null
> > +++ b/Documentation/hwmon/corsair-cpro.rst
> > @@ -0,0 +1,42 @@
> > +Kernel driver corsair-cpro
> > +==========================
> > +
> > +Supported devices:
> > +
> > + * Corsair Commander Pro
> > + * Corsair Commander Pro (1000D)
> > +
> > +Author: Marius Zachmann
> > +
> > +Description
> > +-----------
> > +
> > +This driver implements the sysfs interface for the Corsair Commander Pro.
> > +The Corsair Commander Pro is a USB device with 6 fan connectors,
> > +4 temperature sensor connectors and 2 Corsair LED connectors.
> > +It can read the voltage levels on the SATA power connector.
> > +
> > +Usage Notes
> > +-----------
> > +
> > +Since it is a USB device, hotswapping is possible. The device is autodetected.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +in0_input Voltage on SATA 12v
> > +in1_input Voltage on SATA 5v
> > +in2_input Voltage on SATA 3.3v
> > +
> > +temp[0-3]_input Connected temperature sensors
> > +
> Index starts with 1 for everything except inX.
>
> > +fan[0-5]_input Connected fan rpm.
> > +fan[0-5]_label Shows connection status of the fan as detected by the
> > + device.
> > + "fanX nc" no connection
> > + "fanX 3pin" 3-pin fan detected
> > + "fanX 4pin" 4-pin fan detected
> > +fan[0-5]_enable the driver only reports fan speeds when 1
> > +pwm[0-5] Sets the fan speed. Values from 0-255.
> > + When reading, it reports the last value, which
> > + was set by the driver.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index f08f290df174..169530c7eede 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4386,6 +4386,12 @@ S: Maintained
> > F: Documentation/hwmon/coretemp.rst
> > F: drivers/hwmon/coretemp.c
> >
> > +CORSAIR-CPRO HARDWARE MONITOR DRIVER
> > +M: Marius Zachmann <[email protected]>
> > +L: [email protected]
> > +S: Maintained
> > +F: drivers/hwmon/corsair-cpro.c
> > +
> > COSA/SRP SYNC SERIAL DRIVER
> > M: Jan "Yenya" Kasprzak <[email protected]>
> > S: Maintained
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 288ae9f63588..9f5a808768ca 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -439,6 +439,16 @@ config SENSORS_BT1_PVT_ALARMS
> > the data conversion will be periodically performed and the data will be
> > saved in the internal driver cache.
> >
> > +config SENSORS_CORSAIR_CPRO
> > + tristate "Corsair Commander Pro controller"
> > + depends on USB_HID
> > + help
> > + If you say yes here you get support for the Corsair Commander Pro
> > + controller.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called corsair-cpro.
> > +
> > config SENSORS_DRIVETEMP
> > tristate "Hard disk drives with temperature sensors"
> > depends on SCSI && ATA
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 3e32c21f5efe..18e1ef74ade7 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -56,6 +56,7 @@ obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o
> > obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> > obj-$(CONFIG_SENSORS_BT1_PVT) += bt1-pvt.o
> > obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
> > +obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
> > obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> > obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> > obj-$(CONFIG_SENSORS_DELL_SMM) += dell-smm-hwmon.o
> > diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> > new file mode 100644
> > index 000000000000..8ba7fb061184
> > --- /dev/null
> > +++ b/drivers/hwmon/corsair-cpro.c
> > @@ -0,0 +1,481 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
>
> This conflicts with MODULE_LICENSE which explicitly states GPL v2.
>
> > +/*
> > + * corsair-cpro.c - Linux driver for Corsair Commander Pro
> > + * Copyright (C) 2020 Marius Zachmann <[email protected]>
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/hid.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/mutex.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +
> > +#define hid_to_usb_dev(hid_dev) \
> > + to_usb_device(hid_dev->dev.parent->parent)
>
> Unnecessary line break.
>
> > +
> > +#define USB_VENDOR_ID_CORSAIR 0x1b1c
> > +#define USB_PRODUCT_ID_CORSAIR_COMMANDERPRO 0x0c10
> > +#define USB_PRODUCT_ID_CORSAIR_1000D 0x1d00
> > +
> > +#define OUT_BUFFER_SIZE 63
> > +#define IN_BUFFER_SIZE 16
> > +#define LABEL_LENGTH 10
> > +
> > +#define CTL_GET_TMP 0x11 /* byte 1 is channel, rest zero */
> > + /* returns temp for channel in bytes 1 and 2 */
> > +#define CTL_GET_VOLT 0x12 /* byte 1 = rail number 12, 5, 3.3 */
> > + /* returns volt in bytes 1,2 */
> > +#define CTL_GET_FAN_CNCT 0x20 /* returns in bytes 1-6 */
> > + /* 0 for no connect */
> > + /* 1 for 3pin, 2 for 4pin */
> > +#define CTL_GET_FAN_RPM 0x21 /* works like CTL_GET_TMP */
> > +#define CTL_SET_FAN_FPWM 0x23 /* byte 1 is fan number */
> > + /* byte 2 is percentage from 0 - 100 */
> > +
>
> Please use tabs consistently after the defined name, eg
>
> #define OUT_BUFFER_SIZE 63
> #define IN_BUFFER_SIZE 16
> #define LABEL_LENGTH 10
>
> > +struct ccp_device {
> > + struct hid_device *hdev;
> > + struct device *hwmondev;
>
> Not used anywhere.
>
> > + struct mutex mutex;
> > + int pwm[6];
> > + int fan_enable[6];
> > + char fan_label[6][LABEL_LENGTH];
> > +
>
> Unnecessary empty line.
>
> > +};
> > +
> > +/* send 63 byte buffer and receive response in same buffer */
> > +static int send_usb_cmd(struct ccp_device *ccp, u8 *buffer)
> > +{
> > + int ret;
> > + struct usb_device *udev = hid_to_usb_dev(ccp->hdev);
> > + int actual_length;
> > +
> > +
> Unnecessary empty line
>
> > + mutex_lock(&ccp->mutex);
> > +
> > + ret = usb_bulk_msg(udev,
> > + usb_sndintpipe(udev, 2),
> > + buffer,
> > + OUT_BUFFER_SIZE,
> > + &actual_length,
> > + 1000);
>
> Too many line breaks. All over the place. Please only split lines where
> necessary (line length limit is now 100).
>
> > + if (ret < 0) {
> > + hid_err(ccp->hdev,
> > + "usb_bulk_msg send failed: %d", ret);
> > + goto exit;
> > + }
> > +
> > + ret = usb_bulk_msg(udev,
> > + usb_rcvintpipe(udev, 1),
> > + buffer,
> > + IN_BUFFER_SIZE,
> > + &actual_length,
> > + 1000);
> > + if (ret) {
> > + hid_err(ccp->hdev,
> > + "usb_bulk_msg receive failed: %d", ret);
> > + goto exit;
> > + }
> > +
> > +exit:
> > + mutex_unlock(&ccp->mutex);
> > + return ret;
> > +}
> > +
> > +/* for commands, which return just a number depending on a channel: */
> > +/* get_temp, get_volt, get_fan_rpm */
> > +static int get_data(struct ccp_device *ccp, int command, int channel, long *val)
> > +{
> > + int ret = 0;
>
> Unnecessary initialization (almost everywhere).
>
> > + u8 *buffer;
> > +
> > + buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> > + if (buffer == 0)
> > + return -ENOMEM;
>
> AFAICS all those commands end up locking anyway. Might as well
> move the buffer into struct ccp_device and use it from there
> (ie lock while using it).
>
> > +
> > + buffer[0] = command;
> > + buffer[1] = channel;
> > + ret = send_usb_cmd(ccp, buffer);
> > + if (ret)
> > + return -EIO;
>
> Please do not replace error codes - not just here, everywhere.
>
> > +
> > + *val = (buffer[1] << 8) + buffer[2];
> > +
> > + kfree(buffer);
> > + return ret;
>
> Just return 0;
>
> > +}
> > +
> > +static int set_pwm(struct ccp_device *ccp, int channel, long val)
> > +{
> > + int ret = 0;
> > + u8 *buffer;
> > +
> > + if (val > 255)
> > + return -EINVAL;
>
> How about val < 0 ?
>
> > +
> > + ccp->pwm[channel] = val;
> > +
> > + /* The Corsair Commander Pro uses values from 0-100 */
> > + val = val << 8;
> > + val = val / 255;
> > + val = val * 100;
> > + val = val >> 8;
>
> val = DIV_ROUND_CLOSEST(val * 100, 255);
>
> would be much easier to read.
>
> > +
> > + buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> > + if (buffer == 0)
>
> if (!buffer)
> > + return -ENOMEM;
> > +
> > + buffer[0] = CTL_SET_FAN_FPWM;
> > + buffer[1] = channel;
> > + buffer[2] = val;
> > + ret = send_usb_cmd(ccp, buffer);
> > +
> > + kfree(buffer);
> > + return ret == 0 ? 0 : -EIO;
> > +}
> > +
> > +static int get_fan_mode_label(struct ccp_device *ccp, int channel)
> > +{
> > + int ret = 0;
> > + int mode;
> > + u8 *buffer;
> > +
> > + buffer = kzalloc(OUT_BUFFER_SIZE, GFP_KERNEL);
> > + if (buffer == 0)
> > + return -ENOMEM;
> > +
> > + buffer[0] = CTL_GET_FAN_CNCT;
> > + ret = send_usb_cmd(ccp, buffer);
> > + if (ret)
> > + goto exit;
> > +
> > + mode = buffer[channel+1];
>
> Space around operators (use checkpatch --strict to find all locations).
>
> > +
> > + switch (mode) {
> > + case 0:
> > + scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> > + "fan%d nc", channel+1);
> > + break;
> > + case 1:
> > + scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> > + "fan%d 3pin", channel+1);
> > + break;
> > + case 2:
> > + scnprintf(ccp->fan_label[channel], LABEL_LENGTH,
> > + "fan%d 4pin", channel+1);
> > + break;
> > + default:
> > + dev_err(&ccp->hdev->dev,
> > + "Mode Description %d not implemented", mode);
>
> If this is an error that needs to be logged you have to return an error.
> I would suggest to name the label something like "fanX other" instead
> in this situation.
>
> > + break;
> > + }
> > +
> > +exit:
> > + kfree(buffer);
> > + return ret == 0 ? 0 : -EIO;
>
> While it is unacceptable, I am curious why you override error codes everywhere.
>
> > +}
> > +
> > +static int get_voltages(struct ccp_device *ccp, int channel, long *val)
> > +{
> > + int ret = 0;
> > +
> > + ret = get_data(ccp, CTL_GET_VOLT, channel, val);
> > +
> > + return ret == 0 ? 0 : -EIO;
> > +}
> > +
> > +static int get_temp(struct ccp_device *ccp, int channel, long *val)
> > +{
> > + int ret = 0;
> > +
> > + ret = get_data(ccp, CTL_GET_TMP, channel, val);
> > + *val = *val * 10;
> > +
> > + return ret == 0 ? 0 : -EIO;
> > +}
> > +
> > +static int get_rpm(struct ccp_device *ccp, int channel, long *val)
> > +{
> > + int ret = 0;
> > +
> > + if (ccp->fan_enable[channel] != 1)
>
> Valid values are 0 and 1, so != 1 is unnecessary. Use
> if (!ccp->fan_enable[channel])
> instead.
>
> > + return -ENODATA;
> > +
> > + ret = get_data(ccp, CTL_GET_FAN_RPM, channel, val);
> > +
> > + return ret == 0 ? 0 : -EIO;
> > +}
> > +
> > +static int ccp_read_string(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, const char **str)
> > +{
> > + int ret = 0;
> > + struct ccp_device *ccp = dev_get_drvdata(dev);
> > +
> > + switch (type) {
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_label:
> > + ret = get_fan_mode_label(ccp, channel);
> > + *str = ccp->fan_label[channel];
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int ccp_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + int ret = 0;
> > + struct ccp_device *ccp = dev_get_drvdata(dev);
> > +
> > + switch (type) {
> > + case hwmon_temp:
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + ret = get_temp(ccp, channel, val);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + break;
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + ret = get_rpm(ccp, channel, val);
> > + break;
> > + case hwmon_fan_enable:
> > + *val = ccp->fan_enable[channel];
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + break;
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + *val = ccp->pwm[channel];
>
> This returns 0 if pwm wasn't set. Is this indeed not readable from the
> device ?
I could not find any possibility, because the official Corsair software does not use it.
I am not sure, whether it would be better to return an error, if it was not set.
>
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + break;
> > + case hwmon_in:
> > + switch (attr) {
> > + case hwmon_in_input:
> > + ret = get_voltages(ccp, channel, val);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > + return ret;
> > +};
> > +
> > +static int ccp_write(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long val)
> > +{
> > + int ret = 0;
> > + struct ccp_device *ccp = dev_get_drvdata(dev);
> > +
> > + switch (type) {
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_enable:
> > + ccp->fan_enable[channel] = val;
>
> Needs to be validated. Accepting random values does not make sense here.
>
> > + break;
> > + default:
> > + ret = -EINVAL;
>
> -EOPNOTSUPP for those cases.
>
> > + break;
> > + }
> > + break;
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + set_pwm(ccp, channel, val);
>
> Do not ignore errors.
>
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + return ret;
> > +};
> > +
> > +static umode_t ccp_is_visible(const void *data, enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + switch (type) {
> > + case hwmon_chip:
> > + switch (attr) {
> > + case hwmon_chip_update_interval:
> > + return 0644;
>
> default:
>
> missing everywhere here.
>
> > + }
> > + break;
> > + case hwmon_temp:
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + return 0444;
> > + }
> > + break;
> > + case hwmon_fan:
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + return 0444;
> > + case hwmon_fan_label:
> > + return 0444;
> > + case hwmon_fan_enable:
> > + return 0644;
> > + }
> > + break;
> > + case hwmon_pwm:
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + return 0644;
> > + }
> > + break;
> > + case hwmon_in:
> > + switch (attr) {
> > + case hwmon_in_input:
> > + return 0444;
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + return 0;
> > +};
> > +
> > +static const struct hwmon_ops ccp_hwmon_ops = {
> > + .is_visible = ccp_is_visible,
> > + .read = ccp_read,
> > + .write = ccp_write,
> > + .read_string = ccp_read_string,
> > +};
> > +
> > +static const struct hwmon_channel_info *ccp_info[] = {
> > + HWMON_CHANNEL_INFO(chip,
> > + HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
> > + HWMON_CHANNEL_INFO(temp,
> > + HWMON_T_INPUT | HWMON_T_MAX,
> > + HWMON_T_INPUT | HWMON_T_MAX,
> > + HWMON_T_INPUT | HWMON_T_MAX,
> > + HWMON_T_INPUT | HWMON_T_MAX
> > + ),
> > + HWMON_CHANNEL_INFO(fan,
> > + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL,
> > + HWMON_F_INPUT | HWMON_F_ENABLE | HWMON_F_LABEL
> > + ),
> > + HWMON_CHANNEL_INFO(pwm,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT
> > + ),
> > + HWMON_CHANNEL_INFO(in,
> > + HWMON_I_INPUT,
> > + HWMON_I_INPUT,
> > + HWMON_I_INPUT
> > + ),
> > + NULL
> > +};
> > +
> > +static const struct hwmon_chip_info ccp_chip_info = {
> > + .ops = &ccp_hwmon_ops,
> > + .info = ccp_info,
> > +
>
>
> Another unnecessary empty line.
>
> > +};
> > +
> > +static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > +{
> > + struct ccp_device *ccp;
> > + int ret = 0;
> > +
> > + ret = hid_parse(hdev);
> > + if (ret) {
> > + hid_err(hdev, "hid_parse failed\n");
> > + goto exit;
>
> return ret;
>
> > + }
> > +
> > + ccp = devm_kzalloc(&hdev->dev, sizeof(struct ccp_device), GFP_KERNEL);
> > + if (ccp == NULL)
> > + goto exit;
>
> return -ENOMEM;
>
> > +
> > + mutex_init(&ccp->mutex);
> > +
> > + ccp->fan_enable[0] = 1;
> > + ccp->fan_enable[1] = 1;
> > + ccp->fan_enable[2] = 1;
> > + ccp->fan_enable[3] = 1;
> > + ccp->fan_enable[4] = 1;
> > + ccp->fan_enable[5] = 1;
> > +
> > + hid_set_drvdata(hdev, ccp);
> > +
> > + ccp->hdev = hdev;
> > + ccp->hwmondev = devm_hwmon_device_register_with_info(&hdev->dev,
> > + "corsaircpro",
> > + ccp,
> > + &ccp_chip_info,
> > + 0);
>
> This can return an ERR_PTR.
>
> > +
> > +exit:
> > + return ret;
> > +}
> > +
> > +static void ccp_remove(struct hid_device *hdev)
> > +{
> > + struct ccp_device *ccp;
> > +
> > + ccp = hid_get_drvdata(hdev);
> > + mutex_destroy(&ccp->mutex);
>
> The memory is about to be released, so this doesn't really add value.
> Just drop the remove function and forget about mutex_destroy().
>
> > +}
> > +
> > +static const struct hid_device_id ccp_devices[] = {
> > + { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR,
> > + USB_PRODUCT_ID_CORSAIR_COMMANDERPRO) },
> > + { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR,
> > + USB_PRODUCT_ID_CORSAIR_1000D) },
> > + { }
> > +};
> > +
> > +static struct hid_driver ccp_driver = {
> > + .name = "corsair-cpro",
> > + .id_table = ccp_devices,
> > + .probe = ccp_probe,
> > + .remove = ccp_remove
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, ccp_devices);
> > +MODULE_LICENSE("GPL v2");
> > +
> > +module_hid_driver(ccp_driver);
>
Thank you for your time!