* [PATCH v4] hwmon: corsair-psu: add support for critical values
@ 2021-03-18 14:17 Wilken Gottwalt
2021-03-18 19:01 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Wilken Gottwalt @ 2021-03-18 14:17 UTC (permalink / raw)
To: linux-kernel; +Cc: Jean Delvare, Guenter Roeck, Jonathan Corbet, linux-hwmon
Adds support for reading the critical values of the temperature sensors
and the rail sensors (voltage and current) once and caches them. Updates
the naming of the constants following a more clear scheme. Also updates
the documentation and fixes some typos. Updates is_visible and ops_read
functions to be more readable.
The new sensors output of a Corsair HX850i will look like this:
corsairpsu-hid-3-1
Adapter: HID adapter
v_in: 230.00 V
v_out +12v: 12.14 V (crit min = +8.41 V, crit max = +15.59 V)
v_out +5v: 5.03 V (crit min = +3.50 V, crit max = +6.50 V)
v_out +3.3v: 3.30 V (crit min = +2.31 V, crit max = +4.30 V)
psu fan: 0 RPM
vrm temp: +46.2°C (crit = +70.0°C)
case temp: +39.8°C (crit = +70.0°C)
power total: 152.00 W
power +12v: 108.00 W
power +5v: 41.00 W
power +3.3v: 5.00 W
curr +12v: 9.00 A (crit max = +85.00 A)
curr +5v: 8.31 A (crit max = +40.00 A)
curr +3.3v: 1.62 A (crit max = +40.00 A)
Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
Changed in v4:
- simplified private data structure and collection of critical values and
unsupported command check
Changes in v3:
- introduced a quirk check function to catch non-working commands
- split is_visible function into subfunctions
- moved the "is value valid" checks into the is_visibility subfunction
- simplified hwmon_ops_read function
- rearranged sysfs entries in the documentation like suggested
Changes in v2:
- simplified reading/caching of critical values and hwmon_ops_read function
- removed unnecessary debug output and comments
---
Documentation/hwmon/corsair-psu.rst | 13 +-
drivers/hwmon/corsair-psu.c | 325 +++++++++++++++++++++++-----
2 files changed, 282 insertions(+), 56 deletions(-)
diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst
index 396b95c9a76a..e8378e7a1d8c 100644
--- a/Documentation/hwmon/corsair-psu.rst
+++ b/Documentation/hwmon/corsair-psu.rst
@@ -47,19 +47,30 @@ Sysfs entries
======================= ========================================================
curr1_input Total current usage
curr2_input Current on the 12v psu rail
+curr2_crit Current max critical value on the 12v psu rail
curr3_input Current on the 5v psu rail
+curr3_crit Current max critical value on the 5v psu rail
curr4_input Current on the 3.3v psu rail
+curr4_crit Current max critical value on the 3.3v psu rail
fan1_input RPM of psu fan
in0_input Voltage of the psu ac input
in1_input Voltage of the 12v psu rail
+in1_crit Voltage max critical value on the 12v psu rail
+in1_lcrit Voltage min critical value on the 12v psu rail
in2_input Voltage of the 5v psu rail
-in3_input Voltage of the 3.3 psu rail
+in2_crit Voltage max critical value on the 5v psu rail
+in2_lcrit Voltage min critical value on the 5v psu rail
+in3_input Voltage of the 3.3v psu rail
+in3_crit Voltage max critical value on the 3.3v psu rail
+in3_lcrit Voltage min critical value on the 3.3v psu rail
power1_input Total power usage
power2_input Power usage of the 12v psu rail
power3_input Power usage of the 5v psu rail
power4_input Power usage of the 3.3v psu rail
temp1_input Temperature of the psu vrm component
+temp1_crit Temperature max cirtical value of the psu vrm component
temp2_input Temperature of the psu case
+temp2_crit Temperature max critical value of psu case
======================= ========================================================
Usage Notes
diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index b0953eeeb2d3..3a5807e4a2ef 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -53,11 +53,17 @@
#define CMD_TIMEOUT_MS 250
#define SECONDS_PER_HOUR (60 * 60)
#define SECONDS_PER_DAY (SECONDS_PER_HOUR * 24)
+#define RAIL_COUNT 3 /* 3v3 + 5v + 12v */
+#define TEMP_COUNT 2
#define PSU_CMD_SELECT_RAIL 0x00 /* expects length 2 */
-#define PSU_CMD_IN_VOLTS 0x88 /* the rest of the commands expect length 3 */
+#define PSU_CMD_RAIL_VOLTS_HCRIT 0x40 /* the rest of the commands expect length 3 */
+#define PSU_CMD_RAIL_VOLTS_LCRIT 0x44
+#define PSU_CMD_RAIL_AMPS_HCRIT 0x46
+#define PSU_CMD_TEMP_HCRIT 0x4F
+#define PSU_CMD_IN_VOLTS 0x88
#define PSU_CMD_IN_AMPS 0x89
-#define PSU_CMD_RAIL_OUT_VOLTS 0x8B
+#define PSU_CMD_RAIL_VOLTS 0x8B
#define PSU_CMD_RAIL_AMPS 0x8C
#define PSU_CMD_TEMP0 0x8D
#define PSU_CMD_TEMP1 0x8E
@@ -116,6 +122,15 @@ struct corsairpsu_data {
u8 *cmd_buffer;
char vendor[REPLY_SIZE];
char product[REPLY_SIZE];
+ long temp_crit[TEMP_COUNT];
+ long in_crit[RAIL_COUNT];
+ long in_lcrit[RAIL_COUNT];
+ long curr_crit[RAIL_COUNT];
+ u8 temp_crit_support;
+ u8 in_crit_support;
+ u8 in_lcrit_support;
+ u8 curr_crit_support;
+ bool in_curr_cmd_support; /* not all commands are supported on every PSU */
};
/* some values are SMBus LINEAR11 data which need a conversion */
@@ -193,7 +208,10 @@ static int corsairpsu_request(struct corsairpsu_data *priv, u8 cmd, u8 rail, voi
mutex_lock(&priv->lock);
switch (cmd) {
- case PSU_CMD_RAIL_OUT_VOLTS:
+ case PSU_CMD_RAIL_VOLTS_HCRIT:
+ case PSU_CMD_RAIL_VOLTS_LCRIT:
+ case PSU_CMD_RAIL_AMPS_HCRIT:
+ case PSU_CMD_RAIL_VOLTS:
case PSU_CMD_RAIL_AMPS:
case PSU_CMD_RAIL_WATTS:
ret = corsairpsu_usb_cmd(priv, 2, PSU_CMD_SELECT_RAIL, rail, NULL);
@@ -229,9 +247,13 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
*/
tmp = ((long)data[3] << 24) + (data[2] << 16) + (data[1] << 8) + data[0];
switch (cmd) {
+ case PSU_CMD_RAIL_VOLTS_HCRIT:
+ case PSU_CMD_RAIL_VOLTS_LCRIT:
+ case PSU_CMD_RAIL_AMPS_HCRIT:
+ case PSU_CMD_TEMP_HCRIT:
case PSU_CMD_IN_VOLTS:
case PSU_CMD_IN_AMPS:
- case PSU_CMD_RAIL_OUT_VOLTS:
+ case PSU_CMD_RAIL_VOLTS:
case PSU_CMD_RAIL_AMPS:
case PSU_CMD_TEMP0:
case PSU_CMD_TEMP1:
@@ -256,75 +278,265 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
return ret;
}
-static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type,
- u32 attr, int channel)
+static void corsairpsu_get_criticals(struct corsairpsu_data *priv)
{
- if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label))
- return 0444;
- else if (type == hwmon_fan && (attr == hwmon_fan_input || attr == hwmon_fan_label))
- return 0444;
- else if (type == hwmon_power && (attr == hwmon_power_input || attr == hwmon_power_label))
- return 0444;
- else if (type == hwmon_in && (attr == hwmon_in_input || attr == hwmon_in_label))
+ long tmp;
+ int rail;
+
+ for (rail = 0; rail < TEMP_COUNT; ++rail) {
+ if (!corsairpsu_get_value(priv, PSU_CMD_TEMP_HCRIT, rail, &tmp)) {
+ priv->temp_crit_support |= BIT(rail);
+ priv->temp_crit[rail] = tmp;
+ }
+ }
+
+ for (rail = 0; rail < RAIL_COUNT; ++rail) {
+ if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_HCRIT, rail, &tmp)) {
+ priv->in_crit_support |= BIT(rail);
+ priv->in_crit[rail] = tmp;
+ }
+
+ if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_LCRIT, rail, &tmp)) {
+ priv->in_lcrit_support |= BIT(rail);
+ priv->in_lcrit[rail] = tmp;
+ }
+
+ if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS_HCRIT, rail, &tmp)) {
+ priv->curr_crit_support |= BIT(rail);
+ priv->curr_crit[rail] = tmp;
+ }
+ }
+}
+
+static void corsairpsu_check_cmd_support(struct corsairpsu_data *priv)
+{
+ long tmp;
+
+ priv->in_curr_cmd_support = !corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, &tmp);
+}
+
+static umode_t corsairpsu_hwmon_temp_is_visible(const struct corsairpsu_data *priv, u32 attr,
+ int channel)
+{
+ umode_t res = 0444;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ case hwmon_temp_label:
+ case hwmon_temp_crit:
+ if (channel > 0 && !(priv->temp_crit_support & BIT(channel - 1)))
+ res = 0;
+ break;
+ default:
+ break;
+ }
+
+ return res;
+}
+
+static umode_t corsairpsu_hwmon_fan_is_visible(const struct corsairpsu_data *priv, u32 attr,
+ int channel)
+{
+ switch (attr) {
+ case hwmon_fan_input:
+ case hwmon_fan_label:
return 0444;
- else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label))
+ default:
+ return 0;
+ }
+}
+
+static umode_t corsairpsu_hwmon_power_is_visible(const struct corsairpsu_data *priv, u32 attr,
+ int channel)
+{
+ switch (attr) {
+ case hwmon_power_input:
+ case hwmon_power_label:
return 0444;
+ default:
+ return 0;
+ };
+}
- return 0;
+static umode_t corsairpsu_hwmon_in_is_visible(const struct corsairpsu_data *priv, u32 attr,
+ int channel)
+{
+ umode_t res = 0444;
+
+ switch (attr) {
+ case hwmon_in_input:
+ case hwmon_in_label:
+ case hwmon_in_crit:
+ if (channel > 0 && !(priv->in_crit_support & BIT(channel - 1)))
+ res = 0;
+ break;
+ case hwmon_in_lcrit:
+ if (channel > 0 && !(priv->in_lcrit_support & BIT(channel - 1)))
+ res = 0;
+ break;
+ default:
+ break;
+ };
+
+ return res;
}
-static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
- int channel, long *val)
+static umode_t corsairpsu_hwmon_curr_is_visible(const struct corsairpsu_data *priv, u32 attr,
+ int channel)
{
- struct corsairpsu_data *priv = dev_get_drvdata(dev);
- int ret;
+ umode_t res = 0444;
- if (type == hwmon_temp && attr == hwmon_temp_input && channel < 2) {
- ret = corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : PSU_CMD_TEMP0, channel,
- val);
- } else if (type == hwmon_fan && attr == hwmon_fan_input) {
- ret = corsairpsu_get_value(priv, PSU_CMD_FAN, 0, val);
- } else if (type == hwmon_power && attr == hwmon_power_input) {
+ switch (attr) {
+ case hwmon_curr_input:
+ if (channel == 0 && !priv->in_curr_cmd_support)
+ res = 0;
+ break;
+ case hwmon_curr_label:
+ case hwmon_curr_crit:
+ if (channel > 0 && !(priv->curr_crit_support & BIT(channel - 1)))
+ res = 0;
+ break;
+ default:
+ break;
+ }
+
+ return res;
+}
+
+static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ const struct corsairpsu_data *priv = data;
+
+ switch (type) {
+ case hwmon_temp:
+ return corsairpsu_hwmon_temp_is_visible(priv, attr, channel);
+ case hwmon_fan:
+ return corsairpsu_hwmon_fan_is_visible(priv, attr, channel);
+ case hwmon_power:
+ return corsairpsu_hwmon_power_is_visible(priv, attr, channel);
+ case hwmon_in:
+ return corsairpsu_hwmon_in_is_visible(priv, attr, channel);
+ case hwmon_curr:
+ return corsairpsu_hwmon_curr_is_visible(priv, attr, channel);
+ default:
+ return 0;
+ }
+}
+
+static int corsairpsu_hwmon_temp_read(struct corsairpsu_data *priv, u32 attr, int channel,
+ long *val)
+{
+ int err = -EOPNOTSUPP;
+
+ switch (attr) {
+ case hwmon_temp_input:
+ return corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : PSU_CMD_TEMP0,
+ channel, val);
+ case hwmon_temp_crit:
+ *val = priv->temp_crit[channel];
+ err = 0;
+ break;
+ default:
+ break;
+ }
+
+ return err;
+}
+
+static int corsairpsu_hwmon_power_read(struct corsairpsu_data *priv, u32 attr, int channel,
+ long *val)
+{
+ if (attr == hwmon_power_input) {
switch (channel) {
case 0:
- ret = corsairpsu_get_value(priv, PSU_CMD_TOTAL_WATTS, 0, val);
- break;
+ return corsairpsu_get_value(priv, PSU_CMD_TOTAL_WATTS, 0, val);
case 1 ... 3:
- ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1, val);
- break;
+ return corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1, val);
default:
- return -EOPNOTSUPP;
+ break;
}
- } else if (type == hwmon_in && attr == hwmon_in_input) {
+ }
+
+ return -EOPNOTSUPP;
+}
+
+static int corsairpsu_hwmon_in_read(struct corsairpsu_data *priv, u32 attr, int channel, long *val)
+{
+ int err = -EOPNOTSUPP;
+
+ switch (attr) {
+ case hwmon_in_input:
switch (channel) {
case 0:
- ret = corsairpsu_get_value(priv, PSU_CMD_IN_VOLTS, 0, val);
- break;
+ return corsairpsu_get_value(priv, PSU_CMD_IN_VOLTS, 0, val);
case 1 ... 3:
- ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_OUT_VOLTS, channel - 1, val);
- break;
+ return corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS, channel - 1, val);
default:
- return -EOPNOTSUPP;
+ break;
}
- } else if (type == hwmon_curr && attr == hwmon_curr_input) {
+ break;
+ case hwmon_in_crit:
+ *val = priv->in_crit[channel - 1];
+ err = 0;
+ break;
+ case hwmon_in_lcrit:
+ *val = priv->in_lcrit[channel - 1];
+ err = 0;
+ break;
+ }
+
+ return err;
+}
+
+static int corsairpsu_hwmon_curr_read(struct corsairpsu_data *priv, u32 attr, int channel,
+ long *val)
+{
+ int err = -EOPNOTSUPP;
+
+ switch (attr) {
+ case hwmon_curr_input:
switch (channel) {
case 0:
- ret = corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, val);
- break;
+ return corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, val);
case 1 ... 3:
- ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val);
- break;
+ return corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val);
default:
- return -EOPNOTSUPP;
+ break;
}
- } else {
- return -EOPNOTSUPP;
+ break;
+ case hwmon_curr_crit:
+ *val = priv->curr_crit[channel - 1];
+ err = 0;
+ break;
+ default:
+ break;
}
- if (ret < 0)
- return ret;
+ return err;
+}
- return 0;
+static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct corsairpsu_data *priv = dev_get_drvdata(dev);
+
+ switch (type) {
+ case hwmon_temp:
+ return corsairpsu_hwmon_temp_read(priv, attr, channel, val);
+ case hwmon_fan:
+ if (attr == hwmon_fan_input)
+ return corsairpsu_get_value(priv, PSU_CMD_FAN, 0, val);
+ return -EOPNOTSUPP;
+ case hwmon_power:
+ return corsairpsu_hwmon_power_read(priv, attr, channel, val);
+ case hwmon_in:
+ return corsairpsu_hwmon_in_read(priv, attr, channel, val);
+ case hwmon_curr:
+ return corsairpsu_hwmon_curr_read(priv, attr, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
}
static int corsairpsu_hwmon_ops_read_string(struct device *dev, enum hwmon_sensor_types type,
@@ -360,8 +572,8 @@ static const struct hwmon_channel_info *corsairpsu_info[] = {
HWMON_CHANNEL_INFO(chip,
HWMON_C_REGISTER_TZ),
HWMON_CHANNEL_INFO(temp,
- HWMON_T_INPUT | HWMON_T_LABEL,
- HWMON_T_INPUT | HWMON_T_LABEL),
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT),
HWMON_CHANNEL_INFO(fan,
HWMON_F_INPUT | HWMON_F_LABEL),
HWMON_CHANNEL_INFO(power,
@@ -371,14 +583,14 @@ static const struct hwmon_channel_info *corsairpsu_info[] = {
HWMON_P_INPUT | HWMON_P_LABEL),
HWMON_CHANNEL_INFO(in,
HWMON_I_INPUT | HWMON_I_LABEL,
- HWMON_I_INPUT | HWMON_I_LABEL,
- HWMON_I_INPUT | HWMON_I_LABEL,
- HWMON_I_INPUT | HWMON_I_LABEL),
+ HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT,
+ HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT,
+ HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT),
HWMON_CHANNEL_INFO(curr,
HWMON_C_INPUT | HWMON_C_LABEL,
- HWMON_C_INPUT | HWMON_C_LABEL,
- HWMON_C_INPUT | HWMON_C_LABEL,
- HWMON_C_INPUT | HWMON_C_LABEL),
+ HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT,
+ HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT,
+ HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT),
NULL
};
@@ -513,6 +725,9 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
goto fail_and_stop;
}
+ corsairpsu_get_criticals(priv);
+ corsairpsu_check_cmd_support(priv);
+
priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv,
&corsairpsu_chip_info, 0);
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v4] hwmon: corsair-psu: add support for critical values
2021-03-18 14:17 [PATCH v4] hwmon: corsair-psu: add support for critical values Wilken Gottwalt
@ 2021-03-18 19:01 ` Guenter Roeck
2021-03-19 8:58 ` Wilken Gottwalt
0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2021-03-18 19:01 UTC (permalink / raw)
To: Wilken Gottwalt; +Cc: linux-kernel, Jean Delvare, Jonathan Corbet, linux-hwmon
On Thu, Mar 18, 2021 at 03:17:14PM +0100, Wilken Gottwalt wrote:
> Adds support for reading the critical values of the temperature sensors
> and the rail sensors (voltage and current) once and caches them. Updates
> the naming of the constants following a more clear scheme. Also updates
> the documentation and fixes some typos. Updates is_visible and ops_read
> functions to be more readable.
>
> The new sensors output of a Corsair HX850i will look like this:
> corsairpsu-hid-3-1
> Adapter: HID adapter
> v_in: 230.00 V
> v_out +12v: 12.14 V (crit min = +8.41 V, crit max = +15.59 V)
> v_out +5v: 5.03 V (crit min = +3.50 V, crit max = +6.50 V)
> v_out +3.3v: 3.30 V (crit min = +2.31 V, crit max = +4.30 V)
> psu fan: 0 RPM
> vrm temp: +46.2°C (crit = +70.0°C)
> case temp: +39.8°C (crit = +70.0°C)
> power total: 152.00 W
> power +12v: 108.00 W
> power +5v: 41.00 W
> power +3.3v: 5.00 W
> curr +12v: 9.00 A (crit max = +85.00 A)
> curr +5v: 8.31 A (crit max = +40.00 A)
> curr +3.3v: 1.62 A (crit max = +40.00 A)
>
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
Applied.
Thanks,
Guenter
> ---
> Changed in v4:
> - simplified private data structure and collection of critical values and
> unsupported command check
>
> Changes in v3:
> - introduced a quirk check function to catch non-working commands
> - split is_visible function into subfunctions
> - moved the "is value valid" checks into the is_visibility subfunction
> - simplified hwmon_ops_read function
> - rearranged sysfs entries in the documentation like suggested
>
> Changes in v2:
> - simplified reading/caching of critical values and hwmon_ops_read function
> - removed unnecessary debug output and comments
> ---
> Documentation/hwmon/corsair-psu.rst | 13 +-
> drivers/hwmon/corsair-psu.c | 325 +++++++++++++++++++++++-----
> 2 files changed, 282 insertions(+), 56 deletions(-)
>
> diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst
> index 396b95c9a76a..e8378e7a1d8c 100644
> --- a/Documentation/hwmon/corsair-psu.rst
> +++ b/Documentation/hwmon/corsair-psu.rst
> @@ -47,19 +47,30 @@ Sysfs entries
> ======================= ========================================================
> curr1_input Total current usage
> curr2_input Current on the 12v psu rail
> +curr2_crit Current max critical value on the 12v psu rail
> curr3_input Current on the 5v psu rail
> +curr3_crit Current max critical value on the 5v psu rail
> curr4_input Current on the 3.3v psu rail
> +curr4_crit Current max critical value on the 3.3v psu rail
> fan1_input RPM of psu fan
> in0_input Voltage of the psu ac input
> in1_input Voltage of the 12v psu rail
> +in1_crit Voltage max critical value on the 12v psu rail
> +in1_lcrit Voltage min critical value on the 12v psu rail
> in2_input Voltage of the 5v psu rail
> -in3_input Voltage of the 3.3 psu rail
> +in2_crit Voltage max critical value on the 5v psu rail
> +in2_lcrit Voltage min critical value on the 5v psu rail
> +in3_input Voltage of the 3.3v psu rail
> +in3_crit Voltage max critical value on the 3.3v psu rail
> +in3_lcrit Voltage min critical value on the 3.3v psu rail
> power1_input Total power usage
> power2_input Power usage of the 12v psu rail
> power3_input Power usage of the 5v psu rail
> power4_input Power usage of the 3.3v psu rail
> temp1_input Temperature of the psu vrm component
> +temp1_crit Temperature max cirtical value of the psu vrm component
> temp2_input Temperature of the psu case
> +temp2_crit Temperature max critical value of psu case
> ======================= ========================================================
>
> Usage Notes
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index b0953eeeb2d3..3a5807e4a2ef 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -53,11 +53,17 @@
> #define CMD_TIMEOUT_MS 250
> #define SECONDS_PER_HOUR (60 * 60)
> #define SECONDS_PER_DAY (SECONDS_PER_HOUR * 24)
> +#define RAIL_COUNT 3 /* 3v3 + 5v + 12v */
> +#define TEMP_COUNT 2
>
> #define PSU_CMD_SELECT_RAIL 0x00 /* expects length 2 */
> -#define PSU_CMD_IN_VOLTS 0x88 /* the rest of the commands expect length 3 */
> +#define PSU_CMD_RAIL_VOLTS_HCRIT 0x40 /* the rest of the commands expect length 3 */
> +#define PSU_CMD_RAIL_VOLTS_LCRIT 0x44
> +#define PSU_CMD_RAIL_AMPS_HCRIT 0x46
> +#define PSU_CMD_TEMP_HCRIT 0x4F
> +#define PSU_CMD_IN_VOLTS 0x88
> #define PSU_CMD_IN_AMPS 0x89
> -#define PSU_CMD_RAIL_OUT_VOLTS 0x8B
> +#define PSU_CMD_RAIL_VOLTS 0x8B
> #define PSU_CMD_RAIL_AMPS 0x8C
> #define PSU_CMD_TEMP0 0x8D
> #define PSU_CMD_TEMP1 0x8E
> @@ -116,6 +122,15 @@ struct corsairpsu_data {
> u8 *cmd_buffer;
> char vendor[REPLY_SIZE];
> char product[REPLY_SIZE];
> + long temp_crit[TEMP_COUNT];
> + long in_crit[RAIL_COUNT];
> + long in_lcrit[RAIL_COUNT];
> + long curr_crit[RAIL_COUNT];
> + u8 temp_crit_support;
> + u8 in_crit_support;
> + u8 in_lcrit_support;
> + u8 curr_crit_support;
> + bool in_curr_cmd_support; /* not all commands are supported on every PSU */
> };
>
> /* some values are SMBus LINEAR11 data which need a conversion */
> @@ -193,7 +208,10 @@ static int corsairpsu_request(struct corsairpsu_data *priv, u8 cmd, u8 rail, voi
>
> mutex_lock(&priv->lock);
> switch (cmd) {
> - case PSU_CMD_RAIL_OUT_VOLTS:
> + case PSU_CMD_RAIL_VOLTS_HCRIT:
> + case PSU_CMD_RAIL_VOLTS_LCRIT:
> + case PSU_CMD_RAIL_AMPS_HCRIT:
> + case PSU_CMD_RAIL_VOLTS:
> case PSU_CMD_RAIL_AMPS:
> case PSU_CMD_RAIL_WATTS:
> ret = corsairpsu_usb_cmd(priv, 2, PSU_CMD_SELECT_RAIL, rail, NULL);
> @@ -229,9 +247,13 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
> */
> tmp = ((long)data[3] << 24) + (data[2] << 16) + (data[1] << 8) + data[0];
> switch (cmd) {
> + case PSU_CMD_RAIL_VOLTS_HCRIT:
> + case PSU_CMD_RAIL_VOLTS_LCRIT:
> + case PSU_CMD_RAIL_AMPS_HCRIT:
> + case PSU_CMD_TEMP_HCRIT:
> case PSU_CMD_IN_VOLTS:
> case PSU_CMD_IN_AMPS:
> - case PSU_CMD_RAIL_OUT_VOLTS:
> + case PSU_CMD_RAIL_VOLTS:
> case PSU_CMD_RAIL_AMPS:
> case PSU_CMD_TEMP0:
> case PSU_CMD_TEMP1:
> @@ -256,75 +278,265 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
> return ret;
> }
>
> -static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type,
> - u32 attr, int channel)
> +static void corsairpsu_get_criticals(struct corsairpsu_data *priv)
> {
> - if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label))
> - return 0444;
> - else if (type == hwmon_fan && (attr == hwmon_fan_input || attr == hwmon_fan_label))
> - return 0444;
> - else if (type == hwmon_power && (attr == hwmon_power_input || attr == hwmon_power_label))
> - return 0444;
> - else if (type == hwmon_in && (attr == hwmon_in_input || attr == hwmon_in_label))
> + long tmp;
> + int rail;
> +
> + for (rail = 0; rail < TEMP_COUNT; ++rail) {
> + if (!corsairpsu_get_value(priv, PSU_CMD_TEMP_HCRIT, rail, &tmp)) {
> + priv->temp_crit_support |= BIT(rail);
> + priv->temp_crit[rail] = tmp;
> + }
> + }
> +
> + for (rail = 0; rail < RAIL_COUNT; ++rail) {
> + if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_HCRIT, rail, &tmp)) {
> + priv->in_crit_support |= BIT(rail);
> + priv->in_crit[rail] = tmp;
> + }
> +
> + if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_LCRIT, rail, &tmp)) {
> + priv->in_lcrit_support |= BIT(rail);
> + priv->in_lcrit[rail] = tmp;
> + }
> +
> + if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS_HCRIT, rail, &tmp)) {
> + priv->curr_crit_support |= BIT(rail);
> + priv->curr_crit[rail] = tmp;
> + }
> + }
> +}
> +
> +static void corsairpsu_check_cmd_support(struct corsairpsu_data *priv)
> +{
> + long tmp;
> +
> + priv->in_curr_cmd_support = !corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, &tmp);
> +}
> +
> +static umode_t corsairpsu_hwmon_temp_is_visible(const struct corsairpsu_data *priv, u32 attr,
> + int channel)
> +{
> + umode_t res = 0444;
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + case hwmon_temp_label:
> + case hwmon_temp_crit:
> + if (channel > 0 && !(priv->temp_crit_support & BIT(channel - 1)))
> + res = 0;
> + break;
> + default:
> + break;
> + }
> +
> + return res;
> +}
> +
> +static umode_t corsairpsu_hwmon_fan_is_visible(const struct corsairpsu_data *priv, u32 attr,
> + int channel)
> +{
> + switch (attr) {
> + case hwmon_fan_input:
> + case hwmon_fan_label:
> return 0444;
> - else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label))
> + default:
> + return 0;
> + }
> +}
> +
> +static umode_t corsairpsu_hwmon_power_is_visible(const struct corsairpsu_data *priv, u32 attr,
> + int channel)
> +{
> + switch (attr) {
> + case hwmon_power_input:
> + case hwmon_power_label:
> return 0444;
> + default:
> + return 0;
> + };
> +}
>
> - return 0;
> +static umode_t corsairpsu_hwmon_in_is_visible(const struct corsairpsu_data *priv, u32 attr,
> + int channel)
> +{
> + umode_t res = 0444;
> +
> + switch (attr) {
> + case hwmon_in_input:
> + case hwmon_in_label:
> + case hwmon_in_crit:
> + if (channel > 0 && !(priv->in_crit_support & BIT(channel - 1)))
> + res = 0;
> + break;
> + case hwmon_in_lcrit:
> + if (channel > 0 && !(priv->in_lcrit_support & BIT(channel - 1)))
> + res = 0;
> + break;
> + default:
> + break;
> + };
> +
> + return res;
> }
>
> -static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> - int channel, long *val)
> +static umode_t corsairpsu_hwmon_curr_is_visible(const struct corsairpsu_data *priv, u32 attr,
> + int channel)
> {
> - struct corsairpsu_data *priv = dev_get_drvdata(dev);
> - int ret;
> + umode_t res = 0444;
>
> - if (type == hwmon_temp && attr == hwmon_temp_input && channel < 2) {
> - ret = corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : PSU_CMD_TEMP0, channel,
> - val);
> - } else if (type == hwmon_fan && attr == hwmon_fan_input) {
> - ret = corsairpsu_get_value(priv, PSU_CMD_FAN, 0, val);
> - } else if (type == hwmon_power && attr == hwmon_power_input) {
> + switch (attr) {
> + case hwmon_curr_input:
> + if (channel == 0 && !priv->in_curr_cmd_support)
> + res = 0;
> + break;
> + case hwmon_curr_label:
> + case hwmon_curr_crit:
> + if (channel > 0 && !(priv->curr_crit_support & BIT(channel - 1)))
> + res = 0;
> + break;
> + default:
> + break;
> + }
> +
> + return res;
> +}
> +
> +static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + const struct corsairpsu_data *priv = data;
> +
> + switch (type) {
> + case hwmon_temp:
> + return corsairpsu_hwmon_temp_is_visible(priv, attr, channel);
> + case hwmon_fan:
> + return corsairpsu_hwmon_fan_is_visible(priv, attr, channel);
> + case hwmon_power:
> + return corsairpsu_hwmon_power_is_visible(priv, attr, channel);
> + case hwmon_in:
> + return corsairpsu_hwmon_in_is_visible(priv, attr, channel);
> + case hwmon_curr:
> + return corsairpsu_hwmon_curr_is_visible(priv, attr, channel);
> + default:
> + return 0;
> + }
> +}
> +
> +static int corsairpsu_hwmon_temp_read(struct corsairpsu_data *priv, u32 attr, int channel,
> + long *val)
> +{
> + int err = -EOPNOTSUPP;
> +
> + switch (attr) {
> + case hwmon_temp_input:
> + return corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : PSU_CMD_TEMP0,
> + channel, val);
> + case hwmon_temp_crit:
> + *val = priv->temp_crit[channel];
> + err = 0;
> + break;
> + default:
> + break;
> + }
> +
> + return err;
> +}
> +
> +static int corsairpsu_hwmon_power_read(struct corsairpsu_data *priv, u32 attr, int channel,
> + long *val)
> +{
> + if (attr == hwmon_power_input) {
> switch (channel) {
> case 0:
> - ret = corsairpsu_get_value(priv, PSU_CMD_TOTAL_WATTS, 0, val);
> - break;
> + return corsairpsu_get_value(priv, PSU_CMD_TOTAL_WATTS, 0, val);
> case 1 ... 3:
> - ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1, val);
> - break;
> + return corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1, val);
> default:
> - return -EOPNOTSUPP;
> + break;
> }
> - } else if (type == hwmon_in && attr == hwmon_in_input) {
> + }
> +
> + return -EOPNOTSUPP;
> +}
> +
> +static int corsairpsu_hwmon_in_read(struct corsairpsu_data *priv, u32 attr, int channel, long *val)
> +{
> + int err = -EOPNOTSUPP;
> +
> + switch (attr) {
> + case hwmon_in_input:
> switch (channel) {
> case 0:
> - ret = corsairpsu_get_value(priv, PSU_CMD_IN_VOLTS, 0, val);
> - break;
> + return corsairpsu_get_value(priv, PSU_CMD_IN_VOLTS, 0, val);
> case 1 ... 3:
> - ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_OUT_VOLTS, channel - 1, val);
> - break;
> + return corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS, channel - 1, val);
> default:
> - return -EOPNOTSUPP;
> + break;
> }
> - } else if (type == hwmon_curr && attr == hwmon_curr_input) {
> + break;
> + case hwmon_in_crit:
> + *val = priv->in_crit[channel - 1];
> + err = 0;
> + break;
> + case hwmon_in_lcrit:
> + *val = priv->in_lcrit[channel - 1];
> + err = 0;
> + break;
> + }
> +
> + return err;
> +}
> +
> +static int corsairpsu_hwmon_curr_read(struct corsairpsu_data *priv, u32 attr, int channel,
> + long *val)
> +{
> + int err = -EOPNOTSUPP;
> +
> + switch (attr) {
> + case hwmon_curr_input:
> switch (channel) {
> case 0:
> - ret = corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, val);
> - break;
> + return corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, val);
> case 1 ... 3:
> - ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val);
> - break;
> + return corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val);
> default:
> - return -EOPNOTSUPP;
> + break;
> }
> - } else {
> - return -EOPNOTSUPP;
> + break;
> + case hwmon_curr_crit:
> + *val = priv->curr_crit[channel - 1];
> + err = 0;
> + break;
> + default:
> + break;
> }
>
> - if (ret < 0)
> - return ret;
> + return err;
> +}
>
> - return 0;
> +static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct corsairpsu_data *priv = dev_get_drvdata(dev);
> +
> + switch (type) {
> + case hwmon_temp:
> + return corsairpsu_hwmon_temp_read(priv, attr, channel, val);
> + case hwmon_fan:
> + if (attr == hwmon_fan_input)
> + return corsairpsu_get_value(priv, PSU_CMD_FAN, 0, val);
> + return -EOPNOTSUPP;
> + case hwmon_power:
> + return corsairpsu_hwmon_power_read(priv, attr, channel, val);
> + case hwmon_in:
> + return corsairpsu_hwmon_in_read(priv, attr, channel, val);
> + case hwmon_curr:
> + return corsairpsu_hwmon_curr_read(priv, attr, channel, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> }
>
> static int corsairpsu_hwmon_ops_read_string(struct device *dev, enum hwmon_sensor_types type,
> @@ -360,8 +572,8 @@ static const struct hwmon_channel_info *corsairpsu_info[] = {
> HWMON_CHANNEL_INFO(chip,
> HWMON_C_REGISTER_TZ),
> HWMON_CHANNEL_INFO(temp,
> - HWMON_T_INPUT | HWMON_T_LABEL,
> - HWMON_T_INPUT | HWMON_T_LABEL),
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT),
> HWMON_CHANNEL_INFO(fan,
> HWMON_F_INPUT | HWMON_F_LABEL),
> HWMON_CHANNEL_INFO(power,
> @@ -371,14 +583,14 @@ static const struct hwmon_channel_info *corsairpsu_info[] = {
> HWMON_P_INPUT | HWMON_P_LABEL),
> HWMON_CHANNEL_INFO(in,
> HWMON_I_INPUT | HWMON_I_LABEL,
> - HWMON_I_INPUT | HWMON_I_LABEL,
> - HWMON_I_INPUT | HWMON_I_LABEL,
> - HWMON_I_INPUT | HWMON_I_LABEL),
> + HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT,
> + HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT,
> + HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT),
> HWMON_CHANNEL_INFO(curr,
> HWMON_C_INPUT | HWMON_C_LABEL,
> - HWMON_C_INPUT | HWMON_C_LABEL,
> - HWMON_C_INPUT | HWMON_C_LABEL,
> - HWMON_C_INPUT | HWMON_C_LABEL),
> + HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT,
> + HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT,
> + HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT),
> NULL
> };
>
> @@ -513,6 +725,9 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id
> goto fail_and_stop;
> }
>
> + corsairpsu_get_criticals(priv);
> + corsairpsu_check_cmd_support(priv);
> +
> priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv,
> &corsairpsu_chip_info, 0);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] hwmon: corsair-psu: add support for critical values
2021-03-18 19:01 ` Guenter Roeck
@ 2021-03-19 8:58 ` Wilken Gottwalt
2021-03-19 17:46 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Wilken Gottwalt @ 2021-03-19 8:58 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-kernel, Jean Delvare, Jonathan Corbet, linux-hwmon
On Thu, 18 Mar 2021 12:01:50 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Mar 18, 2021 at 03:17:14PM +0100, Wilken Gottwalt wrote:
> > Adds support for reading the critical values of the temperature sensors
> > and the rail sensors (voltage and current) once and caches them. Updates
> > the naming of the constants following a more clear scheme. Also updates
> > the documentation and fixes some typos. Updates is_visible and ops_read
> > functions to be more readable.
> >
> > The new sensors output of a Corsair HX850i will look like this:
> > corsairpsu-hid-3-1
> > Adapter: HID adapter
> > v_in: 230.00 V
> > v_out +12v: 12.14 V (crit min = +8.41 V, crit max = +15.59 V)
> > v_out +5v: 5.03 V (crit min = +3.50 V, crit max = +6.50 V)
> > v_out +3.3v: 3.30 V (crit min = +2.31 V, crit max = +4.30 V)
> > psu fan: 0 RPM
> > vrm temp: +46.2°C (crit = +70.0°C)
> > case temp: +39.8°C (crit = +70.0°C)
> > power total: 152.00 W
> > power +12v: 108.00 W
> > power +5v: 41.00 W
> > power +3.3v: 5.00 W
> > curr +12v: 9.00 A (crit max = +85.00 A)
> > curr +5v: 8.31 A (crit max = +40.00 A)
> > curr +3.3v: 1.62 A (crit max = +40.00 A)
> >
> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
>
> Applied.
Thank very much. Hmm, I actually could calculate the in_curr value from total
power and the ac input as a replacement if the value can not be read. What do
you think?
> Thanks,
> Guenter
>
> > ---
> > Changed in v4:
> > - simplified private data structure and collection of critical values and
> > unsupported command check
> >
> > Changes in v3:
> > - introduced a quirk check function to catch non-working commands
> > - split is_visible function into subfunctions
> > - moved the "is value valid" checks into the is_visibility subfunction
> > - simplified hwmon_ops_read function
> > - rearranged sysfs entries in the documentation like suggested
> >
> > Changes in v2:
> > - simplified reading/caching of critical values and hwmon_ops_read function
> > - removed unnecessary debug output and comments
> > ---
> > Documentation/hwmon/corsair-psu.rst | 13 +-
> > drivers/hwmon/corsair-psu.c | 325 +++++++++++++++++++++++-----
> > 2 files changed, 282 insertions(+), 56 deletions(-)
> >
> > diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst
> > index 396b95c9a76a..e8378e7a1d8c 100644
> > --- a/Documentation/hwmon/corsair-psu.rst
> > +++ b/Documentation/hwmon/corsair-psu.rst
> > @@ -47,19 +47,30 @@ Sysfs entries
> > ======================= ========================================================
> > curr1_input Total current usage
> > curr2_input Current on the 12v psu rail
> > +curr2_crit Current max critical value on the 12v psu rail
> > curr3_input Current on the 5v psu rail
> > +curr3_crit Current max critical value on the 5v psu rail
> > curr4_input Current on the 3.3v psu rail
> > +curr4_crit Current max critical value on the 3.3v psu rail
> > fan1_input RPM of psu fan
> > in0_input Voltage of the psu ac input
> > in1_input Voltage of the 12v psu rail
> > +in1_crit Voltage max critical value on the 12v psu rail
> > +in1_lcrit Voltage min critical value on the 12v psu rail
> > in2_input Voltage of the 5v psu rail
> > -in3_input Voltage of the 3.3 psu rail
> > +in2_crit Voltage max critical value on the 5v psu rail
> > +in2_lcrit Voltage min critical value on the 5v psu rail
> > +in3_input Voltage of the 3.3v psu rail
> > +in3_crit Voltage max critical value on the 3.3v psu rail
> > +in3_lcrit Voltage min critical value on the 3.3v psu rail
> > power1_input Total power usage
> > power2_input Power usage of the 12v psu rail
> > power3_input Power usage of the 5v psu rail
> > power4_input Power usage of the 3.3v psu rail
> > temp1_input Temperature of the psu vrm component
> > +temp1_crit Temperature max cirtical value of the psu vrm component
> > temp2_input Temperature of the psu case
> > +temp2_crit Temperature max critical value of psu case
> > ======================= ========================================================
> >
> > Usage Notes
> > diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> > index b0953eeeb2d3..3a5807e4a2ef 100644
> > --- a/drivers/hwmon/corsair-psu.c
> > +++ b/drivers/hwmon/corsair-psu.c
> > @@ -53,11 +53,17 @@
> > #define CMD_TIMEOUT_MS 250
> > #define SECONDS_PER_HOUR (60 * 60)
> > #define SECONDS_PER_DAY (SECONDS_PER_HOUR * 24)
> > +#define RAIL_COUNT 3 /* 3v3 + 5v + 12v */
> > +#define TEMP_COUNT 2
> >
> > #define PSU_CMD_SELECT_RAIL 0x00 /* expects length 2 */
> > -#define PSU_CMD_IN_VOLTS 0x88 /* the rest of the commands expect length 3 */
> > +#define PSU_CMD_RAIL_VOLTS_HCRIT 0x40 /* the rest of the commands expect length 3 */
> > +#define PSU_CMD_RAIL_VOLTS_LCRIT 0x44
> > +#define PSU_CMD_RAIL_AMPS_HCRIT 0x46
> > +#define PSU_CMD_TEMP_HCRIT 0x4F
> > +#define PSU_CMD_IN_VOLTS 0x88
> > #define PSU_CMD_IN_AMPS 0x89
> > -#define PSU_CMD_RAIL_OUT_VOLTS 0x8B
> > +#define PSU_CMD_RAIL_VOLTS 0x8B
> > #define PSU_CMD_RAIL_AMPS 0x8C
> > #define PSU_CMD_TEMP0 0x8D
> > #define PSU_CMD_TEMP1 0x8E
> > @@ -116,6 +122,15 @@ struct corsairpsu_data {
> > u8 *cmd_buffer;
> > char vendor[REPLY_SIZE];
> > char product[REPLY_SIZE];
> > + long temp_crit[TEMP_COUNT];
> > + long in_crit[RAIL_COUNT];
> > + long in_lcrit[RAIL_COUNT];
> > + long curr_crit[RAIL_COUNT];
> > + u8 temp_crit_support;
> > + u8 in_crit_support;
> > + u8 in_lcrit_support;
> > + u8 curr_crit_support;
> > + bool in_curr_cmd_support; /* not all commands are supported on every PSU */
> > };
> >
> > /* some values are SMBus LINEAR11 data which need a conversion */
> > @@ -193,7 +208,10 @@ static int corsairpsu_request(struct corsairpsu_data *priv, u8 cmd, u8
> > rail, voi
> > mutex_lock(&priv->lock);
> > switch (cmd) {
> > - case PSU_CMD_RAIL_OUT_VOLTS:
> > + case PSU_CMD_RAIL_VOLTS_HCRIT:
> > + case PSU_CMD_RAIL_VOLTS_LCRIT:
> > + case PSU_CMD_RAIL_AMPS_HCRIT:
> > + case PSU_CMD_RAIL_VOLTS:
> > case PSU_CMD_RAIL_AMPS:
> > case PSU_CMD_RAIL_WATTS:
> > ret = corsairpsu_usb_cmd(priv, 2, PSU_CMD_SELECT_RAIL, rail, NULL);
> > @@ -229,9 +247,13 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8
> > rail, l */
> > tmp = ((long)data[3] << 24) + (data[2] << 16) + (data[1] << 8) + data[0];
> > switch (cmd) {
> > + case PSU_CMD_RAIL_VOLTS_HCRIT:
> > + case PSU_CMD_RAIL_VOLTS_LCRIT:
> > + case PSU_CMD_RAIL_AMPS_HCRIT:
> > + case PSU_CMD_TEMP_HCRIT:
> > case PSU_CMD_IN_VOLTS:
> > case PSU_CMD_IN_AMPS:
> > - case PSU_CMD_RAIL_OUT_VOLTS:
> > + case PSU_CMD_RAIL_VOLTS:
> > case PSU_CMD_RAIL_AMPS:
> > case PSU_CMD_TEMP0:
> > case PSU_CMD_TEMP1:
> > @@ -256,75 +278,265 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8
> > rail, l return ret;
> > }
> >
> > -static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type,
> > - u32 attr, int channel)
> > +static void corsairpsu_get_criticals(struct corsairpsu_data *priv)
> > {
> > - if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label))
> > - return 0444;
> > - else if (type == hwmon_fan && (attr == hwmon_fan_input || attr == hwmon_fan_label))
> > - return 0444;
> > - else if (type == hwmon_power && (attr == hwmon_power_input || attr ==
> > hwmon_power_label))
> > - return 0444;
> > - else if (type == hwmon_in && (attr == hwmon_in_input || attr == hwmon_in_label))
> > + long tmp;
> > + int rail;
> > +
> > + for (rail = 0; rail < TEMP_COUNT; ++rail) {
> > + if (!corsairpsu_get_value(priv, PSU_CMD_TEMP_HCRIT, rail, &tmp)) {
> > + priv->temp_crit_support |= BIT(rail);
> > + priv->temp_crit[rail] = tmp;
> > + }
> > + }
> > +
> > + for (rail = 0; rail < RAIL_COUNT; ++rail) {
> > + if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_HCRIT, rail, &tmp)) {
> > + priv->in_crit_support |= BIT(rail);
> > + priv->in_crit[rail] = tmp;
> > + }
> > +
> > + if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_LCRIT, rail, &tmp)) {
> > + priv->in_lcrit_support |= BIT(rail);
> > + priv->in_lcrit[rail] = tmp;
> > + }
> > +
> > + if (!corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS_HCRIT, rail, &tmp)) {
> > + priv->curr_crit_support |= BIT(rail);
> > + priv->curr_crit[rail] = tmp;
> > + }
> > + }
> > +}
> > +
> > +static void corsairpsu_check_cmd_support(struct corsairpsu_data *priv)
> > +{
> > + long tmp;
> > +
> > + priv->in_curr_cmd_support = !corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, &tmp);
> > +}
> > +
> > +static umode_t corsairpsu_hwmon_temp_is_visible(const struct corsairpsu_data *priv, u32 attr,
> > + int channel)
> > +{
> > + umode_t res = 0444;
> > +
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + case hwmon_temp_label:
> > + case hwmon_temp_crit:
> > + if (channel > 0 && !(priv->temp_crit_support & BIT(channel - 1)))
> > + res = 0;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return res;
> > +}
> > +
> > +static umode_t corsairpsu_hwmon_fan_is_visible(const struct corsairpsu_data *priv, u32 attr,
> > + int channel)
> > +{
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + case hwmon_fan_label:
> > return 0444;
> > - else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label))
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static umode_t corsairpsu_hwmon_power_is_visible(const struct corsairpsu_data *priv, u32 attr,
> > + int channel)
> > +{
> > + switch (attr) {
> > + case hwmon_power_input:
> > + case hwmon_power_label:
> > return 0444;
> > + default:
> > + return 0;
> > + };
> > +}
> >
> > - return 0;
> > +static umode_t corsairpsu_hwmon_in_is_visible(const struct corsairpsu_data *priv, u32 attr,
> > + int channel)
> > +{
> > + umode_t res = 0444;
> > +
> > + switch (attr) {
> > + case hwmon_in_input:
> > + case hwmon_in_label:
> > + case hwmon_in_crit:
> > + if (channel > 0 && !(priv->in_crit_support & BIT(channel - 1)))
> > + res = 0;
> > + break;
> > + case hwmon_in_lcrit:
> > + if (channel > 0 && !(priv->in_lcrit_support & BIT(channel - 1)))
> > + res = 0;
> > + break;
> > + default:
> > + break;
> > + };
> > +
> > + return res;
> > }
> >
> > -static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32
> > attr,
> > - int channel, long *val)
> > +static umode_t corsairpsu_hwmon_curr_is_visible(const struct corsairpsu_data *priv, u32 attr,
> > + int channel)
> > {
> > - struct corsairpsu_data *priv = dev_get_drvdata(dev);
> > - int ret;
> > + umode_t res = 0444;
> >
> > - if (type == hwmon_temp && attr == hwmon_temp_input && channel < 2) {
> > - ret = corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : PSU_CMD_TEMP0,
> > channel,
> > - val);
> > - } else if (type == hwmon_fan && attr == hwmon_fan_input) {
> > - ret = corsairpsu_get_value(priv, PSU_CMD_FAN, 0, val);
> > - } else if (type == hwmon_power && attr == hwmon_power_input) {
> > + switch (attr) {
> > + case hwmon_curr_input:
> > + if (channel == 0 && !priv->in_curr_cmd_support)
> > + res = 0;
> > + break;
> > + case hwmon_curr_label:
> > + case hwmon_curr_crit:
> > + if (channel > 0 && !(priv->curr_crit_support & BIT(channel - 1)))
> > + res = 0;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return res;
> > +}
> > +
> > +static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + const struct corsairpsu_data *priv = data;
> > +
> > + switch (type) {
> > + case hwmon_temp:
> > + return corsairpsu_hwmon_temp_is_visible(priv, attr, channel);
> > + case hwmon_fan:
> > + return corsairpsu_hwmon_fan_is_visible(priv, attr, channel);
> > + case hwmon_power:
> > + return corsairpsu_hwmon_power_is_visible(priv, attr, channel);
> > + case hwmon_in:
> > + return corsairpsu_hwmon_in_is_visible(priv, attr, channel);
> > + case hwmon_curr:
> > + return corsairpsu_hwmon_curr_is_visible(priv, attr, channel);
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static int corsairpsu_hwmon_temp_read(struct corsairpsu_data *priv, u32 attr, int channel,
> > + long *val)
> > +{
> > + int err = -EOPNOTSUPP;
> > +
> > + switch (attr) {
> > + case hwmon_temp_input:
> > + return corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : PSU_CMD_TEMP0,
> > + channel, val);
> > + case hwmon_temp_crit:
> > + *val = priv->temp_crit[channel];
> > + err = 0;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static int corsairpsu_hwmon_power_read(struct corsairpsu_data *priv, u32 attr, int channel,
> > + long *val)
> > +{
> > + if (attr == hwmon_power_input) {
> > switch (channel) {
> > case 0:
> > - ret = corsairpsu_get_value(priv, PSU_CMD_TOTAL_WATTS, 0, val);
> > - break;
> > + return corsairpsu_get_value(priv, PSU_CMD_TOTAL_WATTS, 0, val);
> > case 1 ... 3:
> > - ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1, val);
> > - break;
> > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1,
> > val); default:
> > - return -EOPNOTSUPP;
> > + break;
> > }
> > - } else if (type == hwmon_in && attr == hwmon_in_input) {
> > + }
> > +
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static int corsairpsu_hwmon_in_read(struct corsairpsu_data *priv, u32 attr, int channel, long
> > *val) +{
> > + int err = -EOPNOTSUPP;
> > +
> > + switch (attr) {
> > + case hwmon_in_input:
> > switch (channel) {
> > case 0:
> > - ret = corsairpsu_get_value(priv, PSU_CMD_IN_VOLTS, 0, val);
> > - break;
> > + return corsairpsu_get_value(priv, PSU_CMD_IN_VOLTS, 0, val);
> > case 1 ... 3:
> > - ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_OUT_VOLTS, channel - 1,
> > val);
> > - break;
> > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS, channel - 1,
> > val); default:
> > - return -EOPNOTSUPP;
> > + break;
> > }
> > - } else if (type == hwmon_curr && attr == hwmon_curr_input) {
> > + break;
> > + case hwmon_in_crit:
> > + *val = priv->in_crit[channel - 1];
> > + err = 0;
> > + break;
> > + case hwmon_in_lcrit:
> > + *val = priv->in_lcrit[channel - 1];
> > + err = 0;
> > + break;
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static int corsairpsu_hwmon_curr_read(struct corsairpsu_data *priv, u32 attr, int channel,
> > + long *val)
> > +{
> > + int err = -EOPNOTSUPP;
> > +
> > + switch (attr) {
> > + case hwmon_curr_input:
> > switch (channel) {
> > case 0:
> > - ret = corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, val);
> > - break;
> > + return corsairpsu_get_value(priv, PSU_CMD_IN_AMPS, 0, val);
> > case 1 ... 3:
> > - ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val);
> > - break;
> > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val);
> > default:
> > - return -EOPNOTSUPP;
> > + break;
> > }
> > - } else {
> > - return -EOPNOTSUPP;
> > + break;
> > + case hwmon_curr_crit:
> > + *val = priv->curr_crit[channel - 1];
> > + err = 0;
> > + break;
> > + default:
> > + break;
> > }
> >
> > - if (ret < 0)
> > - return ret;
> > + return err;
> > +}
> >
> > - return 0;
> > +static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32
> > attr,
> > + int channel, long *val)
> > +{
> > + struct corsairpsu_data *priv = dev_get_drvdata(dev);
> > +
> > + switch (type) {
> > + case hwmon_temp:
> > + return corsairpsu_hwmon_temp_read(priv, attr, channel, val);
> > + case hwmon_fan:
> > + if (attr == hwmon_fan_input)
> > + return corsairpsu_get_value(priv, PSU_CMD_FAN, 0, val);
> > + return -EOPNOTSUPP;
> > + case hwmon_power:
> > + return corsairpsu_hwmon_power_read(priv, attr, channel, val);
> > + case hwmon_in:
> > + return corsairpsu_hwmon_in_read(priv, attr, channel, val);
> > + case hwmon_curr:
> > + return corsairpsu_hwmon_curr_read(priv, attr, channel, val);
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > }
> >
> > static int corsairpsu_hwmon_ops_read_string(struct device *dev, enum hwmon_sensor_types type,
> > @@ -360,8 +572,8 @@ static const struct hwmon_channel_info *corsairpsu_info[] = {
> > HWMON_CHANNEL_INFO(chip,
> > HWMON_C_REGISTER_TZ),
> > HWMON_CHANNEL_INFO(temp,
> > - HWMON_T_INPUT | HWMON_T_LABEL,
> > - HWMON_T_INPUT | HWMON_T_LABEL),
> > + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT,
> > + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT),
> > HWMON_CHANNEL_INFO(fan,
> > HWMON_F_INPUT | HWMON_F_LABEL),
> > HWMON_CHANNEL_INFO(power,
> > @@ -371,14 +583,14 @@ static const struct hwmon_channel_info *corsairpsu_info[] = {
> > HWMON_P_INPUT | HWMON_P_LABEL),
> > HWMON_CHANNEL_INFO(in,
> > HWMON_I_INPUT | HWMON_I_LABEL,
> > - HWMON_I_INPUT | HWMON_I_LABEL,
> > - HWMON_I_INPUT | HWMON_I_LABEL,
> > - HWMON_I_INPUT | HWMON_I_LABEL),
> > + HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT,
> > + HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT,
> > + HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_LCRIT | HWMON_I_CRIT),
> > HWMON_CHANNEL_INFO(curr,
> > HWMON_C_INPUT | HWMON_C_LABEL,
> > - HWMON_C_INPUT | HWMON_C_LABEL,
> > - HWMON_C_INPUT | HWMON_C_LABEL,
> > - HWMON_C_INPUT | HWMON_C_LABEL),
> > + HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT,
> > + HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT,
> > + HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_CRIT),
> > NULL
> > };
> >
> > @@ -513,6 +725,9 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> > hid_device_id goto fail_and_stop;
> > }
> >
> > + corsairpsu_get_criticals(priv);
> > + corsairpsu_check_cmd_support(priv);
> > +
> > priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv,
> > &corsairpsu_chip_info, 0);
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] hwmon: corsair-psu: add support for critical values
2021-03-19 8:58 ` Wilken Gottwalt
@ 2021-03-19 17:46 ` Guenter Roeck
0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2021-03-19 17:46 UTC (permalink / raw)
To: Wilken Gottwalt; +Cc: linux-kernel, Jean Delvare, Jonathan Corbet, linux-hwmon
On 3/19/21 1:58 AM, Wilken Gottwalt wrote:
> On Thu, 18 Mar 2021 12:01:50 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> On Thu, Mar 18, 2021 at 03:17:14PM +0100, Wilken Gottwalt wrote:
>>> Adds support for reading the critical values of the temperature sensors
>>> and the rail sensors (voltage and current) once and caches them. Updates
>>> the naming of the constants following a more clear scheme. Also updates
>>> the documentation and fixes some typos. Updates is_visible and ops_read
>>> functions to be more readable.
>>>
>>> The new sensors output of a Corsair HX850i will look like this:
>>> corsairpsu-hid-3-1
>>> Adapter: HID adapter
>>> v_in: 230.00 V
>>> v_out +12v: 12.14 V (crit min = +8.41 V, crit max = +15.59 V)
>>> v_out +5v: 5.03 V (crit min = +3.50 V, crit max = +6.50 V)
>>> v_out +3.3v: 3.30 V (crit min = +2.31 V, crit max = +4.30 V)
>>> psu fan: 0 RPM
>>> vrm temp: +46.2°C (crit = +70.0°C)
>>> case temp: +39.8°C (crit = +70.0°C)
>>> power total: 152.00 W
>>> power +12v: 108.00 W
>>> power +5v: 41.00 W
>>> power +3.3v: 5.00 W
>>> curr +12v: 9.00 A (crit max = +85.00 A)
>>> curr +5v: 8.31 A (crit max = +40.00 A)
>>> curr +3.3v: 1.62 A (crit max = +40.00 A)
>>>
>>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
>>
>> Applied.
>
> Thank very much. Hmm, I actually could calculate the in_curr value from total
> power and the ac input as a replacement if the value can not be read. What do
> you think?
>
No, we better leave that up to userspace. While one might argue that it makes sense
here, unfortunately others will use it as argument to calculate other values not
provided by chips in the kernel (eg power if only voltage and current are supported).
I'd rather not create a precedence.
Guenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-19 17:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-18 14:17 [PATCH v4] hwmon: corsair-psu: add support for critical values Wilken Gottwalt
2021-03-18 19:01 ` Guenter Roeck
2021-03-19 8:58 ` Wilken Gottwalt
2021-03-19 17:46 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).