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

Jan Kundrát
2021-06-02T13:04:38Z

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