| Message ID | YE93HebFD0acwZIX@monster.powergraphx.local (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] hwmon: corsair-psu: add support for critical values | expand |
On 3/15/21 8:02 AM, 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 a typo. > > 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 in: N/A What does that mean ? If it isn't supported by the power supply, should we drop that entirely ? Maybe drop it via the is_visible function if it is available for some variants, but always displaying N/A doesn't add value. This is a bit odd, though, since I would assume it translates to the PSU_CMD_IN_AMPS command. Any chance to track down what is happening here ? > 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) > > This patch changes: > - hwmon corsair-psu documentation > - hwmon corsair-psu driver > That is obvious; no reason to state in the commit log. > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net> > --- > 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 | 223 +++++++++++++++++++++------- > 2 files changed, 185 insertions(+), 51 deletions(-) > > diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst > index 396b95c9a76a..b77e53810a13 100644 > --- a/Documentation/hwmon/corsair-psu.rst > +++ b/Documentation/hwmon/corsair-psu.rst > @@ -45,19 +45,30 @@ Sysfs entries > ------------- > > ======================= ======================================================== > +curr2_crit Current max critical value on the 12v psu rail > +curr3_crit Current max critical value on the 5v psu rail > +curr4_crit Current max critical value on the 3.3v psu rail > curr1_input Total current usage > curr2_input Current on the 12v psu rail > curr3_input Current on the 5v psu rail > curr4_input Current on the 3.3v psu rail I think it would be better to align those by index. curr1_input curr2_input curr2_crit ... Personally I always list _input first, but that is a matter of personal preference. > fan1_input RPM of psu fan > +in1_crit Voltage max critical value on the 12v psu rail > +in2_crit Voltage max critical value on the 5v psu rail > +in3_crit Voltage max critical value on the 3.3v psu rail > in0_input Voltage of the psu ac input > in1_input Voltage of the 12v psu rail > in2_input Voltage of the 5v psu rail > -in3_input Voltage of the 3.3 psu rail > +in3_input Voltage of the 3.3v psu rail > +in1_lcrit Voltage min critical value on the 12v psu rail > +in2_lcrit Voltage min critical value on the 5v 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_crit Temperature max cirtical value of the psu vrm component > +temp2_crit Temperature max critical value of psu case > temp1_input Temperature of the psu vrm component > temp2_input Temperature of the psu case > ======================= ======================================================== > diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c > index b0953eeeb2d3..a176ac6a2540 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,14 @@ 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; > }; > > /* some values are SMBus LINEAR11 data which need a conversion */ > @@ -193,7 +207,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 +246,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 +277,175 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l > return ret; > } > > +static void corsairpsu_get_criticals(struct corsairpsu_data *priv) > +{ > + long tmp; > + int ret; > + u8 rail; Side note: Using anything but sizeof(int) for index variables often results in more complex code because the compiler needs to mask index operations. This doesn't typically apply to x86 because that architecure has byte operations, but to pretty much every other architecture. > + > + priv->temp_crit_support = 0; > + priv->in_lcrit_support = 0; > + priv->in_crit_support = 0; > + priv->curr_crit_support = 0; > + > + for (rail = 0; rail < TEMP_COUNT; ++rail) { > + ret = corsairpsu_get_value(priv, PSU_CMD_TEMP_HCRIT, rail, &tmp); > + if (ret == 0) { Nit: the ret variable isn't really needed here. if (!corsairpsu_get_value(priv, PSU_CMD_TEMP_HCRIT, rail, &tmp)) { works just as well. Personal preference, though. > + priv->temp_crit_support |= BIT(rail); > + priv->temp_crit[rail] = tmp; > + } > + } > + > + for (rail = 0; rail < RAIL_COUNT; ++rail) { > + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_HCRIT, rail, &tmp); > + if (ret == 0) { > + priv->in_crit_support |= BIT(rail); > + priv->in_crit[rail] = tmp; > + } > + > + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_LCRIT, rail, &tmp); > + if (ret == 0) { > + priv->in_lcrit_support |= BIT(rail); > + priv->in_lcrit[rail] = tmp; > + } > + > + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS_HCRIT, rail, &tmp); > + if (ret == 0) { > + priv->curr_crit_support |= BIT(rail); > + priv->curr_crit[rail] = tmp; > + } > + } > +} > + > static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type, > u32 attr, int channel) > { > - if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label)) > + if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label || > + attr == hwmon_temp_crit)) > 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)) > + else if (type == hwmon_in && (attr == hwmon_in_input || attr == hwmon_in_label || > + attr == hwmon_in_lcrit || attr == hwmon_in_crit)) > return 0444; > - else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label)) > + else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label || > + attr == hwmon_curr_crit)) > return 0444; > > return 0; > } > > -static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, > - int channel, long *val) > +static int corsairpsu_hwmon_temp(struct corsairpsu_data *priv, u32 attr, int channel, long *val) Please make those functions _<type>_read, not just _<type>. It makes the code easier to understand, and we won't have to change it if write functions are ever added. > { > - struct corsairpsu_data *priv = dev_get_drvdata(dev); > - int ret; > + int err = -EOPNOTSUPP; > + > + if (channel < 2) { > + switch (attr) { > + case hwmon_temp_input: > + return corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : PSU_CMD_TEMP0, > + channel, val); > + case hwmon_temp_crit: > + if (priv->temp_crit_support & BIT(channel)) { > + *val = priv->temp_crit[channel]; > + err = 0; > + } > + } Dropping default cases from switch statements is never a good idea. It hides missing break statements, like here, and it may result in compiler or static analyzer warnings that not all situations are covered. Please don't do that (neither skipping break; statements not dropping default:). Yes, it technically can' happen, but that kind of code invites bugs if/when it is modified later. Pus, it shows that you thought about that case, even if it is only default: break; > + } > > - 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) { > + return err; > +} > + > +static int corsairpsu_hwmon_power(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; > - default: > - return -EOPNOTSUPP; > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1, val); Same as above and below. > } > - } else if (type == hwmon_in && attr == hwmon_in_input) { > + } > + > + return -EOPNOTSUPP; > +} > + > +static int corsairpsu_hwmon_in(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; > - default: > - return -EOPNOTSUPP; > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS, channel - 1, val); > + } > + break; > + case hwmon_in_crit: > + if (priv->in_crit_support & BIT(channel - 1)) { > + *val = priv->in_crit[channel - 1]; > + err = 0; > } > - } else if (type == hwmon_curr && attr == hwmon_curr_input) { > + break; > + case hwmon_in_lcrit: > + if (priv->in_lcrit_support & BIT(channel - 1)) { > + *val = priv->in_lcrit[channel - 1]; > + err = 0; > + } > + break; > + } > + > + return err; > +} > + > +static int corsairpsu_hwmon_curr(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; > - default: > - return -EOPNOTSUPP; > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val); > } > - } else { > - return -EOPNOTSUPP; > + break; > + case hwmon_curr_crit: > + if (priv->curr_crit_support & BIT(channel - 1)) { > + *val = priv->curr_crit[channel - 1]; > + err = 0; > + } > + break; > } > > - if (ret < 0) > - return ret; > + return err; > +} > > - return 0; Nice, you found that :-) > +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(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(priv, attr, channel, val); > + case hwmon_in: > + return corsairpsu_hwmon_in(priv, attr, channel, val); > + case hwmon_curr: > + return corsairpsu_hwmon_curr(priv, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > } > > static int corsairpsu_hwmon_ops_read_string(struct device *dev, enum hwmon_sensor_types type, > @@ -360,8 +481,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 +492,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 +634,8 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id > goto fail_and_stop; > } > > + corsairpsu_get_criticals(priv); > + > priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv, > &corsairpsu_chip_info, 0); > >
On Mon, 15 Mar 2021 08:53:25 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > On 3/15/21 8:02 AM, 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 a typo. > > > > 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 in: N/A > > What does that mean ? If it isn't supported by the power supply, > should we drop that entirely ? Maybe drop it via the is_visible > function if it is available for some variants, but always displaying > N/A doesn't add value. > > This is a bit odd, though, since I would assume it translates > to the PSU_CMD_IN_AMPS command. Any chance to track down what is > happening here ? I have one of the earliest PSUs of this series, it is just not supported on mine. I'm not sure if it would be worth the trouble to catch that and turn it off dynamically. > > 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) > > > This patch changes: > > - hwmon corsair-psu documentation > > - hwmon corsair-psu driver > > > > That is obvious; no reason to state in the commit log. > > > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net> > > --- > > 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 | 223 +++++++++++++++++++++------- > > 2 files changed, 185 insertions(+), 51 deletions(-) > > > > diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst > > index 396b95c9a76a..b77e53810a13 100644 > > --- a/Documentation/hwmon/corsair-psu.rst > > +++ b/Documentation/hwmon/corsair-psu.rst > > @@ -45,19 +45,30 @@ Sysfs entries > > ------------- > > > > ======================= ======================================================== > > +curr2_crit Current max critical value on the 12v psu rail > > +curr3_crit Current max critical value on the 5v psu rail > > +curr4_crit Current max critical value on the 3.3v psu rail > > curr1_input Total current usage > > curr2_input Current on the 12v psu rail > > curr3_input Current on the 5v psu rail > > curr4_input Current on the 3.3v psu rail > > I think it would be better to align those by index. > > curr1_input > curr2_input > curr2_crit > ... > > Personally I always list _input first, but that is a matter of > personal preference. Yeah, sounds reasonable. I can do that. > > fan1_input RPM of psu fan > > +in1_crit Voltage max critical value on the 12v psu rail > > +in2_crit Voltage max critical value on the 5v psu rail > > +in3_crit Voltage max critical value on the 3.3v psu rail > > in0_input Voltage of the psu ac input > > in1_input Voltage of the 12v psu rail > > in2_input Voltage of the 5v psu rail > > -in3_input Voltage of the 3.3 psu rail > > +in3_input Voltage of the 3.3v psu rail > > +in1_lcrit Voltage min critical value on the 12v psu rail > > +in2_lcrit Voltage min critical value on the 5v 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_crit Temperature max cirtical value of the psu vrm component > > +temp2_crit Temperature max critical value of psu case > > temp1_input Temperature of the psu vrm component > > temp2_input Temperature of the psu case > > ======================= ======================================================== > > diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c > > index b0953eeeb2d3..a176ac6a2540 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,14 @@ 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; > > }; > > > > /* some values are SMBus LINEAR11 data which need a conversion */ > > @@ -193,7 +207,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 +246,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 +277,175 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 > > rail, l return ret; > > } > > > > +static void corsairpsu_get_criticals(struct corsairpsu_data *priv) > > +{ > > + long tmp; > > + int ret; > > + u8 rail; > > Side note: Using anything but sizeof(int) for index variables often results in more > complex code because the compiler needs to mask index operations. This doesn't > typically apply to x86 because that architecure has byte operations, but to pretty > much every other architecture. Yeah, I know what you mean. I thought I match it to the function parameters. Though I would be more concerned about the "case 1 ... 3" stuff which is definitly no liked by static code analysis or even "-pedantic", it's not part of the C standards. > > + > > + priv->temp_crit_support = 0; > > + priv->in_lcrit_support = 0; > > + priv->in_crit_support = 0; > > + priv->curr_crit_support = 0; > > + > > + for (rail = 0; rail < TEMP_COUNT; ++rail) { > > + ret = corsairpsu_get_value(priv, PSU_CMD_TEMP_HCRIT, rail, &tmp); > > + if (ret == 0) { > > Nit: the ret variable isn't really needed here. > if (!corsairpsu_get_value(priv, PSU_CMD_TEMP_HCRIT, rail, &tmp)) { > works just as well. Personal preference, though. Huh? Damn it ... Nice spot, I really missed that completely. Oh boy, I even don't use the ret value at all ... Ah yeah, I switched the function to void. > > + priv->temp_crit_support |= BIT(rail); > > + priv->temp_crit[rail] = tmp; > > + } > > + } > > + > > + for (rail = 0; rail < RAIL_COUNT; ++rail) { > > + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_HCRIT, rail, &tmp); > > + if (ret == 0) { > > + priv->in_crit_support |= BIT(rail); > > + priv->in_crit[rail] = tmp; > > + } > > + > > + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_LCRIT, rail, &tmp); > > + if (ret == 0) { > > + priv->in_lcrit_support |= BIT(rail); > > + priv->in_lcrit[rail] = tmp; > > + } > > + > > + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS_HCRIT, rail, &tmp); > > + if (ret == 0) { > > + priv->curr_crit_support |= BIT(rail); > > + priv->curr_crit[rail] = tmp; > > + } > > + } > > +} > > + > > static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type, > > u32 attr, int channel) > > { > > - if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label)) > > + if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label || > > + attr == hwmon_temp_crit)) > > 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)) > > + else if (type == hwmon_in && (attr == hwmon_in_input || attr == hwmon_in_label || > > + attr == hwmon_in_lcrit || attr == hwmon_in_crit)) > > return 0444; > > - else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label)) > > + else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label || > > + attr == hwmon_curr_crit)) > > return 0444; > > > > return 0; > > } > > > > -static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 > > attr, > > - int channel, long *val) > > +static int corsairpsu_hwmon_temp(struct corsairpsu_data *priv, u32 attr, int channel, long > > *val) > > Please make those functions _<type>_read, not just _<type>. It makes the code easier > to understand, and we won't have to change it if write functions are ever added. You will laugh... I named the functions that way before, but then I was unhappy with hitting the 100 chars line length. ;-) > > { > > - struct corsairpsu_data *priv = dev_get_drvdata(dev); > > - int ret; > > + int err = -EOPNOTSUPP; > > + > > + if (channel < 2) { > > + switch (attr) { > > + case hwmon_temp_input: > > + return corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : > > PSU_CMD_TEMP0, > > + channel, val); > > + case hwmon_temp_crit: > > + if (priv->temp_crit_support & BIT(channel)) { > > + *val = priv->temp_crit[channel]; > > + err = 0; > > + } > > + } > > Dropping default cases from switch statements is never a good idea. It hides missing > break statements, like here, and it may result in compiler or static analyzer warnings > that not all situations are covered. Please don't do that (neither skipping break; > statements not dropping default:). Yes, it technically can' happen, but that kind of > code invites bugs if/when it is modified later. Pus, it shows that you thought about > that case, even if it is only > default: > break; This is odd, usually the compiler complains about this. But he did not in this patch version. But you are right, I will fix all the switches. > > + } > > > > - 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) { > > + return err; > > +} > > + > > +static int corsairpsu_hwmon_power(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; > > - default: > > - return -EOPNOTSUPP; > > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1, > > val); > > Same as above and below. > > > } > > - } else if (type == hwmon_in && attr == hwmon_in_input) { > > + } > > + > > + return -EOPNOTSUPP; > > +} > > + > > +static int corsairpsu_hwmon_in(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; > > - default: > > - return -EOPNOTSUPP; > > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS, channel - 1, > > val); > > + } > > + break; > > + case hwmon_in_crit: > > + if (priv->in_crit_support & BIT(channel - 1)) { > > + *val = priv->in_crit[channel - 1]; > > + err = 0; > > } > > - } else if (type == hwmon_curr && attr == hwmon_curr_input) { > > + break; > > + case hwmon_in_lcrit: > > + if (priv->in_lcrit_support & BIT(channel - 1)) { > > + *val = priv->in_lcrit[channel - 1]; > > + err = 0; > > + } > > + break; > > + } > > + > > + return err; > > +} > > + > > +static int corsairpsu_hwmon_curr(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; > > - default: > > - return -EOPNOTSUPP; > > + return corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val); > > } > > - } else { > > - return -EOPNOTSUPP; > > + break; > > + case hwmon_curr_crit: > > + if (priv->curr_crit_support & BIT(channel - 1)) { > > + *val = priv->curr_crit[channel - 1]; > > + err = 0; > > + } > > + break; > > } > > > > - if (ret < 0) > > - return ret; > > + return err; > > +} > > > > - return 0; > > Nice, you found that :-) I was unhappy with the complexity anyway. Splitting it in several functions was something I should have done right from the start. So yeah, I had to fix this ugly piece, too. > > +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(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(priv, attr, channel, val); > > + case hwmon_in: > > + return corsairpsu_hwmon_in(priv, attr, channel, val); > > + case hwmon_curr: > > + return corsairpsu_hwmon_curr(priv, attr, channel, val); > > + default: > > + return -EOPNOTSUPP; > > + } > > } > > > > static int corsairpsu_hwmon_ops_read_string(struct device *dev, enum hwmon_sensor_types type, > > @@ -360,8 +481,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 +492,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 +634,8 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct > > hid_device_id goto fail_and_stop; > > } > > > > + corsairpsu_get_criticals(priv); > > + > > priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv, > > &corsairpsu_chip_info, 0); > > > > >
On 3/15/21 9:55 AM, Wilken Gottwalt wrote: > On Mon, 15 Mar 2021 08:53:25 -0700 > Guenter Roeck <linux@roeck-us.net> wrote: > >> On 3/15/21 8:02 AM, 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 a typo. >>> >>> 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 in: N/A >> >> What does that mean ? If it isn't supported by the power supply, >> should we drop that entirely ? Maybe drop it via the is_visible >> function if it is available for some variants, but always displaying >> N/A doesn't add value. >> >> This is a bit odd, though, since I would assume it translates >> to the PSU_CMD_IN_AMPS command. Any chance to track down what is >> happening here ? > > I have one of the earliest PSUs of this series, it is just not supported on > mine. I'm not sure if it would be worth the trouble to catch that and turn > it off dynamically. > I think so, because otherwise we'll get complaints about it (people are really picky abut such things lately). Better not display it at all if it is not supported on a given PSU version. This should be relatively easy to catch in the is_visible function. Nice PS, anyway. Too bad it is so expensive (and large). Do you know if the HX750i uses the same protocol ? [ ... ] >>> +static void corsairpsu_get_criticals(struct corsairpsu_data *priv) >>> +{ >>> + long tmp; >>> + int ret; >>> + u8 rail; >> >> Side note: Using anything but sizeof(int) for index variables often results in more >> complex code because the compiler needs to mask index operations. This doesn't >> typically apply to x86 because that architecure has byte operations, but to pretty >> much every other architecture. > > Yeah, I know what you mean. I thought I match it to the function parameters. That doesn't really help if it makes the generated code more complex. > Though I would be more concerned about the "case 1 ... 3" stuff which is > definitly no liked by static code analysis or even "-pedantic", it's not > part of the C standards. > Hmm, which C standard are we talking about ? There are more than 1,800 instances of this in the Linux kernel, but I don't recall any static analyzer or compiler complaints about it. [ ... ] >>> -static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 >>> attr, >>> - int channel, long *val) >>> +static int corsairpsu_hwmon_temp(struct corsairpsu_data *priv, u32 attr, int channel, long >>> *val) >> >> Please make those functions _<type>_read, not just _<type>. It makes the code easier >> to understand, and we won't have to change it if write functions are ever added. > > You will laugh... I named the functions that way before, but then I was unhappy > with hitting the 100 chars line length. ;-) > Making the code less readable to meet a line limit isn't really that desirable. If you want to stick with one line, you could drop the "hwmon_" from function prefixes instead. Those don't really add any value. Thanks, Guenter
On Mon, 15 Mar 2021 11:00:53 -0700 Guenter Roeck <linux@roeck-us.net> wrote: > On 3/15/21 9:55 AM, Wilken Gottwalt wrote: > > On Mon, 15 Mar 2021 08:53:25 -0700 > > Guenter Roeck <linux@roeck-us.net> wrote: > > > >> On 3/15/21 8:02 AM, 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 a typo. > >>> > >>> 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 in: N/A > >> > >> What does that mean ? If it isn't supported by the power supply, > >> should we drop that entirely ? Maybe drop it via the is_visible > >> function if it is available for some variants, but always displaying > >> N/A doesn't add value. > >> > >> This is a bit odd, though, since I would assume it translates > >> to the PSU_CMD_IN_AMPS command. Any chance to track down what is > >> happening here ? > > > > I have one of the earliest PSUs of this series, it is just not supported on > > mine. I'm not sure if it would be worth the trouble to catch that and turn > > it off dynamically. > > > > I think so, because otherwise we'll get complaints about it (people > are really picky abut such things lately). Better not display it at all > if it is not supported on a given PSU version. This should be relatively > easy to catch in the is_visible function. So do you have any idea how to do it? The PSU does not tell you what is supported or not, you only find out by running the commands. I mean the only thing I think of is like I did it for the critical values, but only keeping the *_support bits. But if I do it that way, I actually should do it for all the commands. This is the point which I ment with "worth the trouble." > Nice PS, anyway. Too bad it is so expensive (and large). Do you know > if the HX750i uses the same protocol ? All HX_num_i and RM_num_i PSUs support the same protocol. There are only small differences in supported commands based on release version. What do you mean by "large"? The size of the case? All HXi and RMi should have the same size (standard ATX). Maybe you looked at one of the AXi series, which are server grade (and are not supported - they use a full USB protocol, no USB HID) and really expensive (but they sensors there can do even more). They are expansive because there is only new-old-stock available and they have some impressive specs. They can deliver some really high currents, something you need if you run a Threadripper like me or if you use a gfx card with current spikes (like AMD Vega or NVidia GTX 970). > [ ... ] > > >>> +static void corsairpsu_get_criticals(struct corsairpsu_data *priv) > >>> +{ > >>> + long tmp; > >>> + int ret; > >>> + u8 rail; > >> > >> Side note: Using anything but sizeof(int) for index variables often results in more > >> complex code because the compiler needs to mask index operations. This doesn't > >> typically apply to x86 because that architecure has byte operations, but to pretty > >> much every other architecture. > > > > Yeah, I know what you mean. I thought I match it to the function parameters. > > That doesn't really help if it makes the generated code more complex. > > > Though I would be more concerned about the "case 1 ... 3" stuff which is > > definitly no liked by static code analysis or even "-pedantic", it's not > > part of the C standards. > > > > Hmm, which C standard are we talking about ? There are more than 1,800 > instances of this in the Linux kernel, but I don't recall any static > analyzer or compiler complaints about it. > > [ ... ] > > >>> -static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 > >>> attr, > >>> - int channel, long *val) > >>> +static int corsairpsu_hwmon_temp(struct corsairpsu_data *priv, u32 attr, int channel, long > >>> *val) > >> > >> Please make those functions _<type>_read, not just _<type>. It makes the code easier > >> to understand, and we won't have to change it if write functions are ever added. > > > > You will laugh... I named the functions that way before, but then I was unhappy > > with hitting the 100 chars line length. ;-) > > > > Making the code less readable to meet a line limit isn't really that desirable. > If you want to stick with one line, you could drop the "hwmon_" from function prefixes > instead. Those don't really add any value. I know I know, it's a personal taste. I really dislike splitting functions headers about serveral lines, especially if indenting is tab based. > Thanks, > Guenter
On 3/16/21 7:21 AM, Wilken Gottwalt wrote: > On Mon, 15 Mar 2021 11:00:53 -0700 > Guenter Roeck <linux@roeck-us.net> wrote: > >> On 3/15/21 9:55 AM, Wilken Gottwalt wrote: >>> On Mon, 15 Mar 2021 08:53:25 -0700 >>> Guenter Roeck <linux@roeck-us.net> wrote: >>> >>>> On 3/15/21 8:02 AM, 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 a typo. >>>>> >>>>> 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 in: N/A >>>> >>>> What does that mean ? If it isn't supported by the power supply, >>>> should we drop that entirely ? Maybe drop it via the is_visible >>>> function if it is available for some variants, but always displaying >>>> N/A doesn't add value. >>>> >>>> This is a bit odd, though, since I would assume it translates >>>> to the PSU_CMD_IN_AMPS command. Any chance to track down what is >>>> happening here ? >>> >>> I have one of the earliest PSUs of this series, it is just not supported on >>> mine. I'm not sure if it would be worth the trouble to catch that and turn >>> it off dynamically. >>> >> >> I think so, because otherwise we'll get complaints about it (people >> are really picky abut such things lately). Better not display it at all >> if it is not supported on a given PSU version. This should be relatively >> easy to catch in the is_visible function. > > So do you have any idea how to do it? The PSU does not tell you what is > supported or not, you only find out by running the commands. I mean the > only thing I think of is like I did it for the critical values, but only > keeping the *_support bits. But if I do it that way, I actually should do > it for all the commands. This is the point which I ment with "worth the > trouble." > It is not really necessary to do it for all commands; only for those known to not be supported on all power supplies. >> Nice PS, anyway. Too bad it is so expensive (and large). Do you know >> if the HX750i uses the same protocol ? > > All HX_num_i and RM_num_i PSUs support the same protocol. There are only > small differences in supported commands based on release version. What do > you mean by "large"? The size of the case? All HXi and RMi should have > the same size (standard ATX). Maybe you looked at one of the AXi series, They don't fit into my small mini-ATX chassis, and the "i" series all seem to be full-size ATX. [ ... ] >> >> Making the code less readable to meet a line limit isn't really that desirable. >> If you want to stick with one line, you could drop the "hwmon_" from function prefixes >> instead. Those don't really add any value. > > I know I know, it's a personal taste. I really dislike splitting functions > headers about serveral lines, especially if indenting is tab based. > Fortunately you can get there by dropping 'hwmon_' from the function names. Thanks, Guenter
diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst index 396b95c9a76a..b77e53810a13 100644 --- a/Documentation/hwmon/corsair-psu.rst +++ b/Documentation/hwmon/corsair-psu.rst @@ -45,19 +45,30 @@ Sysfs entries ------------- ======================= ======================================================== +curr2_crit Current max critical value on the 12v psu rail +curr3_crit Current max critical value on the 5v psu rail +curr4_crit Current max critical value on the 3.3v psu rail curr1_input Total current usage curr2_input Current on the 12v psu rail curr3_input Current on the 5v psu rail curr4_input Current on the 3.3v psu rail fan1_input RPM of psu fan +in1_crit Voltage max critical value on the 12v psu rail +in2_crit Voltage max critical value on the 5v psu rail +in3_crit Voltage max critical value on the 3.3v psu rail in0_input Voltage of the psu ac input in1_input Voltage of the 12v psu rail in2_input Voltage of the 5v psu rail -in3_input Voltage of the 3.3 psu rail +in3_input Voltage of the 3.3v psu rail +in1_lcrit Voltage min critical value on the 12v psu rail +in2_lcrit Voltage min critical value on the 5v 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_crit Temperature max cirtical value of the psu vrm component +temp2_crit Temperature max critical value of psu case temp1_input Temperature of the psu vrm component temp2_input Temperature of the psu case ======================= ======================================================== diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c index b0953eeeb2d3..a176ac6a2540 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,14 @@ 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; }; /* some values are SMBus LINEAR11 data which need a conversion */ @@ -193,7 +207,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 +246,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 +277,175 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l return ret; } +static void corsairpsu_get_criticals(struct corsairpsu_data *priv) +{ + long tmp; + int ret; + u8 rail; + + priv->temp_crit_support = 0; + priv->in_lcrit_support = 0; + priv->in_crit_support = 0; + priv->curr_crit_support = 0; + + for (rail = 0; rail < TEMP_COUNT; ++rail) { + ret = corsairpsu_get_value(priv, PSU_CMD_TEMP_HCRIT, rail, &tmp); + if (ret == 0) { + priv->temp_crit_support |= BIT(rail); + priv->temp_crit[rail] = tmp; + } + } + + for (rail = 0; rail < RAIL_COUNT; ++rail) { + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_HCRIT, rail, &tmp); + if (ret == 0) { + priv->in_crit_support |= BIT(rail); + priv->in_crit[rail] = tmp; + } + + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS_LCRIT, rail, &tmp); + if (ret == 0) { + priv->in_lcrit_support |= BIT(rail); + priv->in_lcrit[rail] = tmp; + } + + ret = corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS_HCRIT, rail, &tmp); + if (ret == 0) { + priv->curr_crit_support |= BIT(rail); + priv->curr_crit[rail] = tmp; + } + } +} + static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr, int channel) { - if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label)) + if (type == hwmon_temp && (attr == hwmon_temp_input || attr == hwmon_temp_label || + attr == hwmon_temp_crit)) 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)) + else if (type == hwmon_in && (attr == hwmon_in_input || attr == hwmon_in_label || + attr == hwmon_in_lcrit || attr == hwmon_in_crit)) return 0444; - else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label)) + else if (type == hwmon_curr && (attr == hwmon_curr_input || attr == hwmon_curr_label || + attr == hwmon_curr_crit)) return 0444; return 0; } -static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, - int channel, long *val) +static int corsairpsu_hwmon_temp(struct corsairpsu_data *priv, u32 attr, int channel, long *val) { - struct corsairpsu_data *priv = dev_get_drvdata(dev); - int ret; + int err = -EOPNOTSUPP; + + if (channel < 2) { + switch (attr) { + case hwmon_temp_input: + return corsairpsu_get_value(priv, channel ? PSU_CMD_TEMP1 : PSU_CMD_TEMP0, + channel, val); + case hwmon_temp_crit: + if (priv->temp_crit_support & BIT(channel)) { + *val = priv->temp_crit[channel]; + err = 0; + } + } + } - 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) { + return err; +} + +static int corsairpsu_hwmon_power(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; - default: - return -EOPNOTSUPP; + return corsairpsu_get_value(priv, PSU_CMD_RAIL_WATTS, channel - 1, val); } - } else if (type == hwmon_in && attr == hwmon_in_input) { + } + + return -EOPNOTSUPP; +} + +static int corsairpsu_hwmon_in(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; - default: - return -EOPNOTSUPP; + return corsairpsu_get_value(priv, PSU_CMD_RAIL_VOLTS, channel - 1, val); + } + break; + case hwmon_in_crit: + if (priv->in_crit_support & BIT(channel - 1)) { + *val = priv->in_crit[channel - 1]; + err = 0; } - } else if (type == hwmon_curr && attr == hwmon_curr_input) { + break; + case hwmon_in_lcrit: + if (priv->in_lcrit_support & BIT(channel - 1)) { + *val = priv->in_lcrit[channel - 1]; + err = 0; + } + break; + } + + return err; +} + +static int corsairpsu_hwmon_curr(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; - default: - return -EOPNOTSUPP; + return corsairpsu_get_value(priv, PSU_CMD_RAIL_AMPS, channel - 1, val); } - } else { - return -EOPNOTSUPP; + break; + case hwmon_curr_crit: + if (priv->curr_crit_support & BIT(channel - 1)) { + *val = priv->curr_crit[channel - 1]; + err = 0; + } + 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(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(priv, attr, channel, val); + case hwmon_in: + return corsairpsu_hwmon_in(priv, attr, channel, val); + case hwmon_curr: + return corsairpsu_hwmon_curr(priv, attr, channel, val); + default: + return -EOPNOTSUPP; + } } static int corsairpsu_hwmon_ops_read_string(struct device *dev, enum hwmon_sensor_types type, @@ -360,8 +481,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 +492,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 +634,8 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct hid_device_id goto fail_and_stop; } + corsairpsu_get_criticals(priv); + priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv, &corsairpsu_chip_info, 0);
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 a typo. 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 in: N/A 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) This patch changes: - hwmon corsair-psu documentation - hwmon corsair-psu driver Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net> --- 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 | 223 +++++++++++++++++++++------- 2 files changed, 185 insertions(+), 51 deletions(-)