[PATCH 0/5] hwmon: (aquacomputer_d5next) Add Aquacomputer Aquaero control

[PATCH 0/5] hwmon: (aquacomputer_d5next) Add Aquacomputer Aquaero control

[Leonard Anderweit]

Add support for controlling the Aquacomputer Aquaero 5/6 fan controllers. The
controllable settings include temperature offset, fan PWM and fan PWM mode.

The first two patches expand the capabilities of the control system without
changing functionality. Patches 3-5 add support for Aquaero control.

Leonard Anderweit (5):
  hwmon: (aquacomputer_d5next) Support one byte control values
  hwmon: (aquacomputer_d5next) Support writing multiple control values
    at once
  hwmon: (aquacomputer_d5next) Add temperature offset control for
    Aquaero
  hwmon: (aquacomputer_d5next) Add fan PWM control for Aquaero
  hwmon: (aquacomputer_d5next) Add PWM mode control for Aquaero

 Documentation/hwmon/aquacomputer_d5next.rst |   7 +-
 drivers/hwmon/aquacomputer_d5next.c         | 247 ++++++++++++++++----
 2 files changed, 213 insertions(+), 41 deletions(-)

-- 
2.39.1

[PATCH 1/5] hwmon: (aquacomputer_d5next) Support one byte control values

[Leonard Anderweit]

Add support for one byte control values. This extends aqc_set_ctrl_val() and
aqc_get_ctrl_val() with a type. Currently supported types are AQC_8 (one byte)
and AQC_BE16 (two bytes big endian). More types will be added in the future.

Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>
---
 drivers/hwmon/aquacomputer_d5next.c | 47 +++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index 12682a610ce7..d69c2396854c 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -70,6 +70,10 @@ static u8 secondary_ctrl_report[] = {
 /* Report IDs for legacy devices */
 #define POWERADJUST3_STATUS_REPORT_ID	0x03
 
+/* Data types for reading and writing control reports */
+#define AQC_8		0
+#define AQC_BE16	1
+
 /* Info, sensor sizes and offsets for most Aquacomputer devices */
 #define AQC_SERIAL_START		0x3
 #define AQC_FIRMWARE_VERSION		0xD
@@ -544,7 +548,7 @@ static int aqc_send_ctrl_data(struct aqc_data *priv)
 }
 
 /* Refreshes the control buffer and stores value at offset in val */
-static int aqc_get_ctrl_val(struct aqc_data *priv, int offset, long *val)
+static int aqc_get_ctrl_val(struct aqc_data *priv, int offset, long *val, int type)
 {
 	int ret;
 
@@ -554,14 +558,23 @@ static int aqc_get_ctrl_val(struct aqc_data *priv, int offset, long *val)
 	if (ret < 0)
 		goto unlock_and_return;
 
-	*val = (s16)get_unaligned_be16(priv->buffer + offset);
+	switch (type) {
+	case AQC_BE16:
+		*val = (s16)get_unaligned_be16(priv->buffer + offset);
+		break;
+	case AQC_8:
+		*val = priv->buffer[offset];
+		break;
+	default:
+		ret = -EINVAL;
+	}
 
 unlock_and_return:
 	mutex_unlock(&priv->mutex);
 	return ret;
 }
 
-static int aqc_set_ctrl_val(struct aqc_data *priv, int offset, long val)
+static int aqc_set_ctrl_val(struct aqc_data *priv, int offset, long val, int type)
 {
 	int ret;
 
@@ -571,7 +584,18 @@ static int aqc_set_ctrl_val(struct aqc_data *priv, int offset, long val)
 	if (ret < 0)
 		goto unlock_and_return;
 
-	put_unaligned_be16((s16)val, priv->buffer + offset);
+	switch (type) {
+	case AQC_BE16:
+		put_unaligned_be16((s16)val, priv->buffer + offset);
+		break;
+	case AQC_8:
+		priv->buffer[offset] = (u8)val;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	if (ret < 0)
+		goto unlock_and_return;
 
 	ret = aqc_send_ctrl_data(priv);
 
@@ -775,7 +799,7 @@ static int aqc_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 		case hwmon_temp_offset:
 			ret =
 			    aqc_get_ctrl_val(priv, priv->temp_ctrl_offset +
-					     channel * AQC_SENSOR_SIZE, val);
+					     channel * AQC_SENSOR_SIZE, val, AQC_BE16);
 			if (ret < 0)
 				return ret;
 
@@ -791,7 +815,8 @@ static int aqc_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 			*val = priv->speed_input[channel];
 			break;
 		case hwmon_fan_pulses:
-			ret = aqc_get_ctrl_val(priv, priv->flow_pulses_ctrl_offset, val);
+			ret = aqc_get_ctrl_val(priv, priv->flow_pulses_ctrl_offset,
+					       val, AQC_BE16);
 			if (ret < 0)
 				return ret;
 			break;
@@ -804,7 +829,8 @@ static int aqc_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 		break;
 	case hwmon_pwm:
 		if (priv->fan_ctrl_offsets) {
-			ret = aqc_get_ctrl_val(priv, priv->fan_ctrl_offsets[channel], val);
+			ret = aqc_get_ctrl_val(priv, priv->fan_ctrl_offsets[channel],
+					       val, AQC_BE16);
 			if (ret < 0)
 				return ret;
 
@@ -877,7 +903,7 @@ static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 			val = clamp_val(val, -15000, 15000) / 10;
 			ret =
 			    aqc_set_ctrl_val(priv, priv->temp_ctrl_offset +
-					     channel * AQC_SENSOR_SIZE, val);
+					     channel * AQC_SENSOR_SIZE, val, AQC_BE16);
 			if (ret < 0)
 				return ret;
 			break;
@@ -889,7 +915,8 @@ static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 		switch (attr) {
 		case hwmon_fan_pulses:
 			val = clamp_val(val, 10, 1000);
-			ret = aqc_set_ctrl_val(priv, priv->flow_pulses_ctrl_offset, val);
+			ret = aqc_set_ctrl_val(priv, priv->flow_pulses_ctrl_offset,
+					       val, AQC_BE16);
 			if (ret < 0)
 				return ret;
 			break;
@@ -906,7 +933,7 @@ static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 					return pwm_value;
 
 				ret = aqc_set_ctrl_val(priv, priv->fan_ctrl_offsets[channel],
-						       pwm_value);
+						       pwm_value, AQC_BE16);
 				if (ret < 0)
 					return ret;
 			}
-- 
2.39.1

[PATCH 2/5] hwmon: (aquacomputer_d5next) Support writing multiple control values at once

[Leonard Anderweit]

Add new function aqc_set_ctrl_vals() to support changing multiple control
values at once while sending only one control report.

Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>
---
 drivers/hwmon/aquacomputer_d5next.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index d69c2396854c..03ef9e0258d2 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -574,9 +574,9 @@ static int aqc_get_ctrl_val(struct aqc_data *priv, int offset, long *val, int ty
 	return ret;
 }
 
