Bug 11520 - eeepc_laptop hwmon missing name
eeepc_laptop hwmon missing name
Status: CLOSED CODE_FIX
Product: Drivers
Classification: Unclassified
Component: Hardware Monitoring
All Linux
: P1 normal
Assigned To: Corentin Chary
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-08 04:58 UTC by Florian Heimgaertner
Modified: 2008-10-10 00:41 UTC (History)
1 user (show)

See Also:
Kernel Version: 2.6.26
Tree: Mainline
Regression: ---


Attachments
eeepc-laptop: add the name attr (775 bytes, patch)
2008-09-23 00:52 UTC, Corentin Chary
Details | Diff
Add the right name attr (777 bytes, patch)
2008-09-23 02:46 UTC, Corentin Chary
Details | Diff
Add the right name attr (935 bytes, patch)
2008-09-23 08:26 UTC, Corentin Chary
Details | Diff
Fix the hwmon interface (1.90 KB, patch)
2008-10-03 06:54 UTC, Corentin Chary
Details | Diff

Description Florian Heimgaertner 2008-09-08 04:58:04 UTC
Hardware Environment: EeePC
Problem Description:

The hwmon sysfs-interface of the eeepc_laptop module doesn't provide the mandatory attribute "name", causing lm-sensors to ignore it.
Comment 1 Jean Delvare 2008-09-08 06:28:58 UTC
Corentin, please provide a patch for this bug.
Comment 2 Corentin Chary 2008-09-08 06:39:27 UTC
There was a patch on the ml, but it was broken. I'll resend a new one soon.
Comment 3 Corentin Chary 2008-09-23 00:52:45 UTC
Created attachment 17968 [details]
eeepc-laptop: add the name attr

Here is a patch, sory for the time it took me to send 3 lignes of code.
Comment 4 Jean Delvare 2008-09-23 01:25:44 UTC
"fan" is way too generic as a hwmon device name. You'll have to come up with something better than that. libsensors uses the name attribute to differentiate between hardware monitoring device types. "eeepc" would do, I guess. Note that dashes are prohibited.

Florian, any chance you can test Corentin's patch and report? In case other things need to be fixed in the eeepc-laptop driver.
Comment 5 Corentin Chary 2008-09-23 02:46:29 UTC
Created attachment 17969 [details]
Add the right name attr

Here is a new patch ...
Comment 6 Jean Delvare 2008-09-23 07:32:32 UTC
Looks good, thank you. Assuming that I can add your Signed-off-by to this patch, I'll push it upstream.
Comment 7 Jean Delvare 2008-09-23 08:03:20 UTC
Hmmm, you didn't even try building the code?

drivers/misc/eeepc-laptop.c:533:57: error: macro "SENSOR_DEVICE_ATTR" requires 5 arguments, but only 4 given
drivers/misc/eeepc-laptop.c:533: warning: type defaults to 'int' in declaration of 'SENSOR_DEVICE_ATTR'
drivers/misc/eeepc-laptop.c:539: error: 'sensor_dev_attr_name' undeclared here (not in a function)

Please provide a patch that builds properly, with your Signed-off-by.
Comment 8 Corentin Chary 2008-09-23 08:26:23 UTC
Created attachment 17972 [details]
Add the right name attr

Here is the right patch ... again.
Sorry for the last, I tried to build asus-laptop instead -_-.
Hop this is all ok now. Anyway, it build.
Comment 9 Florian Heimgaertner 2008-09-24 12:24:07 UTC
(In reply to comment #8)
> Created an attachment (id=17972) [details]
> Add the right name attr
> 
> Here is the right patch ... again.

Ok, tested with 2.6.26.4. 
lm-sensors now shows the fan speed.
pwmconfig/fancontrol still don't work, they apparently don't like "virtual" sensors...
Comment 10 Jean Delvare 2008-09-24 13:28:14 UTC
pwmconfig and fancontrol do not care about virtual vs. real sensors. All they care about is that the driver respects the standard sysfs interface for hwmon drivers. Which apparently the eeepc-laptop doesn't... apparently it uses values 0-100 for PWM duty cycles, where the standard interface says 0-255 should be used. So scaling is missing in the driver.

Note that fancontrol requires a temperature value so that it can compute the target fan speed. Apparently the eeepc-laptop module doesn't provide a temperature value, so unless there is another thermal sensor driver on the machine, it is expected that pwmconfig and fancontrol won't work.
Comment 11 Florian Heimgaertner 2008-09-25 02:30:29 UTC
pwmconfig is looking at /sys/class/hwmon/hwmon*/device, which doesn't exist for eeepc-laptop.
btw: according to a comment in pwmconfig fan[1-9]_pwm is deprecated, so fan1_pwm should be renamed to pwm1
Comment 12 Jean Delvare 2008-09-27 08:39:32 UTC
(In reply to comment #11)
> pwmconfig is looking at /sys/class/hwmon/hwmon*/device, which doesn't exist
> for eeepc-laptop.

Ah, you're correct. This problem has been addressed in libsensors some times ago already, but not in pwmconfig. I've just fixed that:
http://www.lm-sensors.org/browser/lm-sensors/branches/lm-sensors-3.0.0/prog/pwm/pwmconfig?rev=5343&format=raw

Can you please give it a try?

> btw: according to a comment in pwmconfig fan[1-9]_pwm is deprecated, so
> fan1_pwm should be renamed to pwm1

You're totally right. Corentin, can you please attach a patch fixing both the scaling and the pwm file name? Thanks.
Comment 13 Corentin Chary 2008-09-27 09:34:40 UTC
No problem, there will be a patch monday.
Comment 14 Corentin Chary 2008-09-29 00:54:50 UTC
I don't know why there is no "device" file.
Should'nt hwmon_device_register(dev) do that ?
Comment 15 Jean Delvare 2008-09-29 03:11:57 UTC
hwmon_device_register(dev) should indeed create the device link. Maybe you should check the value returned by acpi_get_physical_device(ehotk->device->handle).

It doesn't really matter though, as the attributes we are interested in are those of the hwmon device.
Comment 16 Corentin Chary 2008-09-30 12:21:08 UTC
I'm currently reinstalling my eeepc as my external hd died.
I'll do some test, then send a patch.
Comment 17 Corentin Chary 2008-10-03 06:54:43 UTC
Created attachment 18144 [details]
Fix the hwmon interface

Here is another patch.

There is still no "device" link, but it's not needed anymore. Now lm-sensors support "virtual" sensors.
http://www.lm-sensors.org/ticket/2309
http://www.lm-sensors.org/changeset/5176
Comment 18 Jean Delvare 2008-10-03 07:57:04 UTC
Patch looks good, thanks. I've added it to my hwmon tree, and will be sending it to Linus shortly.
Comment 19 Jean Delvare 2008-10-03 08:33:21 UTC
Florian, can you please test the patch and confirm that it works fine for you too?
Comment 20 Jean Delvare 2008-10-10 00:07:32 UTC
Fix is in kernel 2.6.27:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=04dcd84bc79d9f756bf5b9fc16c7df3344823ca8

Corentin, you can close this bug.

Note You need to log in before you can comment on or make changes to this bug.