On Wed, 5 Jan 2005 09:41:20 +0100 (CET) "Jean Delvare" <khali at linux-fr.org> wrote: > > Hi Stefan, > > > 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 :) > > If this means that some other 2.4 or 2.6 drivers would need fixing, > just tell me where and I'll fix them. Most of the code was from the fscher driver, especially all the masking and the sysfs things we're currently discussing (like revision and control) and some badly placed parentheses. The register definitions and some minor issues were from the old fscpos driver. > > > > 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? > > It's not the way you want to look at things, or you will end up with a > driver way bigger than it could and should. We want to keep the > drivers to a minimum size, in order to save memory space and limit the > possible bugs and the maintainance effort. We should get rid of > whatever isn't explicitely needed. It can still be added later if > there is a real need. Okay, I put a dev_info in fscpos_detect and the sysfs file is gone. > > > > 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). > > The driver, not the user, should take care of this. When one is > reading from the status file, the driver should automatically rearm > any flag that needs be. Don't the 2.4 fscpos and the 2.6 fscher > drivers do it that way? No, they don't. So what you're saying is that once an alarm has been read it should disappear if the problem is no longer there? > > > > 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. > > I don't know either. Did you test the 2.4 driver in similar > conditions? No, I didn't. I guess I'll compile myself a 2.4 kernel later today and try, but I can't see any related code in there which does magical calculations my driver doesn't do. I'll try anyway. > It's odd that the min value would affect the speed, since it > supposedly is only a limit to the alarm condition, not a speed target. According to the PDF you sent me, this really is meant to actually control the fan speed. > > > 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)... > > For bit vector registers, masking might be fine. What is not OK > however is to mask a *range* with 0xff instead of simply checking that > the value is between 0 and 255 and return -EINVAL or similar if it > isn't. It's usually better to return an error to user-space than > silently drop an invalid value. Do the best you can. Okay, I will. > > > > 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. > > Any reason why control is writable? I find it strange that something > called control would only return a status. Okay, meanwhile I found the LED and it seems that the control register does not work as announced in the specs: It won't indicate whether the LED flashes (the event register does that) nor can you stop it (has to be done where the actual problem occurs, eg. fan2_state), so I removed it. > > And, as above, "sensors -u" complains if I remove them. > > Does "sensors" complain as well? Does "sensors" display anything for > these two files? If it doesn't, I guess it shows that these files are > not really important and could possibly be removed. No, sensors doesn't show anything, so should I just remove them? > > > 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. > > Sometimes it isn't enough. For libsensors/sensors to work out of the > box with a new 2.6 driver, it is required that the new driver follows > the standard AND the old driver had relatively standard feature names > as well. You of course cannot control that second condition. That > said, if you have good results already, then I guess that the feature > names were correct. Yes, it appears so. I guess the only changes required will be to remove references to the features i removed (like rev and control). 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/9b2f9498/attachment.bin
