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;
Date:   Wed, 10 Aug 2022 17:35:41 +0000
From:   Wilken Gottwalt <wilken.gottwalt@posteo.net>
To:     Guenter Roeck <linux@roeck-us.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: <20220810193541.29f95ec5@posteo.net>
In-Reply-To: <20220810133012.GB274220@roeck-us.net>
References: <YvN4SbnAp3jl+Vzo@monster.localdomain>
        <20220810133012.GB274220@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 10 Aug 2022 06:30:12 -0700
Guenter Roeck <linux@roeck-us.net> 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