| Message ID | 20230211165923.17807-6-leonard.anderweit@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (aquacomputer_d5next) Add Aquacomputer Aquaero control | expand |
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,
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
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,
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(-)