Re: [PATCH 04/10] hwmon: (amc6821) Add support for fan1_target and pwm1_enable mode 4

[Quentin Schulz <quentin.schulz@cherry.de>]

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"?

Cheers,
Quentin