-static int aqc_set_ctrl_val(struct aqc_data *priv, int offset, long val, int type)
+static int aqc_set_ctrl_vals(struct aqc_data *priv, int *offsets, long *vals, int *types, int len)
 {
-	int ret;
+	int ret, i;
 
 	mutex_lock(&priv->mutex);
 
@@ -584,15 +584,17 @@ static int aqc_set_ctrl_val(struct aqc_data *priv, int offset, long val, int typ
 	if (ret < 0)
 		goto unlock_and_return;
 
-	switch (type) {
-	case AQC_BE16:
-		put_unaligned_be16((s16)val, priv->buffer + offset);
-		break;
-	case AQC_8:
-		priv->buffer[offset] = (u8)val;
-		break;
-	default:
-		ret = -EINVAL;
+	for (i = 0; i < len; i++) {
+		switch (types[i]) {
+		case AQC_BE16:
+			put_unaligned_be16((s16)vals[i], priv->buffer + offsets[i]);
+			break;
+		case AQC_8:
+			priv->buffer[offsets[i]] = (u8)vals[i];
+			break;
+		default:
+			ret = -EINVAL;
+		}
 	}
 	if (ret < 0)
 		goto unlock_and_return;
@@ -604,6 +606,11 @@ static int aqc_set_ctrl_val(struct aqc_data *priv, int offset, long val, int typ
 	return ret;
 }
 
+static int aqc_set_ctrl_val(struct aqc_data *priv, int offset, long val, int type)
+{
+	return aqc_set_ctrl_vals(priv, &offset, &val, &type, 1);
+}
+
 static umode_t aqc_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr, int channel)
 {
 	const struct aqc_data *priv = data;
-- 
2.39.1

[PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset control for Aquaero

[Leonard Anderweit]

Support sending control reports to Aquacomputer Aquaero. This commit only
enables control of the temperature offset as this works in the same manner as
with already supported devices.
Also, mention temperature offset control for Aquaero in docs.

Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>
---
 Documentation/hwmon/aquacomputer_d5next.rst |  4 +-
 drivers/hwmon/aquacomputer_d5next.c         | 62 ++++++++++++++++-----
 2 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/Documentation/hwmon/aquacomputer_d5next.rst b/Documentation/hwmon/aquacomputer_d5next.rst
index 7d0d015b1a52..de43374940b8 100644
--- a/Documentation/hwmon/aquacomputer_d5next.rst
+++ b/Documentation/hwmon/aquacomputer_d5next.rst
@@ -25,7 +25,7 @@ communicate through proprietary USB HID protocols.
 
 The Aquaero devices expose eight physical, eight virtual and four calculated
 virtual temperature sensors, as well as two flow sensors. The fans expose their
-speed (in RPM), power, voltage and current.
+speed (in RPM), power, voltage and current. The temperature offset can be controlled.
 
 For the D5 Next pump, available sensors are pump and fan speed, power, voltage
 and current, as well as coolant temperature and eight virtual temp sensors. Also
@@ -75,7 +75,7 @@ Sysfs entries
 
 ================ ==============================================================
 temp[1-20]_input Physical/virtual temperature sensors (in millidegrees Celsius)
-temp[1-4]_offset Temperature sensor correction offset (in millidegrees Celsius)
+temp[1-8]_offset Temperature sensor correction offset (in millidegrees Celsius)
 fan[1-8]_input   Pump/fan speed (in RPM) / Flow speed (in dL/h)
 fan5_pulses      Quadro flow sensor pulses
 power[1-8]_input Pump/fan power (in micro Watts)
diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index 03ef9e0258d2..95461e2907e1 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -56,6 +56,7 @@ static const char *const aqc_device_names[] = {
 #define SERIAL_PART_OFFSET		2
 
 #define CTRL_REPORT_ID			0x03
+#define AQUAERO_CTRL_REPORT_ID		0x0b
 
 /* The HID report that the official software always sends
  * after writing values, currently same for all devices
@@ -67,6 +68,14 @@ static u8 secondary_ctrl_report[] = {
 	0x02, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x34, 0xC6
 };
 
+/* Secondary HID report values for Aquaero */
+#define AQUAERO_SECONDARY_CTRL_REPORT_ID	0x06
+#define AQUAERO_SECONDARY_CTRL_REPORT_SIZE	0x07
+
+static u8 aquaero_secondary_ctrl_report[] = {
+	0x06, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00
+};
+
 /* Report IDs for legacy devices */
 #define POWERADJUST3_STATUS_REPORT_ID	0x03
 
@@ -94,6 +103,7 @@ static u8 secondary_ctrl_report[] = {
 #define AQUAERO_NUM_VIRTUAL_SENSORS		8
 #define AQUAERO_NUM_CALC_VIRTUAL_SENSORS	4
 #define AQUAERO_NUM_FLOW_SENSORS		2
+#define AQUAERO_CTRL_REPORT_SIZE		0xa93
 
 /* Sensor report offsets for Aquaero fan controllers */
 #define AQUAERO_SENSOR_START			0x65
@@ -106,6 +116,9 @@ static u8 secondary_ctrl_report[] = {
 #define AQUAERO_FAN_SPEED_OFFSET		0x00
 static u16 aquaero_sensor_fan_offsets[] = { 0x167, 0x173, 0x17f, 0x18B };
 
+/* Control report offsets for the Aquaero fan controllers */
+#define AQUAERO_TEMP_CTRL_OFFSET	0xdb
+
 /* Specs of the D5 Next pump */
 #define D5NEXT_NUM_FANS			2
 #define D5NEXT_NUM_SENSORS		1
@@ -441,6 +454,10 @@ struct aqc_data {
 	const char *name;
 
 	int status_report_id;	/* Used for legacy devices, report is stored in buffer */
+	int ctrl_report_id;
+	int secondary_ctrl_report_id;
+	int secondary_ctrl_report_size;
+	u8 *secondary_ctrl_report;
 
 	int buffer_size;
 	u8 *buffer;
@@ -513,7 +530,7 @@ static int aqc_get_ctrl_data(struct aqc_data *priv)
 	int ret;
 
 	memset(priv->buffer, 0x00, priv->buffer_size);
-	ret = hid_hw_raw_request(priv->hdev, CTRL_REPORT_ID, priv->buffer, priv->buffer_size,
+	ret = hid_hw_raw_request(priv->hdev, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
 				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
 	if (ret < 0)
 		ret = -ENODATA;
@@ -527,23 +544,27 @@ static int aqc_send_ctrl_data(struct aqc_data *priv)
 	int ret;
 	u16 checksum;
 
-	/* Init and xorout value for CRC-16/USB is 0xffff */
-	checksum = crc16(0xffff, priv->buffer + priv->checksum_start, priv->checksum_length);
-	checksum ^= 0xffff;
+	/* Checksum is not needed for Aquaero */
+	if (priv->kind != aquaero) {
+		/* Init and xorout value for CRC-16/USB is 0xffff */
+		checksum = crc16(0xffff, priv->buffer + priv->checksum_start,
+				 priv->checksum_length);
+		checksum ^= 0xffff;
 
-	/* Place the new checksum at the end of the report */
-	put_unaligned_be16(checksum, priv->buffer + priv->checksum_offset);
+		/* Place the new checksum at the end of the report */
+		put_unaligned_be16(checksum, priv->buffer + priv->checksum_offset);
+	}
 
 	/* Send the patched up report back to the device */
-	ret = hid_hw_raw_request(priv->hdev, CTRL_REPORT_ID, priv->buffer, priv->buffer_size,
+	ret = hid_hw_raw_request(priv->hdev, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
 				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret < 0)
 		return ret;
 
 	/* The official software sends this report after every change, so do it here as well */
-	ret = hid_hw_raw_request(priv->hdev, SECONDARY_CTRL_REPORT_ID, secondary_ctrl_report,
-				 SECONDARY_CTRL_REPORT_SIZE, HID_FEATURE_REPORT,
-				 HID_REQ_SET_REPORT);
+	ret = hid_hw_raw_request(priv->hdev, priv->secondary_ctrl_report_id,
+				 priv->secondary_ctrl_report, priv->secondary_ctrl_report_size,
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	return ret;
 }
 
@@ -969,10 +990,10 @@ static const struct hwmon_channel_info *aqc_info[] = {
 			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
-			   HWMON_T_INPUT | HWMON_T_LABEL,
-			   HWMON_T_INPUT | HWMON_T_LABEL,
-			   HWMON_T_INPUT | HWMON_T_LABEL,
-			   HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
+			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
 			   HWMON_T_INPUT | HWMON_T_LABEL,
 			   HWMON_T_INPUT | HWMON_T_LABEL,
 			   HWMON_T_INPUT | HWMON_T_LABEL,
@@ -1275,6 +1296,9 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		priv->num_flow_sensors = AQUAERO_NUM_FLOW_SENSORS;
 		priv->flow_sensors_start_offset = AQUAERO_FLOW_SENSORS_START;
 
+		priv->buffer_size = AQUAERO_CTRL_REPORT_SIZE;
+		priv->temp_ctrl_offset = AQUAERO_TEMP_CTRL_OFFSET;
+
 		priv->temp_label = label_temp_sensors;
 		priv->virtual_temp_label = label_virtual_temp_sensors;
 		priv->calc_virt_temp_label = label_aquaero_calc_temp_sensors;
@@ -1438,6 +1462,11 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		priv->firmware_version_offset = AQUAERO_FIRMWARE_VERSION;
 
 		priv->fan_structure = &aqc_aquaero_fan_structure;
+
+		priv->ctrl_report_id = AQUAERO_CTRL_REPORT_ID;
+		priv->secondary_ctrl_report_id = AQUAERO_SECONDARY_CTRL_REPORT_ID;
+		priv->secondary_ctrl_report_size = AQUAERO_SECONDARY_CTRL_REPORT_SIZE;
+		priv->secondary_ctrl_report = aquaero_secondary_ctrl_report;
 		break;
 	case poweradjust3:
 		priv->status_report_id = POWERADJUST3_STATUS_REPORT_ID;
@@ -1446,6 +1475,11 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		priv->serial_number_start_offset = AQC_SERIAL_START;
 		priv->firmware_version_offset = AQC_FIRMWARE_VERSION;
 
+		priv->ctrl_report_id = CTRL_REPORT_ID;
+		priv->secondary_ctrl_report_id = SECONDARY_CTRL_REPORT_ID;
+		priv->secondary_ctrl_report_size = SECONDARY_CTRL_REPORT_SIZE;
+		priv->secondary_ctrl_report = secondary_ctrl_report;
+
 		if (priv->kind == aquastreamult)
 			priv->fan_structure = &aqc_aquastreamult_fan_structure;
 		else
-- 
2.39.1

[PATCH 4/5] hwmon: (aquacomputer_d5next) Add fan PWM control for Aquaero

[Leonard Anderweit]

Add the option to control fan PWM on Aquacomputer Aquaero. The Aquaero is the
most complex Aquacomputer device, control is therefore more complicated then on
already supported devices.
Setting PWM requires multiple steps. First, an internal static PWM controller
is set to the desired PWM value. Second, the fan is set to use that cwPWMpwm
controller. Last, the minimum and maximum accepted PWM values of the fan are
set to allow all possible PWM values.

Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>
---
 Documentation/hwmon/aquacomputer_d5next.rst |  3 +-
 drivers/hwmon/aquacomputer_d5next.c         | 63 +++++++++++++++++++--
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/Documentation/hwmon/aquacomputer_d5next.rst b/Documentation/hwmon/aquacomputer_d5next.rst
index de43374940b8..2dbb3bd37878 100644
--- a/Documentation/hwmon/aquacomputer_d5next.rst
+++ b/Documentation/hwmon/aquacomputer_d5next.rst
@@ -25,7 +25,8 @@ communicate through proprietary USB HID protocols.
 
 The Aquaero devices expose eight physical, eight virtual and four calculated
 virtual temperature sensors, as well as two flow sensors. The fans expose their
-speed (in RPM), power, voltage and current. The temperature offset can be controlled.
+speed (in RPM), power, voltage and current. The temperature offset and the fan speed
+can be controlled.
 
 For the D5 Next pump, available sensors are pump and fan speed, power, voltage
 and current, as well as coolant temperature and eight virtual temp sensors. Also
diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index 95461e2907e1..02551c26918a 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -104,6 +104,9 @@ static u8 aquaero_secondary_ctrl_report[] = {
 #define AQUAERO_NUM_CALC_VIRTUAL_SENSORS	4
 #define AQUAERO_NUM_FLOW_SENSORS		2
 #define AQUAERO_CTRL_REPORT_SIZE		0xa93
+#define AQUAERO_CTRL_PRESET_ID			0x5c
+#define AQUAERO_CTRL_PRESET_SIZE		0x02
+#define AQUAERO_CTRL_PRESET_START		0x55c
 
 /* Sensor report offsets for Aquaero fan controllers */
 #define AQUAERO_SENSOR_START			0x65
@@ -118,6 +121,10 @@ static u16 aquaero_sensor_fan_offsets[] = { 0x167, 0x173, 0x17f, 0x18B };
 
 /* Control report offsets for the Aquaero fan controllers */
 #define AQUAERO_TEMP_CTRL_OFFSET	0xdb
+#define AQUAERO_FAN_CTRL_MIN_PWR_OFFSET	0x04
+#define AQUAERO_FAN_CTRL_MAX_PWR_OFFSET	0x06
+#define AQUAERO_FAN_CTRL_SRC_OFFSET	0x10
+static u16 aquaero_ctrl_fan_offsets[] = { 0x20c, 0x220, 0x234, 0x248 };
 
 /* Specs of the D5 Next pump */
 #define D5NEXT_NUM_FANS			2
@@ -856,13 +863,22 @@ static int aqc_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 		*val = priv->power_input[channel];
 		break;
 	case hwmon_pwm:
-		if (priv->fan_ctrl_offsets) {
+		switch (priv->kind) {
+		case aquaero:
+			ret = aqc_get_ctrl_val(priv, AQUAERO_CTRL_PRESET_START + channel * AQUAERO_CTRL_PRESET_SIZE,
+					       val, AQC_BE16);
+			if (ret < 0)
+				return ret;
+			*val = aqc_percent_to_pwm(*val);
+			break;
+		default:
 			ret = aqc_get_ctrl_val(priv, priv->fan_ctrl_offsets[channel],
 					       val, AQC_BE16);
 			if (ret < 0)
 				return ret;
 
 			*val = aqc_percent_to_pwm(ret);
+			break;
 		}
 		break;
 	case hwmon_in:
@@ -921,6 +937,10 @@ static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 		     long val)
 {
 	int ret, pwm_value;
+	/* Arrays for setting multiple values at once in the control report */
+	int ctrl_values_offsets[4];
+	long ctrl_values[4];
+	int ctrl_values_types[4];
 	struct aqc_data *priv = dev_get_drvdata(dev);
 
 	switch (type) {
@@ -955,15 +975,47 @@ static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	case hwmon_pwm:
 		switch (attr) {
 		case hwmon_pwm_input:
-			if (priv->fan_ctrl_offsets) {
-				pwm_value = aqc_pwm_to_percent(val);
-				if (pwm_value < 0)
-					return pwm_value;
+			pwm_value = aqc_pwm_to_percent(val);
+			if (pwm_value < 0)
+				return pwm_value;
 
+			switch (priv->kind) {
+			case aquaero:
+				/* Write pwm value to preset corresponding to the channel */
+				ctrl_values_offsets[0] = AQUAERO_CTRL_PRESET_START +
+				    channel * AQUAERO_CTRL_PRESET_SIZE;
+				ctrl_values[0] = pwm_value;
+				ctrl_values_types[0] = AQC_BE16;
+
+				/* Write preset number in fan control source */
+				ctrl_values_offsets[1] = priv->fan_ctrl_offsets[channel] +
+				    AQUAERO_FAN_CTRL_SRC_OFFSET;
+				ctrl_values[1] = AQUAERO_CTRL_PRESET_ID + channel;
+				ctrl_values_types[1] = AQC_BE16;
+
+				/* Set minimum power to 0 to allow the fan to turn off */
+				ctrl_values_offsets[2] = priv->fan_ctrl_offsets[channel] +
+				    AQUAERO_FAN_CTRL_MIN_PWR_OFFSET;
+				ctrl_values[2] = 0;
+				ctrl_values_types[2] = AQC_BE16;
+
+				/* Set maximum power to 255 to allow the fan to reach max speed */
+				ctrl_values_offsets[3] = priv->fan_ctrl_offsets[channel] +
+				    AQUAERO_FAN_CTRL_MAX_PWR_OFFSET;
+				ctrl_values[3] = aqc_pwm_to_percent(255);
+				ctrl_values_types[3] = AQC_BE16;
+
+				ret = aqc_set_ctrl_vals(priv, ctrl_values_offsets, ctrl_values,
+							ctrl_values_types, 4);
+				if (ret < 0)
+					return ret;
+				break;
+			default:
 				ret = aqc_set_ctrl_val(priv, priv->fan_ctrl_offsets[channel],
 						       pwm_value, AQC_BE16);
 				if (ret < 0)
 					return ret;
+				break;
 			}
 			break;
 		default:
@@ -1286,6 +1338,7 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 		priv->num_fans = AQUAERO_NUM_FANS;
 		priv->fan_sensor_offsets = aquaero_sensor_fan_offsets;
+		priv->fan_ctrl_offsets = aquaero_ctrl_fan_offsets;
 
 		priv->num_temp_sensors = AQUAERO_NUM_SENSORS;
 		priv->temp_sensor_start_offset = AQUAERO_SENSOR_START;
-- 
2.39.1

[PATCH 5/5] hwmon: (aquacomputer_d5next) Add PWM mode control for Aquaero

[Leonard Anderweit]

Add PWM mode control for the Aquacomputer Aquaero. On the Aquaero 6 all four
ports can switch between DC and PWM control. On the Aquaero 5 this is only
supported for the fourth port, but changing the setting for the other ports has
no consequences.

Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>
---
 Documentation/hwmon/aquacomputer_d5next.rst |  4 +-
 drivers/hwmon/aquacomputer_d5next.c         | 86 ++++++++++++++++-----
 2 files changed, 70 insertions(+), 20 deletions(-)

diff --git a/Documentation/hwmon/aquacomputer_d5next.rst b/Documentation/hwmon/aquacomputer_d5next.rst
index 2dbb3bd37878..002cb9eecdf5 100644
--- a/Documentation/hwmon/aquacomputer_d5next.rst
+++ b/Documentation/hwmon/aquacomputer_d5next.rst
@@ -26,7 +26,8 @@ communicate through proprietary USB HID protocols.
 The Aquaero devices expose eight physical, eight virtual and four calculated
 virtual temperature sensors, as well as two flow sensors. The fans expose their
 speed (in RPM), power, voltage and current. The temperature offset and the fan speed
-can be controlled.
+can be controlled. The fan PWM mode (DC/PWM) can be controlled. The Aquaero 6 supports
+this on all four fan connectors and the Aquaero 5 only on the fourth connector.
 
 For the D5 Next pump, available sensors are pump and fan speed, power, voltage
 and current, as well as coolant temperature and eight virtual temp sensors. Also
@@ -83,6 +84,7 @@ power[1-8]_input Pump/fan power (in micro Watts)
 in[0-7]_input    Pump/fan voltage (in milli Volts)
 curr[1-8]_input  Pump/fan current (in milli Amperes)
 pwm[1-8]         Fan PWM (0 - 255)
+pwm[1-4]_mode    Fan control mode (0: DC mode; 1: PWM mode)
 ================ ==============================================================
 
 Debugfs entries
diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
index 02551c26918a..2ec00124785f 100644
--- a/drivers/hwmon/aquacomputer_d5next.c
+++ b/drivers/hwmon/aquacomputer_d5next.c
@@ -123,6 +123,7 @@ static u16 aquaero_sensor_fan_offsets[] = { 0x167, 0x173, 0x17f, 0x18B };
 #define AQUAERO_TEMP_CTRL_OFFSET	0xdb
 #define AQUAERO_FAN_CTRL_MIN_PWR_OFFSET	0x04
 #define AQUAERO_FAN_CTRL_MAX_PWR_OFFSET	0x06
+#define AQUAERO_FAN_CTRL_MODE_OFFSET	0x0f
 #define AQUAERO_FAN_CTRL_SRC_OFFSET	0x10
 static u16 aquaero_ctrl_fan_offsets[] = { 0x20c, 0x220, 0x234, 0x248 };
 
@@ -672,10 +673,23 @@ static umode_t aqc_is_visible(const void *data, enum hwmon_sensor_types type, u3
 		break;
 	case hwmon_pwm:
 		if (priv->fan_ctrl_offsets && channel < priv->num_fans) {
-			switch (attr) {
-			case hwmon_pwm_input:
-				return 0644;
+			switch (priv->kind) {
+			case aquaero:
+				switch (attr) {
+				case hwmon_pwm_input:
+				case hwmon_pwm_mode:
+					return 0644;
+				default:
+					break;
+				}
+				break;
 			default:
+				switch (attr) {
+				case hwmon_pwm_input:
+					return 0644;
+				default:
+					break;
+				}
 				break;
 			}
 		}
@@ -863,22 +877,46 @@ static int aqc_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 		*val = priv->power_input[channel];
 		break;
 	case hwmon_pwm:
-		switch (priv->kind) {
-		case aquaero:
-			ret = aqc_get_ctrl_val(priv, AQUAERO_CTRL_PRESET_START + channel * AQUAERO_CTRL_PRESET_SIZE,
-					       val, AQC_BE16);
-			if (ret < 0)
-				return ret;
-			*val = aqc_percent_to_pwm(*val);
+		switch (attr) {
+		case hwmon_pwm_input:
+			switch (priv->kind) {
+			case aquaero:
+				ret = aqc_get_ctrl_val(priv, AQUAERO_CTRL_PRESET_START +
+						       channel * AQUAERO_CTRL_PRESET_SIZE,
+						       val, AQC_BE16);
+				if (ret < 0)
+					return ret;
+				*val = aqc_percent_to_pwm(*val);
+				break;
+			default:
+				ret = aqc_get_ctrl_val(priv, priv->fan_ctrl_offsets[channel],
+						       val, AQC_BE16);
+				if (ret < 0)
+					return ret;
+
+				*val = aqc_percent_to_pwm(ret);
+				break;
+			}
 			break;
-		default:
-			ret = aqc_get_ctrl_val(priv, priv->fan_ctrl_offsets[channel],
-					       val, AQC_BE16);
+		case hwmon_pwm_mode:
+			ret = aqc_get_ctrl_val(priv,
+					       priv->fan_ctrl_offsets[channel] +
+					       AQUAERO_FAN_CTRL_MODE_OFFSET, val, AQC_8);
 			if (ret < 0)
 				return ret;
 
-			*val = aqc_percent_to_pwm(ret);
+			switch (*val) {
+			case 0:	/* DC mode */
+				break;
+			case 2:	/* PWM mode */
+				*val = 1;
+				break;
+			default:
+				break;
+			}
 			break;
+		default:
+			return -EOPNOTSUPP;
 		}
 		break;
 	case hwmon_in:
@@ -936,7 +974,7 @@ static int aqc_read_string(struct device *dev, enum hwmon_sensor_types type, u32
 static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
 		     long val)
 {
-	int ret, pwm_value;
+	int ret, pwm_value, ctrl_mode;
 	/* Arrays for setting multiple values at once in the control report */
 	int ctrl_values_offsets[4];
 	long ctrl_values[4];
@@ -1018,6 +1056,16 @@ static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 				break;
 			}
 			break;
+		case hwmon_pwm_mode:
+			if (val > 1 || val < 0)
+				return -EINVAL;
+			ctrl_mode = 2 * val;
+			ret = aqc_set_ctrl_val(priv,
+					       priv->fan_ctrl_offsets[channel] +
+					       AQUAERO_FAN_CTRL_MODE_OFFSET, ctrl_mode, AQC_8);
+			if (ret < 0)
+				return ret;
+			break;
 		default:
 			break;
 		}
@@ -1077,10 +1125,10 @@ static const struct hwmon_channel_info *aqc_info[] = {
 			   HWMON_P_INPUT | HWMON_P_LABEL,
 			   HWMON_P_INPUT | HWMON_P_LABEL),
 	HWMON_CHANNEL_INFO(pwm,
-			   HWMON_PWM_INPUT,
-			   HWMON_PWM_INPUT,
-			   HWMON_PWM_INPUT,
-			   HWMON_PWM_INPUT,
+			   HWMON_PWM_INPUT | HWMON_PWM_MODE,
+			   HWMON_PWM_INPUT | HWMON_PWM_MODE,
+			   HWMON_PWM_INPUT | HWMON_PWM_MODE,
+			   HWMON_PWM_INPUT | HWMON_PWM_MODE,
 			   HWMON_PWM_INPUT,
 			   HWMON_PWM_INPUT,
 			   HWMON_PWM_INPUT,
-- 
2.39.1

Re: [PATCH 5/5] hwmon: (aquacomputer_d5next) Add PWM mode control for Aquaero

[Guenter Roeck]

On 2/11/23 08:59, Leonard Anderweit wrote:
> Add PWM mode control for the Aquacomputer Aquaero. On the Aquaero 6 all four
> ports can switch between DC and PWM control. On the Aquaero 5 this is only
> supported for the fourth port, but changing the setting for the other ports has
> no consequences.
> 

Adding the capability without actually supporting it raises the expectation
that it works from those who don't know better. Please only provide the
capability to set the mode where it is actually supported.

Otherwise one could argue along the line of 'hey, let's just "enable"
all attributes no matter if supported or not', which would lead to a
lot of confusion. I really hope that isn't done with other attributes
in this driver.

> Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>
> ---
>   Documentation/hwmon/aquacomputer_d5next.rst |  4 +-
>   drivers/hwmon/aquacomputer_d5next.c         | 86 ++++++++++++++++-----
>   2 files changed, 70 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/hwmon/aquacomputer_d5next.rst b/Documentation/hwmon/aquacomputer_d5next.rst
> index 2dbb3bd37878..002cb9eecdf5 100644
> --- a/Documentation/hwmon/aquacomputer_d5next.rst
> +++ b/Documentation/hwmon/aquacomputer_d5next.rst
> @@ -26,7 +26,8 @@ communicate through proprietary USB HID protocols.
>   The Aquaero devices expose eight physical, eight virtual and four calculated
>   virtual temperature sensors, as well as two flow sensors. The fans expose their
>   speed (in RPM), power, voltage and current. The temperature offset and the fan speed
> -can be controlled.
> +can be controlled. The fan PWM mode (DC/PWM) can be controlled. The Aquaero 6 supports
> +this on all four fan connectors and the Aquaero 5 only on the fourth connector.
>   
>   For the D5 Next pump, available sensors are pump and fan speed, power, voltage
>   and current, as well as coolant temperature and eight virtual temp sensors. Also
> @@ -83,6 +84,7 @@ power[1-8]_input Pump/fan power (in micro Watts)
>   in[0-7]_input    Pump/fan voltage (in milli Volts)
>   curr[1-8]_input  Pump/fan current (in milli Amperes)
>   pwm[1-8]         Fan PWM (0 - 255)
> +pwm[1-4]_mode    Fan control mode (0: DC mode; 1: PWM mode)

... and it isn't even mentioned here that the mode only works on
Aquacomputer Aquaero, and only on the 4th port for Aquaero 5.

Really, please don't do that, and I sincerely hope that the driver
doesn't hide (i.e., claim to support when it isn't really supported)
other similar limitations.

Guenter

>   ================ ==============================================================
>   
>   Debugfs entries
> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 02551c26918a..2ec00124785f 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c
> @@ -123,6 +123,7 @@ static u16 aquaero_sensor_fan_offsets[] = { 0x167, 0x173, 0x17f, 0x18B };
>   #define AQUAERO_TEMP_CTRL_OFFSET	0xdb
>   #define AQUAERO_FAN_CTRL_MIN_PWR_OFFSET	0x04
>   #define AQUAERO_FAN_CTRL_MAX_PWR_OFFSET	0x06
> +#define AQUAERO_FAN_CTRL_MODE_OFFSET	0x0f
>   #define AQUAERO_FAN_CTRL_SRC_OFFSET	0x10
>   static u16 aquaero_ctrl_fan_offsets[] = { 0x20c, 0x220, 0x234, 0x248 };
>   
> @@ -672,10 +673,23 @@ static umode_t aqc_is_visible(const void *data, enum hwmon_sensor_types type, u3
>   		break;
>   	case hwmon_pwm:
>   		if (priv->fan_ctrl_offsets && channel < priv->num_fans) {
> -			switch (attr) {
> -			case hwmon_pwm_input:
> -				return 0644;
> +			switch (priv->kind) {
> +			case aquaero:
> +				switch (attr) {
> +				case hwmon_pwm_input:
> +				case hwmon_pwm_mode:
> +					return 0644;
> +				default:
> +					break;
> +				}
> +				break;
>   			default:
> +				switch (attr) {
> +				case hwmon_pwm_input:
> +					return 0644;
> +				default:
> +					break;
> +				}
>   				break;
>   			}
>   		}
> @@ -863,22 +877,46 @@ static int aqc_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>   		*val = priv->power_input[channel];
>   		break;
>   	case hwmon_pwm:
> -		switch (priv->kind) {
> -		case aquaero:
> -			ret = aqc_get_ctrl_val(priv, AQUAERO_CTRL_PRESET_START + channel * AQUAERO_CTRL_PRESET_SIZE,
> -					       val, AQC_BE16);
> -			if (ret < 0)
> -				return ret;
> -			*val = aqc_percent_to_pwm(*val);
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			switch (priv->kind) {
> +			case aquaero:
> +				ret = aqc_get_ctrl_val(priv, AQUAERO_CTRL_PRESET_START +
> +						       channel * AQUAERO_CTRL_PRESET_SIZE,
> +						       val, AQC_BE16);
> +				if (ret < 0)
> +					return ret;
> +				*val = aqc_percent_to_pwm(*val);
> +				break;
> +			default:
> +				ret = aqc_get_ctrl_val(priv, priv->fan_ctrl_offsets[channel],
> +						       val, AQC_BE16);
> +				if (ret < 0)
> +					return ret;
> +
> +				*val = aqc_percent_to_pwm(ret);
> +				break;
> +			}
>   			break;
> -		default:
> -			ret = aqc_get_ctrl_val(priv, priv->fan_ctrl_offsets[channel],
> -					       val, AQC_BE16);
> +		case hwmon_pwm_mode:
> +			ret = aqc_get_ctrl_val(priv,
> +					       priv->fan_ctrl_offsets[channel] +
> +					       AQUAERO_FAN_CTRL_MODE_OFFSET, val, AQC_8);
>   			if (ret < 0)
>   				return ret;
>   
> -			*val = aqc_percent_to_pwm(ret);
> +			switch (*val) {
> +			case 0:	/* DC mode */
> +				break;
> +			case 2:	/* PWM mode */
> +				*val = 1;
> +				break;
> +			default:
> +				break;
> +			}
>   			break;
> +		default:
> +			return -EOPNOTSUPP;
>   		}
>   		break;
>   	case hwmon_in:
> @@ -936,7 +974,7 @@ static int aqc_read_string(struct device *dev, enum hwmon_sensor_types type, u32
>   static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
>   		     long val)
>   {
> -	int ret, pwm_value;
> +	int ret, pwm_value, ctrl_mode;
>   	/* Arrays for setting multiple values at once in the control report */
>   	int ctrl_values_offsets[4];
>   	long ctrl_values[4];
> @@ -1018,6 +1056,16 @@ static int aqc_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>   				break;
>   			}
>   			break;
> +		case hwmon_pwm_mode:
> +			if (val > 1 || val < 0)
> +				return -EINVAL;
> +			ctrl_mode = 2 * val;
> +			ret = aqc_set_ctrl_val(priv,
> +					       priv->fan_ctrl_offsets[channel] +
> +					       AQUAERO_FAN_CTRL_MODE_OFFSET, ctrl_mode, AQC_8);
> +			if (ret < 0)
> +				return ret;
> +			break;
>   		default:
>   			break;
>   		}
> @@ -1077,10 +1125,10 @@ static const struct hwmon_channel_info *aqc_info[] = {
>   			   HWMON_P_INPUT | HWMON_P_LABEL,
>   			   HWMON_P_INPUT | HWMON_P_LABEL),
>   	HWMON_CHANNEL_INFO(pwm,
> -			   HWMON_PWM_INPUT,
> -			   HWMON_PWM_INPUT,
> -			   HWMON_PWM_INPUT,
> -			   HWMON_PWM_INPUT,
> +			   HWMON_PWM_INPUT | HWMON_PWM_MODE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_MODE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_MODE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_MODE,
>   			   HWMON_PWM_INPUT,
>   			   HWMON_PWM_INPUT,
>   			   HWMON_PWM_INPUT,

Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset control for Aquaero

[Guenter Roeck]

On 2/11/23 08:59, Leonard Anderweit wrote:
> Support sending control reports to Aquacomputer Aquaero. This commit only
> enables control of the temperature offset as this works in the same manner as
> with already supported devices.
> Also, mention temperature offset control for Aquaero in docs.
> 
> Signed-off-by: Leonard Anderweit <leonard.anderweit@gmail.com>
> ---
>   Documentation/hwmon/aquacomputer_d5next.rst |  4 +-
>   drivers/hwmon/aquacomputer_d5next.c         | 62 ++++++++++++++++-----
>   2 files changed, 50 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/hwmon/aquacomputer_d5next.rst b/Documentation/hwmon/aquacomputer_d5next.rst
> index 7d0d015b1a52..de43374940b8 100644
> --- a/Documentation/hwmon/aquacomputer_d5next.rst
> +++ b/Documentation/hwmon/aquacomputer_d5next.rst
> @@ -25,7 +25,7 @@ communicate through proprietary USB HID protocols.
>   
>   The Aquaero devices expose eight physical, eight virtual and four calculated
>   virtual temperature sensors, as well as two flow sensors. The fans expose their
> -speed (in RPM), power, voltage and current.
> +speed (in RPM), power, voltage and current. The temperature offset can be controlled.
>   
>   For the D5 Next pump, available sensors are pump and fan speed, power, voltage
>   and current, as well as coolant temperature and eight virtual temp sensors. Also
> @@ -75,7 +75,7 @@ Sysfs entries
>   
>   ================ ==============================================================
>   temp[1-20]_input Physical/virtual temperature sensors (in millidegrees Celsius)
> -temp[1-4]_offset Temperature sensor correction offset (in millidegrees Celsius)
> +temp[1-8]_offset Temperature sensor correction offset (in millidegrees Celsius)
>   fan[1-8]_input   Pump/fan speed (in RPM) / Flow speed (in dL/h)
>   fan5_pulses      Quadro flow sensor pulses
>   power[1-8]_input Pump/fan power (in micro Watts)
> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 03ef9e0258d2..95461e2907e1 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c
> @@ -56,6 +56,7 @@ static const char *const aqc_device_names[] = {
>   #define SERIAL_PART_OFFSET		2
>   
>   #define CTRL_REPORT_ID			0x03
> +#define AQUAERO_CTRL_REPORT_ID		0x0b
>   
>   /* The HID report that the official software always sends
>    * after writing values, currently same for all devices
> @@ -67,6 +68,14 @@ static u8 secondary_ctrl_report[] = {
>   	0x02, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x34, 0xC6
>   };
>   
> +/* Secondary HID report values for Aquaero */
> +#define AQUAERO_SECONDARY_CTRL_REPORT_ID	0x06
> +#define AQUAERO_SECONDARY_CTRL_REPORT_SIZE	0x07
> +
> +static u8 aquaero_secondary_ctrl_report[] = {
> +	0x06, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00
> +};
> +
>   /* Report IDs for legacy devices */
>   #define POWERADJUST3_STATUS_REPORT_ID	0x03
>   
> @@ -94,6 +103,7 @@ static u8 secondary_ctrl_report[] = {
>   #define AQUAERO_NUM_VIRTUAL_SENSORS		8
>   #define AQUAERO_NUM_CALC_VIRTUAL_SENSORS	4
>   #define AQUAERO_NUM_FLOW_SENSORS		2
> +#define AQUAERO_CTRL_REPORT_SIZE		0xa93
>   
>   /* Sensor report offsets for Aquaero fan controllers */
>   #define AQUAERO_SENSOR_START			0x65
> @@ -106,6 +116,9 @@ static u8 secondary_ctrl_report[] = {
>   #define AQUAERO_FAN_SPEED_OFFSET		0x00
>   static u16 aquaero_sensor_fan_offsets[] = { 0x167, 0x173, 0x17f, 0x18B };
>   
> +/* Control report offsets for the Aquaero fan controllers */
> +#define AQUAERO_TEMP_CTRL_OFFSET	0xdb
> +
>   /* Specs of the D5 Next pump */
>   #define D5NEXT_NUM_FANS			2
>   #define D5NEXT_NUM_SENSORS		1
> @@ -441,6 +454,10 @@ struct aqc_data {
>   	const char *name;
>   
>   	int status_report_id;	/* Used for legacy devices, report is stored in buffer */
> +	int ctrl_report_id;
> +	int secondary_ctrl_report_id;
> +	int secondary_ctrl_report_size;
> +	u8 *secondary_ctrl_report;
>   
>   	int buffer_size;
>   	u8 *buffer;
> @@ -513,7 +530,7 @@ static int aqc_get_ctrl_data(struct aqc_data *priv)
>   	int ret;
>   
>   	memset(priv->buffer, 0x00, priv->buffer_size);
> -	ret = hid_hw_raw_request(priv->hdev, CTRL_REPORT_ID, priv->buffer, priv->buffer_size,
> +	ret = hid_hw_raw_request(priv->hdev, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
>   				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
>   	if (ret < 0)
>   		ret = -ENODATA;
> @@ -527,23 +544,27 @@ static int aqc_send_ctrl_data(struct aqc_data *priv)
>   	int ret;
>   	u16 checksum;
>   
> -	/* Init and xorout value for CRC-16/USB is 0xffff */
> -	checksum = crc16(0xffff, priv->buffer + priv->checksum_start, priv->checksum_length);
> -	checksum ^= 0xffff;
> +	/* Checksum is not needed for Aquaero */
> +	if (priv->kind != aquaero) {
> +		/* Init and xorout value for CRC-16/USB is 0xffff */
> +		checksum = crc16(0xffff, priv->buffer + priv->checksum_start,
> +				 priv->checksum_length);
> +		checksum ^= 0xffff;

aquaero is already supported, and the checksum is so far generated
and sent. Is it ignored ? Also, is it guaranteed that _all_ aquero devices
don't need it ?

If it is not needed and ignored, does it really add value to selectively drop it ?

Either case, this change is not mentioned in the commit log, and it
violates the "one logical change per patch" rule. Please split it into
a separate patch and explain why the change is needed.

Another change to separate is the introduction of ctrl_report_id
and the secondary_ctrl_report variables, which is also done silently
and not explained. That should also be a separate patch to simplify
review.

Thanks,
Guenter

>   
> -	/* Place the new checksum at the end of the report */
> -	put_unaligned_be16(checksum, priv->buffer + priv->checksum_offset);
> +		/* Place the new checksum at the end of the report */
> +		put_unaligned_be16(checksum, priv->buffer + priv->checksum_offset);
> +	}
>   
>   	/* Send the patched up report back to the device */
> -	ret = hid_hw_raw_request(priv->hdev, CTRL_REPORT_ID, priv->buffer, priv->buffer_size,
> +	ret = hid_hw_raw_request(priv->hdev, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
>   				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
>   	if (ret < 0)
>   		return ret;
>   
>   	/* The official software sends this report after every change, so do it here as well */
> -	ret = hid_hw_raw_request(priv->hdev, SECONDARY_CTRL_REPORT_ID, secondary_ctrl_report,
> -				 SECONDARY_CTRL_REPORT_SIZE, HID_FEATURE_REPORT,
> -				 HID_REQ_SET_REPORT);
> +	ret = hid_hw_raw_request(priv->hdev, priv->secondary_ctrl_report_id,
> +				 priv->secondary_ctrl_report, priv->secondary_ctrl_report_size,
> +				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
>   	return ret;
>   }
>   
> @@ -969,10 +990,10 @@ static const struct hwmon_channel_info *aqc_info[] = {
>   			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
>   			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
>   			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> -			   HWMON_T_INPUT | HWMON_T_LABEL,
> -			   HWMON_T_INPUT | HWMON_T_LABEL,
> -			   HWMON_T_INPUT | HWMON_T_LABEL,
> -			   HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> +			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
>   			   HWMON_T_INPUT | HWMON_T_LABEL,
>   			   HWMON_T_INPUT | HWMON_T_LABEL,
>   			   HWMON_T_INPUT | HWMON_T_LABEL,
> @@ -1275,6 +1296,9 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   		priv->num_flow_sensors = AQUAERO_NUM_FLOW_SENSORS;
>   		priv->flow_sensors_start_offset = AQUAERO_FLOW_SENSORS_START;
>   
> +		priv->buffer_size = AQUAERO_CTRL_REPORT_SIZE;
> +		priv->temp_ctrl_offset = AQUAERO_TEMP_CTRL_OFFSET;
> +
>   		priv->temp_label = label_temp_sensors;
>   		priv->virtual_temp_label = label_virtual_temp_sensors;
>   		priv->calc_virt_temp_label = label_aquaero_calc_temp_sensors;
> @@ -1438,6 +1462,11 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   		priv->firmware_version_offset = AQUAERO_FIRMWARE_VERSION;
>   
>   		priv->fan_structure = &aqc_aquaero_fan_structure;
> +
> +		priv->ctrl_report_id = AQUAERO_CTRL_REPORT_ID;
> +		priv->secondary_ctrl_report_id = AQUAERO_SECONDARY_CTRL_REPORT_ID;
> +		priv->secondary_ctrl_report_size = AQUAERO_SECONDARY_CTRL_REPORT_SIZE;
> +		priv->secondary_ctrl_report = aquaero_secondary_ctrl_report;
>   		break;
>   	case poweradjust3:
>   		priv->status_report_id = POWERADJUST3_STATUS_REPORT_ID;
> @@ -1446,6 +1475,11 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   		priv->serial_number_start_offset = AQC_SERIAL_START;
>   		priv->firmware_version_offset = AQC_FIRMWARE_VERSION;
>   
> +		priv->ctrl_report_id = CTRL_REPORT_ID;
> +		priv->secondary_ctrl_report_id = SECONDARY_CTRL_REPORT_ID;
> +		priv->secondary_ctrl_report_size = SECONDARY_CTRL_REPORT_SIZE;
> +		priv->secondary_ctrl_report = secondary_ctrl_report;
> +
>   		if (priv->kind == aquastreamult)
>   			priv->fan_structure = &aqc_aquastreamult_fan_structure;
>   		else

Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset control for Aquaero

[Aleksa Savic]

On 2023-02-11 19:08:27 GMT+01:00, Guenter Roeck wrote:
> 
> aquaero is already supported, and the checksum is so far generated
> and sent. Is it ignored ? Also, is it guaranteed that _all_ aquero devices
> don't need it ?

Reading its sensors is currently supported, not writing to it (before these
patches).

The checksum is ignored and not needed for either aquaero 5 (which Leonard has)
nor 6 (which I have).

> 
> If it is not needed and ignored, does it really add value to selectively drop it ?

I think we can indeed remove that check.

Thanks,
Aleksa

> 
> Either case, this change is not mentioned in the commit log, and it
> violates the "one logical change per patch" rule. Please split it into
> a separate patch and explain why the change is needed.
> 
> Another change to separate is the introduction of ctrl_report_id
> and the secondary_ctrl_report variables, which is also done silently
> and not explained. That should also be a separate patch to simplify
> review.
> 
> Thanks,
> Guenter

Re: [PATCH 5/5] hwmon: (aquacomputer_d5next) Add PWM mode control for Aquaero

[Aleksa Savic]

On 2023-02-11 18:41:52 GMT+01:00, Guenter Roeck wrote:
> 
> Adding the capability without actually supporting it raises the expectation
> that it works from those who don't know better. Please only provide the
> capability to set the mode where it is actually supported.
> 
> Otherwise one could argue along the line of 'hey, let's just "enable"
> all attributes no matter if supported or not', which would lead to a
> lot of confusion. I really hope that isn't done with other attributes
> in this driver.

(snip)

> 
> ... and it isn't even mentioned here that the mode only works on
> Aquacomputer Aquaero, and only on the 4th port for Aquaero 5.
> 
> Really, please don't do that, and I sincerely hope that the driver
> doesn't hide (i.e., claim to support when it isn't really supported)
> other similar limitations.
> 
> Guenter

It doesn't, features are exposed if the device supports them. I'm part
to blame for this one, I didn't create an issue at the upstream Github
repo to track this. We'll try to find a way to differentiate between
aquaero 5 and 6 - the aquaero 6 is basically an expanded version of
the 5 with more powerful hardware, firmware seems to be relatively
similar.

Aleksa

Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset control for Aquaero

[Leonard Anderweit]

Am 11.02.23 um 19:54 schrieb Aleksa Savic:
> On 2023-02-11 19:08:27 GMT+01:00, Guenter Roeck wrote:
>>
>> aquaero is already supported, and the checksum is so far generated
>> and sent. Is it ignored ? Also, is it guaranteed that _all_ aquero devices
>> don't need it ?
> 
> Reading its sensors is currently supported, not writing to it (before these
> patches).
> 
> The checksum is ignored and not needed for either aquaero 5 (which Leonard has)
> nor 6 (which I have).
> 
>>
>> If it is not needed and ignored, does it really add value to selectively drop it ?
> 
> I think we can indeed remove that check.

I don't think that check can be removed as the checksum is not appended 
to the control report but is in the last two bytes. So using the 
checksum on Aquaero will overwrite the data at that location. It is 
currently unknown what these two bytes do so I do not want to overwrite 
them.

> 
> Thanks,
> Aleksa
> 
>>
>> Either case, this change is not mentioned in the commit log, and it
>> violates the "one logical change per patch" rule. Please split it into
>> a separate patch and explain why the change is needed.
>>
>> Another change to separate is the introduction of ctrl_report_id
>> and the secondary_ctrl_report variables, which is also done silently
>> and not explained. That should also be a separate patch to simplify
>> review.

I will separate the changes into more commits for the next version.

Regards,
Leonard

>>
>> Thanks,
>> Guenter
> 

Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset control for Aquaero

[Guenter Roeck]

On 2/11/23 11:48, Leonard Anderweit wrote:
> Am 11.02.23 um 19:54 schrieb Aleksa Savic:
>> On 2023-02-11 19:08:27 GMT+01:00, Guenter Roeck wrote:
>>>
>>> aquaero is already supported, and the checksum is so far generated
>>> and sent. Is it ignored ? Also, is it guaranteed that _all_ aquero devices
>>> don't need it ?
>>
>> Reading its sensors is currently supported, not writing to it (before these
>> patches).
>>
>> The checksum is ignored and not needed for either aquaero 5 (which Leonard has)
>> nor 6 (which I have).
>>
>>>
>>> If it is not needed and ignored, does it really add value to selectively drop it ?
>>
>> I think we can indeed remove that check.
> 
> I don't think that check can be removed as the checksum is not appended to the control report but is in the last two bytes. So using the checksum on Aquaero will overwrite the data at that location. It is currently unknown what these two bytes do so I do not want to overwrite them.
> 

The current code _does_ overwrite those bytes, or am I missing something ?

If so, changing that would be a bug fix which really should not be hidden
in a patch making functional changes.

Thanks,
Guenter

>>
>> Thanks,
>> Aleksa
>>
>>>
>>> Either case, this change is not mentioned in the commit log, and it
>>> violates the "one logical change per patch" rule. Please split it into
>>> a separate patch and explain why the change is needed.
>>>
>>> Another change to separate is the introduction of ctrl_report_id
>>> and the secondary_ctrl_report variables, which is also done silently
>>> and not explained. That should also be a separate patch to simplify
>>> review.
> 
> I will separate the changes into more commits for the next version.
> 
> Regards,
> Leonard
> 
>>>
>>> Thanks,
>>> Guenter
>>

Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset control for Aquaero

[Aleksa Savic]

On 2023-02-11 21:48:56 GMT+01:00, Guenter Roeck wrote:
> On 2/11/23 11:48, Leonard Anderweit wrote:
>> Am 11.02.23 um 19:54 schrieb Aleksa Savic:
>>> On 2023-02-11 19:08:27 GMT+01:00, Guenter Roeck wrote:
>>>>
>>>> aquaero is already supported, and the checksum is so far generated
>>>> and sent. Is it ignored ? Also, is it guaranteed that _all_ aquero devices
>>>> don't need it ?
>>>
>>> Reading its sensors is currently supported, not writing to it (before these
>>> patches).
>>>
>>> The checksum is ignored and not needed for either aquaero 5 (which Leonard has)
>>> nor 6 (which I have).
>>>
>>>>
>>>> If it is not needed and ignored, does it really add value to selectively drop it ?
>>>
>>> I think we can indeed remove that check.
>>
>> I don't think that check can be removed as the checksum is not appended 
>> to the control report but is in the last two bytes. So using the 
>> checksum on Aquaero will overwrite the data at that location. It is 
>> currently unknown what these two bytes do so I do not want to overwrite 
>> them.
>>
> 
> The current code _does_ overwrite those bytes, or am I missing something ?
> 
> If so, changing that would be a bug fix which really should not be hidden
> in a patch making functional changes.
> 
> Thanks,
> Guenter

The current code indeed does that because the devices that have writing
implemented work that way (D5 Next, Quadro, Octo - they have priv->fan_ctrl_offsets
set, which is checked in aqc_is_visible()) plus they need the checksum.

Regarding the aquaero checksum, I was under the wrong impression that its
control report contained a place for it. I've just captured a few reports and it
seems to contain purely settings all the way to the end. I've also compared
reports before and after making changes and only the changed settings reflected
in the hex dumps, showing there really is no checksum.

So, to correct myself from earlier: the checksum is not getting ignored; it has
no place in it at all, as the code and testing show.

Thanks,
Aleksa

> 
>>>
>>> Thanks,
>>> Aleksa
>>>
>>>>
>>>> Either case, this change is not mentioned in the commit log, and it
>>>> violates the "one logical change per patch" rule. Please split it into
>>>> a separate patch and explain why the change is needed.
>>>>
>>>> Another change to separate is the introduction of ctrl_report_id
>>>> and the secondary_ctrl_report variables, which is also done silently
>>>> and not explained. That should also be a separate patch to simplify
>>>> review.
>>
>> I will separate the changes into more commits for the next version.
>>
>> Regards,
>> Leonard
>>
>>>>
>>>> Thanks,
>>>> Guenter
>>>
>