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:54 AM, Pali Rohár wrote:
> On Saturday 20 December 2014 13:44:59 Guenter Roeck wrote:
>> 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
>
> Ok, so what you suggest now? We have function which reports on
> supported models if speed level X is supported for fan Y and
> which RPM speed Z is expected for that level X. I have no problem
> to (re)write for fan multiplier detection and maximal fan speed
> level, but I need to know how to do that (which approach you
> accept and which not). What I want to have is correct data in
> userspace and better without hardcoded constants for each laptop
> in driver code.
>

Take your patch, run 's/\[fan\]//', see where it takes you.
Essentially assume that the auto-detected parameters for one fan
apply to all fans. I would think that it should be relatively safe
to assume that the first fan always exists, so you can start with
using that to detect parameters and see where it takes you.

Guenter