Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Wed, 10 Aug 2022 06:30:12 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Wilken Gottwalt <wilken.gottwalt@posteo.net>
Cc:     linux-kernel@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
        Jonathan Corbet <corbet@lwn.net>, linux-hwmon@vger.kernel.org
Subject: Re: [PATCH] hwmon: corsair-psu: add reporting of rail mode via
 debugfs
Message-ID: <20220810133012.GB274220@roeck-us.net>
References: <YvN4SbnAp3jl+Vzo@monster.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YvN4SbnAp3jl+Vzo@monster.localdomain>
Precedence: bulk

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 <wilken.gottwalt@posteo.net>
> ---
>  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
>