[lm-sensors] patch: asc7621 driver bug fixes
ken at kenm.demon.co.uk
Sun Jul 6 18:49:27 CEST 2008
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
* 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
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
Hans de Goede wrote:
> Ken Milmore wrote:
>> Here are some suggested bug fixes for the asc7621 driver source which
>> you posted to lm_sensors on 29 May.
> 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?
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
More information about the lm-sensors