Jean Delvare wrote:
> Hi Aurelien,
>
>
>>Here is a new version of the patch. As some of the changes you suggested
>>also apply to the lm78 driver, you will also find a patch for it.
>
>
> That's great. Comments about your lm78 patch:
>
>
>>@@ -83,7 +83,6 @@
>> {
>> if (rpm == 0)
>> return 255;
>>- rpm = SENSORS_LIMIT(rpm, 1, 1000000);
>> return SENSORS_LIMIT((1350000 + rpm * div / 2) / (rpm * div), 1, 254);
>> }
>
> I don't think it is sufficient, as negative rpm values will now return
> undetermined values. Replacing the "== 0" test with "<= 0" should
> fix that, right?
Actually FAN_TO_REG is always call with a positive value (eg through
strtoul if read from the user), so I though it is ok. However it's
better to do the test again there as the driver could evolve. Changed.
>
>> {
>> int nval = SENSORS_LIMIT(val, -128000, 127000) ;
>>- return nval<0 ? (nval-500)/1000+0x100 : (nval+500)/1000;
>>+ return nval<0 ? -(nval-500)/1000 : (nval+500)/1000;
>> }
>
>
> Hm, why did you add a "-" here? Looks fine without it (and broken with
> it).
You're right. Changed.
>
>>- if (!request_region(address, LM78_EXTENT, "lm78")) {
>>+ if (!request_region(address, LM78_EXTENT, lm78_driver.name)) {
>
>
> This one is already in 2.6.11-rc2, so don't do it or it'll collide.
Ok, removed.
> About sis5595, I missed a detail:
>
>
>>+ ERROR2:
>>+ kfree(data);
>>+ ERROR1:
>>+ release_region(address, SIS5595_EXTENT);
>>+ ERROR0:
>>+ return err;
>
>
> labels should be left-aligned. Also I think that labels named
> "exit_free", "exit_release" and "exit" would be prefered.
Fixed.
> And of course my comments on the lm78 patch apply to sis5595 also. Other
> than that I'm fine with your code, feel free to send it to Greg once it
> is generated against and tested with 2.6.11-rc2-mm1 (or wherever the
> latest i2c subsystem is when you do it).
Ok. I am planning to do that this evening. Should I also send a patch to
Grer for the lm78? Anyway, I'll show you my latest patches on IRC this
evening.
Bye,
Aurelien
--
.''`. Aurelien Jarno GPG: 1024D/F1BCDB73
: :' : Debian GNU/Linux developer | Electrical Engineer
`. `' aurel32 at debian.org | aurelien at aurel32.net
`- people.debian.org/~aurel32 | www.aurel32.net
