Adds reporting via debugfs if the PSU is running in single or multi rail
mode. Also updates the documentation accordingly.
Signed-off-by: Wilken Gottwalt <[email protected]>
---
Documentation/hwmon/corsair-psu.rst | 5 +++--
drivers/hwmon/corsair-psu.c | 21 ++++++++++++++++++++-
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst
index e8378e7a1d8c..c3a76305c587 100644
--- a/Documentation/hwmon/corsair-psu.rst
+++ b/Documentation/hwmon/corsair-psu.rst
@@ -86,8 +86,9 @@ Debugfs entries
---------------
======================= ========================================================
-uptime Current uptime of the psu
+ocpmode Single or multi rail mode of the PCIe power connectors
+product Product name of the psu
+uptime Session uptime of the psu
uptime_total Total uptime of the psu
vendor Vendor name of the psu
-product Product name of the psu
======================= ========================================================
diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
index 14389fd7afb8..9d103613db39 100644
--- a/drivers/hwmon/corsair-psu.c
+++ b/drivers/hwmon/corsair-psu.c
@@ -71,9 +71,10 @@
#define PSU_CMD_RAIL_WATTS 0x96
#define PSU_CMD_VEND_STR 0x99
#define PSU_CMD_PROD_STR 0x9A
-#define PSU_CMD_TOTAL_WATTS 0xEE
#define PSU_CMD_TOTAL_UPTIME 0xD1
#define PSU_CMD_UPTIME 0xD2
+#define PSU_CMD_OCPMODE 0xD8
+#define PSU_CMD_TOTAL_WATTS 0xEE
#define PSU_CMD_INIT 0xFE
#define L_IN_VOLTS "v_in"
@@ -268,6 +269,7 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
break;
case PSU_CMD_TOTAL_UPTIME:
case PSU_CMD_UPTIME:
+ case PSU_CMD_OCPMODE:
*val = tmp;
break;
default:
@@ -660,6 +662,22 @@ static int product_show(struct seq_file *seqf, void *unused)
}
DEFINE_SHOW_ATTRIBUTE(product);
+static int ocpmode_show(struct seq_file *seqf, void *unused)
+{
+ struct corsairpsu_data *priv = seqf->private;
+ long val;
+ int ret;
+
+ ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
+ if (ret < 0)
+ seq_puts(seqf, "N/A\n");
+ else
+ seq_printf(seqf, "%s\n", (val == 0x02) ? "multi rail" : "single rail");
+
+ return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(ocpmode);
+
static void corsairpsu_debugfs_init(struct corsairpsu_data *priv)
{
char name[32];
@@ -671,6 +689,7 @@ static void corsairpsu_debugfs_init(struct corsairpsu_data *priv)
debugfs_create_file("uptime_total", 0444, priv->debugfs, priv, &uptime_total_fops);
debugfs_create_file("vendor", 0444, priv->debugfs, priv, &vendor_fops);
debugfs_create_file("product", 0444, priv->debugfs, priv, &product_fops);
+ debugfs_create_file("ocpmode", 0444, priv->debugfs, priv, &ocpmode_fops);
}
#else
--
2.37.1
On Wed, Aug 10, 2022 at 09:20:09AM +0000, Wilken Gottwalt wrote:
> Adds reporting via debugfs if the PSU is running in single or multi rail
> mode. Also updates the documentation accordingly.
>
Please use imperative mode.
"Adds reporting" -> Report
"updates" -> update
See Documentation/process/submitting-patches.rst,
"Describe your changes".
> Signed-off-by: Wilken Gottwalt <[email protected]>
> ---
> Documentation/hwmon/corsair-psu.rst | 5 +++--
> drivers/hwmon/corsair-psu.c | 21 ++++++++++++++++++++-
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst
> index e8378e7a1d8c..c3a76305c587 100644
> --- a/Documentation/hwmon/corsair-psu.rst
> +++ b/Documentation/hwmon/corsair-psu.rst
> @@ -86,8 +86,9 @@ Debugfs entries
> ---------------
>
> ======================= ========================================================
> -uptime Current uptime of the psu
> +ocpmode Single or multi rail mode of the PCIe power connectors
> +product Product name of the psu
> +uptime Session uptime of the psu
> uptime_total Total uptime of the psu
> vendor Vendor name of the psu
> -product Product name of the psu
> ======================= ========================================================
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index 14389fd7afb8..9d103613db39 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -71,9 +71,10 @@
> #define PSU_CMD_RAIL_WATTS 0x96
> #define PSU_CMD_VEND_STR 0x99
> #define PSU_CMD_PROD_STR 0x9A
> -#define PSU_CMD_TOTAL_WATTS 0xEE
> #define PSU_CMD_TOTAL_UPTIME 0xD1
> #define PSU_CMD_UPTIME 0xD2
> +#define PSU_CMD_OCPMODE 0xD8
> +#define PSU_CMD_TOTAL_WATTS 0xEE
> #define PSU_CMD_INIT 0xFE
>
> #define L_IN_VOLTS "v_in"
> @@ -268,6 +269,7 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
> break;
> case PSU_CMD_TOTAL_UPTIME:
> case PSU_CMD_UPTIME:
> + case PSU_CMD_OCPMODE:
> *val = tmp;
> break;
> default:
> @@ -660,6 +662,22 @@ static int product_show(struct seq_file *seqf, void *unused)
> }
> DEFINE_SHOW_ATTRIBUTE(product);
>
> +static int ocpmode_show(struct seq_file *seqf, void *unused)
> +{
> + struct corsairpsu_data *priv = seqf->private;
> + long val;
> + int ret;
> +
> + ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
> + if (ret < 0)
> + seq_puts(seqf, "N/A\n");
> + else
> + seq_printf(seqf, "%s\n", (val == 0x02) ? "multi rail" : "single rail");
If this is not always available, would it be better not to create the file
in the first place ? If that is not feasible, it should at least be
documented that the value is not always available to ensure that no one
complains about it (or at least no one who read the documentation).
Also, is the value strictly 0x02 for multi-rail configurations, or
is that possibly just a bit or the number of rails ?
Thanks,
Guenter
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(ocpmode);
> +
> static void corsairpsu_debugfs_init(struct corsairpsu_data *priv)
> {
> char name[32];
> @@ -671,6 +689,7 @@ static void corsairpsu_debugfs_init(struct corsairpsu_data *priv)
> debugfs_create_file("uptime_total", 0444, priv->debugfs, priv, &uptime_total_fops);
> debugfs_create_file("vendor", 0444, priv->debugfs, priv, &vendor_fops);
> debugfs_create_file("product", 0444, priv->debugfs, priv, &product_fops);
> + debugfs_create_file("ocpmode", 0444, priv->debugfs, priv, &ocpmode_fops);
> }
>
> #else
> --
> 2.37.1
>
On 8/10/22 10:35, Wilken Gottwalt wrote:
> On Wed, 10 Aug 2022 06:30:12 -0700
> Guenter Roeck <[email protected]> wrote:
>
>>> +static int ocpmode_show(struct seq_file *seqf, void *unused)
>>> +{
>>> + struct corsairpsu_data *priv = seqf->private;
>>> + long val;
>>> + int ret;
>>> +
>>> + ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
>>> + if (ret < 0)
>>> + seq_puts(seqf, "N/A\n");
>>> + else
>>> + seq_printf(seqf, "%s\n", (val == 0x02) ? "multi rail" : "single rail");
>>
>> If this is not always available, would it be better not to create the file
>> in the first place ? If that is not feasible, it should at least be
>> documented that the value is not always available to ensure that no one
>> complains about it (or at least no one who read the documentation).
>>
>> Also, is the value strictly 0x02 for multi-rail configurations, or
>> is that possibly just a bit or the number of rails ?
>
> The mode is actually switchable on fly, similar to the fan. I do not want to
> provide the switching functionality, because also similar to the fan controls,
> it can be used to damage the PSU. It is part of the over current protection
> system (hence the name ocpmode) and people use the RAW interface to switch the
> fans and the ocpmode. This is also the reason I made it that way, because you
> could poll the information right in the process of switching, which can result
> in bogus values. 0x02 is the value for "switching to multi rail was successful",
> every other value is considered "single rail mode". Or you get a malformed
> message which results in "N/A" or unknown. So yes, you are right, I could at
> least add a define for the value and be more clear in the documentation. Would
> that be okay for you?
>
Define or not is up to you, but it would be nice if at least some of the explanation
above would be documented in the driver.
Guenter
On Wed, 10 Aug 2022 06:30:12 -0700
Guenter Roeck <[email protected]> wrote:
> > +static int ocpmode_show(struct seq_file *seqf, void *unused)
> > +{
> > + struct corsairpsu_data *priv = seqf->private;
> > + long val;
> > + int ret;
> > +
> > + ret = corsairpsu_get_value(priv, PSU_CMD_OCPMODE, 0, &val);
> > + if (ret < 0)
> > + seq_puts(seqf, "N/A\n");
> > + else
> > + seq_printf(seqf, "%s\n", (val == 0x02) ? "multi rail" : "single rail");
>
> If this is not always available, would it be better not to create the file
> in the first place ? If that is not feasible, it should at least be
> documented that the value is not always available to ensure that no one
> complains about it (or at least no one who read the documentation).
>
> Also, is the value strictly 0x02 for multi-rail configurations, or
> is that possibly just a bit or the number of rails ?
The mode is actually switchable on fly, similar to the fan. I do not want to
provide the switching functionality, because also similar to the fan controls,
it can be used to damage the PSU. It is part of the over current protection
system (hence the name ocpmode) and people use the RAW interface to switch the
fans and the ocpmode. This is also the reason I made it that way, because you
could poll the information right in the process of switching, which can result
in bogus values. 0x02 is the value for "switching to multi rail was successful",
every other value is considered "single rail mode". Or you get a malformed
message which results in "N/A" or unknown. So yes, you are right, I could at
least add a define for the value and be more clear in the documentation. Would
that be okay for you?
greetings,
Wilken