[PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[Wilken Gottwalt]

Add reporting if the PSU is running in single or multi rail mode via
ocpmode debugfs entry. Also update the documentation accordingly.

Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
---
Changes in v2:
  - fixed spelling issues in commit message
---
 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

Re: [PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[Guenter Roeck]

On 8/10/22 06:53, Wilken Gottwalt wrote:
> Add reporting if the PSU is running in single or multi rail mode via
> ocpmode debugfs entry. Also update the documentation accordingly.
> 
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> ---
> Changes in v2:
>    - fixed spelling issues in commit message

You did not address or even provide feedback on my second comment.

Guenter

> ---
>   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

Re: [PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[Wilken Gottwalt]

On Wed, 10 Aug 2022 09:31:21 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 8/10/22 06:53, Wilken Gottwalt wrote:
> > Add reporting if the PSU is running in single or multi rail mode via
> > ocpmode debugfs entry. Also update the documentation accordingly.
> > 
> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> > ---
> > Changes in v2:
> >    - fixed spelling issues in commit message
> 
> You did not address or even provide feedback on my second comment.

Oh darn ... sorry, I was quite busy and didn't really pay attention. I will
answer the earlier mail and think about it.

Though, maybe you can help me with that what keeps me so busy. Would it be okay
to use a kthread in a hwmon driver to do sampling (500ms - 10s) in conjunction
with HWMON_C_UPDATE_INTERVAL, or is this a strict no-no? I know it is actually
used to set a sample/update rate in a sensor (-register), but this USB-HID
approach is a pure polling thing. It seems to work quite and enables the driver
to collect data quite early in the boot process.

greetings

Re: [PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[Guenter Roeck]

On 8/10/22 09:56, Wilken Gottwalt wrote:
> On Wed, 10 Aug 2022 09:31:21 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 8/10/22 06:53, Wilken Gottwalt wrote:
>>> Add reporting if the PSU is running in single or multi rail mode via
>>> ocpmode debugfs entry. Also update the documentation accordingly.
>>>
>>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
>>> ---
>>> Changes in v2:
>>>     - fixed spelling issues in commit message
>>
>> You did not address or even provide feedback on my second comment.
> 
> Oh darn ... sorry, I was quite busy and didn't really pay attention. I will
> answer the earlier mail and think about it.
> 
> Though, maybe you can help me with that what keeps me so busy. Would it be okay
> to use a kthread in a hwmon driver to do sampling (500ms - 10s) in conjunction
> with HWMON_C_UPDATE_INTERVAL, or is this a strict no-no? I know it is actually
> used to set a sample/update rate in a sensor (-register), but this USB-HID
> approach is a pure polling thing. It seems to work quite and enables the driver
> to collect data quite early in the boot process.
> 

It really depends. Is it _necessary_ ? The pwm-fan driver uses a timer for
periodic polling, but that is because it has to. We should not do it purely
for convenience, and from the code I don't immediately see why it would
be necessary.

Guenter

Re: [PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[Wilken Gottwalt]

On Wed, 10 Aug 2022 10:29:08 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 8/10/22 09:56, Wilken Gottwalt wrote:
> > On Wed, 10 Aug 2022 09:31:21 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> On 8/10/22 06:53, Wilken Gottwalt wrote:
> >>> Add reporting if the PSU is running in single or multi rail mode via
> >>> ocpmode debugfs entry. Also update the documentation accordingly.
> >>>
> >>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> >>> ---
> >>> Changes in v2:
> >>>     - fixed spelling issues in commit message
> >>
> >> You did not address or even provide feedback on my second comment.
> > 
> > Oh darn ... sorry, I was quite busy and didn't really pay attention. I will
> > answer the earlier mail and think about it.
> > 
> > Though, maybe you can help me with that what keeps me so busy. Would it be okay
> > to use a kthread in a hwmon driver to do sampling (500ms - 10s) in conjunction
> > with HWMON_C_UPDATE_INTERVAL, or is this a strict no-no? I know it is actually
> > used to set a sample/update rate in a sensor (-register), but this USB-HID
> > approach is a pure polling thing. It seems to work quite and enables the driver
> > to collect data quite early in the boot process.
> > 
> 
> It really depends. Is it _necessary_ ? The pwm-fan driver uses a timer for
> periodic polling, but that is because it has to. We should not do it purely
> for convenience, and from the code I don't immediately see why it would
> be necessary.

Together with the polling I would add encountered lowest and highest values and
the average of basically all available sensors (kind of session statistics). I
know it is a bit odd, but currently these power supplies are sold again in a
newer version and people really like to use them in their servers/workstations
because of the "realtime" data and this driver. No joke, but I really got
several requests to add this and I must admit I have quite some fun implementing
it.

Maybe I can provide a patch after I'm done and you can decide if this is okay
or not. After all I provide an external more enhanced driver via github where
some features are added, which are a clear no-go. It would be nice to have that
in mainline, but I'm absolutely fine with a "no".

greetings,
Wilken

Re: [PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[Guenter Roeck]

On 8/10/22 10:48, Wilken Gottwalt wrote:
> On Wed, 10 Aug 2022 10:29:08 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 8/10/22 09:56, Wilken Gottwalt wrote:
>>> On Wed, 10 Aug 2022 09:31:21 -0700
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>>> On 8/10/22 06:53, Wilken Gottwalt wrote:
>>>>> Add reporting if the PSU is running in single or multi rail mode via
>>>>> ocpmode debugfs entry. Also update the documentation accordingly.
>>>>>
>>>>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
>>>>> ---
>>>>> Changes in v2:
>>>>>      - fixed spelling issues in commit message
>>>>
>>>> You did not address or even provide feedback on my second comment.
>>>
>>> Oh darn ... sorry, I was quite busy and didn't really pay attention. I will
>>> answer the earlier mail and think about it.
>>>
>>> Though, maybe you can help me with that what keeps me so busy. Would it be okay
>>> to use a kthread in a hwmon driver to do sampling (500ms - 10s) in conjunction
>>> with HWMON_C_UPDATE_INTERVAL, or is this a strict no-no? I know it is actually
>>> used to set a sample/update rate in a sensor (-register), but this USB-HID
>>> approach is a pure polling thing. It seems to work quite and enables the driver
>>> to collect data quite early in the boot process.
>>>
>>
>> It really depends. Is it _necessary_ ? The pwm-fan driver uses a timer for
>> periodic polling, but that is because it has to. We should not do it purely
>> for convenience, and from the code I don't immediately see why it would
>> be necessary.
> 
> Together with the polling I would add encountered lowest and highest values and
> the average of basically all available sensors (kind of session statistics). I
> know it is a bit odd, but currently these power supplies are sold again in a
> newer version and people really like to use them in their servers/workstations
> because of the "realtime" data and this driver. No joke, but I really got
> several requests to add this and I must admit I have quite some fun implementing
> it.
> 

That is out of scope for a kernel driver. If desired, a user space application
should do the polling and calculate statistics such as lowest/highest or averages.

Guenter

> Maybe I can provide a patch after I'm done and you can decide if this is okay
> or not. After all I provide an external more enhanced driver via github where
> some features are added, which are a clear no-go. It would be nice to have that
> in mainline, but I'm absolutely fine with a "no".
> 
> greetings,
> Wilken

Re: [PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[Wilken Gottwalt]

On Wed, 10 Aug 2022 11:21:36 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 8/10/22 10:48, Wilken Gottwalt wrote:
> > On Wed, 10 Aug 2022 10:29:08 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> >> On 8/10/22 09:56, Wilken Gottwalt wrote:
> >>> On Wed, 10 Aug 2022 09:31:21 -0700
> >>> Guenter Roeck <linux@roeck-us.net> wrote:
> >>>
> >>>> On 8/10/22 06:53, Wilken Gottwalt wrote:
> >>>>> Add reporting if the PSU is running in single or multi rail mode via
> >>>>> ocpmode debugfs entry. Also update the documentation accordingly.
> >>>>>
> >>>>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> >>>>> ---
> >>>>> Changes in v2:
> >>>>>      - fixed spelling issues in commit message
> >>>>
> >>>> You did not address or even provide feedback on my second comment.
> >>>
> >>> Oh darn ... sorry, I was quite busy and didn't really pay attention. I will
> >>> answer the earlier mail and think about it.
> >>>
> >>> Though, maybe you can help me with that what keeps me so busy. Would it be okay
> >>> to use a kthread in a hwmon driver to do sampling (500ms - 10s) in conjunction
> >>> with HWMON_C_UPDATE_INTERVAL, or is this a strict no-no? I know it is actually
> >>> used to set a sample/update rate in a sensor (-register), but this USB-HID
> >>> approach is a pure polling thing. It seems to work quite and enables the driver
> >>> to collect data quite early in the boot process.
> >>>
> >>
> >> It really depends. Is it _necessary_ ? The pwm-fan driver uses a timer for
> >> periodic polling, but that is because it has to. We should not do it purely
> >> for convenience, and from the code I don't immediately see why it would
> >> be necessary.
> > 
> > Together with the polling I would add encountered lowest and highest values and
> > the average of basically all available sensors (kind of session statistics). I
> > know it is a bit odd, but currently these power supplies are sold again in a
> > newer version and people really like to use them in their servers/workstations
> > because of the "realtime" data and this driver. No joke, but I really got
> > several requests to add this and I must admit I have quite some fun implementing
> > it.
> > 
> 
> That is out of scope for a kernel driver. If desired, a user space application
> should do the polling and calculate statistics such as lowest/highest or averages.

That is exactly what I told the requesting people. Now it is in the public
record and I hope that kind of requests go down a bit, at least for pushing
this in the mainline kernel.

greetings,
Wilken

Re: [PATCH v2] hwmon: corsair-psu: add reporting of rail mode via debugfs

[Guenter Roeck]

On 8/11/22 01:05, Wilken Gottwalt wrote:
> On Wed, 10 Aug 2022 11:21:36 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 8/10/22 10:48, Wilken Gottwalt wrote:
>>> On Wed, 10 Aug 2022 10:29:08 -0700
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>>> On 8/10/22 09:56, Wilken Gottwalt wrote:
>>>>> On Wed, 10 Aug 2022 09:31:21 -0700
>>>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>
>>>>>> On 8/10/22 06:53, Wilken Gottwalt wrote:
>>>>>>> Add reporting if the PSU is running in single or multi rail mode via
>>>>>>> ocpmode debugfs entry. Also update the documentation accordingly.
>>>>>>>
>>>>>>> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>>       - fixed spelling issues in commit message
>>>>>>
>>>>>> You did not address or even provide feedback on my second comment.
>>>>>
>>>>> Oh darn ... sorry, I was quite busy and didn't really pay attention. I will
>>>>> answer the earlier mail and think about it.
>>>>>
>>>>> Though, maybe you can help me with that what keeps me so busy. Would it be okay
>>>>> to use a kthread in a hwmon driver to do sampling (500ms - 10s) in conjunction
>>>>> with HWMON_C_UPDATE_INTERVAL, or is this a strict no-no? I know it is actually
>>>>> used to set a sample/update rate in a sensor (-register), but this USB-HID
>>>>> approach is a pure polling thing. It seems to work quite and enables the driver
>>>>> to collect data quite early in the boot process.
>>>>>
>>>>
>>>> It really depends. Is it _necessary_ ? The pwm-fan driver uses a timer for
>>>> periodic polling, but that is because it has to. We should not do it purely
>>>> for convenience, and from the code I don't immediately see why it would
>>>> be necessary.
>>>
>>> Together with the polling I would add encountered lowest and highest values and
>>> the average of basically all available sensors (kind of session statistics). I
>>> know it is a bit odd, but currently these power supplies are sold again in a
>>> newer version and people really like to use them in their servers/workstations
>>> because of the "realtime" data and this driver. No joke, but I really got
>>> several requests to add this and I must admit I have quite some fun implementing
>>> it.
>>>
>>
>> That is out of scope for a kernel driver. If desired, a user space application
>> should do the polling and calculate statistics such as lowest/highest or averages.
> 
> That is exactly what I told the requesting people. Now it is in the public
> record and I hope that kind of requests go down a bit, at least for pushing
> this in the mainline kernel.
> 

 From Documentation/hwmon/sysfs-interface.rst:

"
All entries (except name) are optional, and should only be created in a
given driver if the chip has the feature.
"

Think about it - having the kernel collect statistics like this would only
make sense if it was added for all drivers, ie in the hwmon core. That would
end up burdening everyone, not just the few people who want it.

Guenter