stefan at desire.ch
Wed Jan 5 06:48:20 CET 2005
Thanks a lot for the review - I managed to clean most of the issues you mentioned. Some of them were just copied from other modules, others were my own :)
> > static ssize_t show_revision(struct device *dev, char *buf)
> Is there a real use for this? You could instead simply dev_info() it
> at module load time.
Well, the information probably isn't too relevant, but is there a reason not to allow access to it? Plus, if I remove it, "sensors -u" complains about "ERROR: Can't get feature `rev' data!"
> > static ssize_t set_temp_status
> > static ssize_t set_fan_status
> Hmm, isn't it odd that a "status" file can be written to? What are the
> two bits for?
It allows you to clear the alert state. I noticed this doesn't comply with the sysfs guidelines which say alarms are read-only but it's quite handy after doing stupid things such as stopping a fan altogether (which causes an alert state for said fan and "sensors" won't show any more fan data until the state has been cleared).
> > static ssize_t set_fan_min
> [..] shouldn't the value be divided by 60?
Indeed, I fixed that. I do however still seem to get it wrong somehow: When I double the value, the fan appears to accelerate by 8*60 RPM. It might somehow be related to the ripple value but I just don't get it.
> Don't mask, check the value for validity instead.
For some registers (mainly watchdog_control) it's quite tricky to figure out all valid values (especially as messing with them might cause unwanted reactions such as spontaneous rebooting)...
> > static ssize_t show_event
> > static ssize_t show_control
> What are these? Do you have a use for them?
I don't *REALLY* know whether they're that useful: Event seems to report all kinds of alerts which could also be gathered from other sources, but offers a single place to see whether everything is OK. The control register reports whether the alert LED is flashing. I haven't seen that LED (yet), nor would I know how to make it flash, but I thought I'll just implement access to all the information available. And, as above, "sensors -u" complains if I remove them.
> Rest look fine to me! Please correct whatever needs be, then you send
> your work to Greg KH in the form of a patch against the most recent
> 2.6 kernel. The patch must include updated to Makefile and Kconfig.
> Don't forget to CC the sensors mailing-list.
Okay, I'll do that as soon as we cleared the remaining issues.
> Good work! :)
> If you have fixes for the 2.4 driver, please send a patch to the
> sensors mailing-list.
> If libsensors needs to be updated to support your new driver, a patch
> will be welcome as well.
I tried to stick with the interface provided by the old driver so I don't think libsensors will need to be updated.
Thanks & regards
"Give a man a fire and he's warm for a day, but set fire to him and he's
warm for the rest of his life." -- Terry Pratchett, Jingo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: not available
More information about the lm-sensors