> * libsensors is reading from 'in0' instead of 'in_input0'
> - I have edited chips.c and attached the patch
Stupid me. Applied.
> * libsensors is reading from 'temp_input1' but gl518sm provides
> 'temp_input'
> - I have edited the driver to use 'temp_*' instead.
I guess you mean 'temp_*1'.
> - however, it might be confusing to read from 'temp_input1', but
> specifying it as 'temp_{max,hyst}' in the config files. It might
> be better to append '1' for all 'temp*'
This would require changes to the library and default configuration
file. Not worth it IMHO. I don't even find it confusing the way it is.
People are not supposed to access sysfs files directly anyway.
> I don't think I will be able to test the 2.4 drivers for a few weeks.
> I will be going overseas for 3 weeks and being a server, it takes a
> bit of effort to bring it down for a kernel change. I am happy to do
> it when I get back though. I am still able to work on the driver but
> using the current kernel.
No problem. The more important is that we get a good driver into Linux
2.6. The chances we break compatibility with the older driver are low,
and if this happens and someone notices it, let him/her speak up.
> I have also updated gl518sm.c:
> - I have found a bug due to me misreading the old code. It affects
> only fan_div*.
Don't you mean fan_min*?
> - I am thinking of replacing '(val >> 4) & 0x0f' with '(val & 0xf0) >> 4'
> as it allows easy checking with the chip specifications (the mask in
> particular). I have changed a few, and if there are no objections,
> I will be changing the rest.
Frankly, I don't see no benefit. I don't find it more nor less readable
one way or one other - so I wouldn't bother changing, especially since
it is likely to introduce errors.
> - I have added fan_auto1 that switches fan1 between [true] always on and
> [false] auto on(30C)/off(25C). This was removed from the port as it
> did not a standard item, and I didn't know what to do with it.
This is fine. There's nothing wrong with not being standard. What's
important is to respect what is defined in the standard. The rest is
more or less free as long as it fits in the spirit. If new things would
need to be standardized, we'll extend the standard.
I've made the necessary change to libsensors so that it knows about your
fan_auto1. That way, "sensors -s" shouldn't complain about it anymore.
A few comments will need to be added to sensors.conf.eg to explain the
differences between the 2.4 and 2.6 drivers, and how the auto fan thing
works. I will do that right after the new driver is finished and
submitted.
> - I have a question regarding the init of the driver. Is there a need
> to init the chip? The BIOS should have initialised the chip, and we
> only need to make sure it is enabled. Initialising the chip resets
> the config such as the min/max which the BIOS should have set, as
> well as other hardware dependent options such as 'fan2off' in the
> old driver. My understanding is that we should not be able to write
> 'fan2off' as it depends on the hardware implementation. So I am
> inclined not to reset the chip on init of the driver.
I fully agree with you here. I am fighting against systematic chip
resets since the day I wrote my first chip driver.
Now, a few more comments about your patch:
> @@ -280,19 +280,21 @@
>
> data->fan_min[1] = FAN_TO_REG(simple_strtoul(buf, NULL, 10),
> DIV_FROM_REG(data->fan_div[0]));
^
Isn't it a bug?
> val = gl518_read_value(client, GL518_REG_CONF);
> - data->beep_enable = (val >> 2) & 1;
> + data->beep_enable = (val & 0x04) >> 2;
> + data->fan_auto1 |= (val & 0x10) >> 3;
What's the point of the last line? For one thing, it seems to be bogus
(the mask doesn't match the shift). For another, I don't really see what
it is supposed to do.
I applied everything from your patch but this line and the shift/mask
permutations, and fixed what I suspected to be a bug. Full patch
attached.
I leave it to you to rework the init function. Remove the chip reset and
any other change that is done and should not. Especially the NoFan2 bit
of the configuration register should be left untouched IMHO.
--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: linux-2.6.2-rc2-i2c-gl518sm-rc3.diff
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040131/e0459af3/attachment.pl
