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

[Guenter Roeck]

On 8/10/22 10:35, Wilken Gottwalt wrote:
> On Wed, 10 Aug 2022 06:30:12 -0700
> Guenter Roeck <linux@xxxxxxxxxxxx> 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