Re: [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes

From: Guenter Roeck <linux@roeck-us.net>
To: "Jan Kundrát" <jan.kundrat@cesnet.cz>
Cc: linux-hwmon@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
	kubernat@cesnet.cz
Subject: Re: [PATCH 4/7] hwmon: (max31790) Add support for fanX_enable attributes
Date: Wed, 2 Jun 2021 09:43:53 -0700	[thread overview]
Message-ID: <20210602164353.GA2908914@roeck-us.net> (raw)
In-Reply-To: <e073d9e2-d7d1-40fb-9717-971833cb25ca@cesnet.cz>

On Wed, Jun 02, 2021 at 03:04:35PM +0200, Jan Kundrát wrote:
> On středa 26. května 2021 17:40:19 CEST, Guenter Roeck wrote:
> > Since pwmX_enable is now fixed and only handles pwm support instead
> > of also enabling/disabling fan tachometers, we need an explicit means
> > to do that.
> > 
> > For fan channels 7..12, display the enable status if the channel
> > is configured for fan speed reporting. The displayed status matches
> > the value of the companion channel but is read-only.
> 
> This phrasing is confusing to me. That's once again that "configured to"
> which in this context doesn't refer to the kernel, but to an initial config
> of the chip. I suggest the following:
> 
> Fan channels 7..12 are only available when the chip has been configured with
> PWM output N-6 disabled. This configuration is outside of scope of the
> kernel. The displayed status matches the value of the companion channel but
> is read-only.
> 
> > +fan[1-12]_enable   RW  0=disable fan speed monitoring, 1=enable fan
> > speed monitoring
> > +                       The value is RO for companion channels (7-12).
> > For those
> > +                       channels, the value matches the value of the
> > primary channel.
> 
> I realize that it probably doesn't belong to this patch because it affects
> the other fan_* files, but the docs would be improved by something like:
> 
> Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%)
> monitoring and control of fan RPM as well as detection of fan failure.
> Six pins are dedicated tachometer inputs. Any of the six PWM outputs can
> -also be configured to serve as tachometer inputs.
> +also be reconfigured to serve as tachometer inputs by the firmware. The
> +kernel will respect the initial configuration of the chip.
> 

"Precise (+/-1%)" sounds like chip advertising, which I'd rather avoid.

> Want an extra patch on top of this series?
> 
> > +	case hwmon_fan_enable:
> > +		config = data->fan_config[channel];
> > +		if (val == 0) {
> > +			/* Disabling TACH_INPUT_EN has no effect in RPM_MODE */
> > +			if (!(config & MAX31790_FAN_CFG_RPM_MODE))
> > +				config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN;
> 
> This means that a "nonsensical" write from userspace will be silently
> ignored, doesn't it? I think it should return an error instead.
> 
Trade-off between confusing users and trying to match the ABI with somewhat
odd chip capabilities. The above isn't exactly specified; it is the result
of trial and error (and a reason why the configuration register must be
treated as volatile when using regmap).

One can argue one way or another. For now I'd rather keep the code as is
because I am away from the evaluation board for the next few weeks and
won't be able to test any functional changes until I am back.
Given the complexity of the chip and its sometimes odd behavior I'll want
to be able to do that kind of testing. Is this important for you ? If so,
we can move forward with patches 1-3 of the series and leave this and
subsequent patches for later.

Thanks,
Guenter