fscpos driver

Stefan Ott stefan at desire.ch
Wed Jan 5 06:48:20 CET 2005


Hi Jean

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! :)

Thanks!

> 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
-- 
Stefan Ott
http://www.desire.ch

"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
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050105/cac92724/attachment.sig>


More information about the lm-sensors mailing list