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

[Guenter Roeck <linux@roeck-us.net>]

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