Re: [PATCH v2 1/2] i8k: Autodetect maximal fan speed and fan RPM multiplier

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

On 12/20/2014 04:18 AM, Pali Rohár wrote:
> On Saturday 20 December 2014 13:04:03 Guenter Roeck wrote:
>> On 12/20/2014 12:57 AM, Pali Rohár wrote:
>>> On Friday 19 December 2014 20:28:08 Guenter Roeck wrote:
>>>> On Fri, Dec 19, 2014 at 07:51:25PM +0100, Pali Rohár wrote:
>>>>> On Friday 19 December 2014 19:32:37 Guenter Roeck wrote:
>>>>>>> -static int i8k_fan_mult;
>>>>>>> -static int i8k_pwm_mult;
>>>>>>> -static int i8k_fan_max = I8K_FAN_HIGH;
>>>>>>> +static int i8k_fan_mult[2];
>>>>>>> +static int i8k_pwm_mult[2];
>>>>>>> +static int i8k_fan_max[2];
>>>>>>
>>>>>> The rationale for this change is not explained in the
>>>>>> commit log.
>>>>>>
>>>>>> Do you have any indication that those values would ever
>>>>>> be different for the two fans, ie that you actually need
>>>>>> arrays here ?
>>>>>
>>>>> I do not know... But if we decide to use only single value
>>>>> for multiplier and max value which fan to use for
>>>>> autodetection?
>>>>
>>>> That does not answer my question. That you can not decide
>>>> which fan to use for auto-detection does not mean that the
>>>> result of that auto-detection would be different for
>>>> different fans.
>>>
>>> Really I do not know if some dell products which have more
>>> fans (some Precision models have 2) and each fan is using
>>> different multiplier or has different max speed value.
>>
>> "I do not know" is not a reason for introducing such code.
>>
>> Guenter
>
> And having broken fan rpm output in userspace is not good.
>

Sure, but only if it is in fact broken.

> My code introduce fan rpm detection for each fan. I do not see
> any problem for doing this detection per fan. You suggest to do
> some global detection and use it for each fan. And for your
> suggestion I have 2 objections:
>
The problem is that there is no evidence that a per-fan multiplier
is needed. You may not see it that way, but unnecessary code _is_
a problem since it increases code size for no good reason. Furthermore,
people not involved in this discussion will be inclined to believe
that the code is necessary, in the wrong assumption that it would
not have been introduced unless it was necessary.

> 1) I do not know if global multiplier will work for all fans.
> Detection per fan does not have this problem "I do not know".
>
It has been working all along, with no evidence that it doesn't work.

> 2) How to do that global multiplier detection? I have no idea how
> you want to do that.
>
A single multiplier worked all along. It doesn't matter which fan is
used to auto-detect the multiplier, it is still a single multiplier.

Again, introducing code if you don't know that or if it may be
needed is not an argument for introducing such code. Introduce it
if and when there is evidence that it is needed.

Thanks,
Guenter