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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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,
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 <[email protected]>
> ---
> 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
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
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
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
>
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
>>
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
>>>
>