Hans / George, Although things are shaping up nicely, I think there is still quite a bit left to do to completely review the driver. The aSC7621 appears to have a lot of bells and whistles for closed loop temperature control etc. and George has tried to support as much of this feature set as possible. Unfortunately that means there's quite a lot to review - I haven't really looked at PWM control yet. In any case, a few more things have come to light: 1. In my last posting I wasn't completely sure if my "patch #2" was a good idea or not. Having looked at the Andigilog specs, I have become convinced that it will be necessary after all. The main problem is with the two-byte registers. To get the read-locking to work, the MSB and LSB must be read together, one after the other (although it is unimportant which one is read first!). So it is necessary to sequence the reads so that the two-byte registers are handled correctly. My patch will correct this problem, although as I have said, I think that other, cleaner solutions may be possible. 2. The voltage scaling used in the driver appears to be slightly off from the nominal values given in the spec. To get the scaling exactly right, it is necessary to use a "muldiv" scheme which I've included in the patch (see below). 3. I fixed a problem in store_fan16() whereby it wasn't possible to set the minimum fan RPM to zero. The aSC7621 explicitly allows this, and of course it is useful to prevent spurious alarms for fans which are missing or stopped. The attached patch rolls up all my changes so far. It applies against George's original patch of 29 May. It includes all the bug fixes from my last submission, the voltage scaling and fan fixes mentioned above and some tidying up of the high and low priority register lists. To sum up, by now I'm very happy with the following functions of the driver: * Reading of voltages, fans and temperatures. * Setting of alarm limits and display of alarms. The following areas still need to be reviewed and perhaps improved: * PWM control. A few scary lookup tables in the code that I don't understand yet! * The miscellaneous configuration knobs (Input selection, PECI config etc). I haven't tested these. * I think there is space for improvement in how the low-priority register list is handled. A full minute between updates is probably too long. I hope this is of some use. I will try to look at reviewing the remaining parts of the driver as time allows. George, I'd appreciate your feedback on my patches so far; I realise you might want to approach some of these changes differently to the way I've done them. Best wishes, Ken. Hans de Goede wrote: > Ken Milmore wrote: >> George, >> >> Here are some suggested bug fixes for the asc7621 driver source which >> you posted to lm_sensors on 29 May. >> (http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023257.html) >> > > Thanks for doing this, I promised George to review this driver, but > sofar haven't gotten around to doing this. Any chance you could do a > complete review of it (or have you already done so, it sure looks that > way :) > > George, I assume you will post a new version of your patch soon with > these fixes incorperated? > > Regards, > > Hans > -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: asc7621.diff Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20080706/78362fb9/attachment.pl
