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

From: "Jan Kundrát" <jan.kundrat@cesnet.cz>
To: Guenter Roeck <linux@roeck-us.net>
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, 02 Jun 2021 15:04:35 +0200	[thread overview]
Message-ID: <e073d9e2-d7d1-40fb-9717-971833cb25ca@cesnet.cz> (raw)
In-Reply-To: <20210526154022.3223012-5-linux@roeck-us.net>

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.

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.

Cheers,
Jan