On 7/1/24 09:29, Quentin Schulz wrote:
Guenter,
On 7/1/24 5:26 PM, Guenter Roeck wrote:
Quentin,
On 7/1/24 04:23, Quentin Schulz wrote:
Hi Guenter,
On 6/28/24 5:13 PM, Guenter Roeck wrote:
After setting fan1_target and setting pwm1_enable to 4,
the fan controller tries to achieve the requested fan speed.
There's something in the docs (section `Software-RPM Control Mode (Fan Speed Regulator`) that rubs me the wrong way though.
"""
When the TACH-MODE bit (bit 1 of
0x02) is cleared ('0'), the duty cycle of PWM-Out is forced to 30% when the calculated desired value of duty
cycle is less than 30%. Therefore, the TACH setting must be not greater than the value corresponding to the
RPM for 30% duty cycle.
"""
It turns out that the tach-mode bit is in reality the DC vs. pwm selector,
and defaults to DC. For pwm fans (4-bit fans), the bit should be set to 1.
That means that pwm1_mode should be supported to set the mode. I'll add a patch
for that.
TACH-MODE is never modified in the driver, so its default value prevails: 0.
I'm wondering if there isn't something we need to do to make sure we're not under those 30% for TACH-Low-Limit/TACH-High-Limit/TACH-SETTING? Forbid the user to write (or clamp instead) <30% duty cycle. Forbid the user to select mode 4 if current values are <30% duty cycle, or update them to be >=30%?
It also says that the "the selected target speed must not be too low
to operate the fan", which makes sense. It also says that the requested
fan speed should not be below the speed translating to 30% duty cycle.
However, that is not a fixed value; it depends on the fan. Some fans may
operate at 500 rpm with a duty cycle of 30%, others at 3,000 rpm.
Looking at Figure 26, I don't think the value written into the pwm
register makes any difference in Software-RPM control mode.
With that in mind, the only thing we could do is to ensure that the
requested fan speed is within the configured low and high limits,
or in other words require the user to set the limits before writing
the target fan speed. That is a bit circular, though - the user
could still write the target speed and _then_ update the limits
to a value outside the requested limit. The best we could do would be
to sanitize settings when the mode is set to 4 and any of the limits
is changed, and return an error if an obviously wrong limit or target
speed is requested (target speed outside limit, or low limit >= high
limit). Do you think that would be worth the effort ?
It depends how far we want to go to prevent the user shooting themself in the foot. I think the kernel's stance on that is "let them"?
The "benefit" of forcing the user to enter a value in a user-modifiable range is that they wouldn't unknowingly trigger a too-low or too-high logic within the IC.
As an example, my bank has a limit on how much I can pay by card per day. However, I can instantly change the value through an app and retry the payment again right after if it's been refused.
Would that be something interesting for this speed limit.... who knows.
Another thing we could do is modify the min and max values if they are higher and lower than the requested speed. But this is trying to be smart, which I think isn't something the kernel is aiming for (as little logic/algorithm as possible)?
So... I guess, the answer is "no, not worth the effort"?
I'll go with "not worth the effort".
Thanks a lot for the feedback!
Guenter
