Re: [PATCH 5/5] Export the temperatures via hwmon

[Goffredo Baroncelli <kreijack@inwind.it>]

On 08/07/2014 11:19 PM, Matt Helsley wrote:
> On Thu, Aug 07, 2014 at 07:50:00PM +0200, Goffredo Baroncelli wrote:
>> On 08/07/2014 09:36 AM, Guenter Roeck wrote:
>>> On 08/06/2014 11:52 PM, Jean Delvare wrote:
>>>> Hi Guenter,
>>>>
>>>> On Wed, 06 Aug 2014 23:20:32 -0700, Guenter Roeck wrote:
>>>>> Patch 4/5 is "Return the fan speed via sysfs: /sys/devices/temperature/fan_level".
>>>>>
>>>>> So you are saying that returning the fan speed with a non-hwmon attribute works,
>>>>> but returning it with a hwmon attribute doesn't ? Not really sure if I understand
>>>>> your logic. Either fan_level doesn't return the fan speed (or an abstraction of it),
>>>>> or something in your line of argument is inconsistent.
>>>>
>>>> fan_level is a fan speed _control_ value, like pwm1. It is not a fan
>>>> speed monitoring value.
>>>>
>>> Ah, ok. The patch description doesn't seem to match, though.
>>> And why not export it as pwm1, if that is what it is ?
>>>
>>> Guenter
>>>
>>>
>>
>> the exported fan_level value is a coefficient near proportional to the speed [*];
>> so it is not the speed nor the pwm.
> 
>> I tried to read the pwm/speed value, but when I did it, every 5/6 seconds the
>> fan seemed to stop for 1s, then the speed raised.... So I stopped the test.
>>
>> These patches (the first two) solved a real issue: with the last kernels this
>> driver doesn't work at all, and the fan go to maximum speed (very loud !)
>> The other three are an improvement.
>>
>> When (if) these patches will be accepted I want to write another solution,
>> but definitely not now. And even if it would work for me, it is very likely 
>> that will  be accepted because nobody is able to test it on all hardware.
>>
>> BR
>> G.Baroncelli
>>
>> [*] It is a bit more complicated: basically the adm1030 controls the fan speed
>> on the basis of an external temperature sensor (the "case" sensor). 
>> Higher is this temperature higher is the fan speed.
>> The pwm of the fan is computed on the basis of the following value:
>> 	- min_temp
>> 	- range_temp -> max_temp=min_temp+range_temp
>> 	- min_pwm_value
>> and of course
>> 	- "case" sensor
>>
>>
>> The fan_level is an index computed on the basis of the "cpu" temperature (
>> which is different from the "case" temperature referred above). This
>> temperature is used to update the min_temp above.
>>
>> So the fan speed id computed on the basis of two external sensor:
>> - cpu sensor
>> - case sensor.
>>
>> To complicate further the things, the range_temp is set to an undocumented 
>> value: this parameter is set via a register (0x25, bit 2:0), which accepted 
>> values between 0x00 and 0x04. However it is set to 0x07
>>
>> See http://lxr.free-electrons.com/source/drivers/macintosh/therm_windtunnel.c#L153
>> and the adm1030 datasheet.
>>
>> I am reluctant to change this thing because this driver is for an obsolete 
>> platform (apple stopped the powermac in 2004/2005), and bad or good I have to
>> suppose that it works. Moreover I am not in the position to test the changes 
>> outside my hardware (1 powermac DP@1Ghz mdd)
> 
> This seems like a valuable explanation of the meaning of the fan_level
> values. When I read the code I was wondering what the magic number "11" was
> doing for instance and this helps clear it up a little. It's a nit perhaps
> but some of this information might be useful in the commit message for that
> patch, the code, or both.

I will take in account your suggestion. In case of another revision, I will improve the comment.



> Also, I think the patches from "Bryan" should have a "From:" attribution line
> at the start of the commit message (See Documentation/SubmittingPatches).

Thanks for the suggestion. In case of another revision I will put the "From:" tag.

> Cheers,
> 	-Matt Helsley
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli (kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5