gl518sm chip driver for 2.6 kernel

Jean Delvare khali at linux-fr.org
Wed Jan 28 23:16:58 CET 2004


> Yes, I did follow the guidelines as well as the sysfs-interface, but
> since its my first kernel module attempt, it might need improvement.

OK, I reviewed your code and you have done a great job. I can hardly
believe that this is your first contact with kernel code.

I have adjusted a few things in your original port. If we left apart
uninteresting things such as white space changes, indentation and the
such (which BTW weren't bad at all), here is a list of changes:

* Add standard debug stuff. This is something I need to add to the
porting-clients doc.

* Reorder includes.

* No function declaration for swap_bytes();

* Switch from gl518_list to gl518_id. This makes the driver more in line
with the others I've seen so far. The other method looks like something
leftover from older times. I think that this change could be backported
to the 2.4 driver as well, since new code is more simple and doesn't
imply any arbitrary limit to the number of chips supported. However, I'd
like you to confirm that the new code works.

* Looks like you forgot to create the sysfs file "beep_mask". I added
the required line. Please correct me if I'm mistaking.

* Remove one use of DEBUG_VIN, which seems to be bogus. It makes non-00
revision chips use revision 00 code if DEBUG_VIN is set. This doesn't
match the description of DEBUG_VIN, so I took the liberty to clear it.

Attached is a patch against Linux 2.6.2-rc2 which adds the new driver
and integrates it cleanly into the kernel tree. I'd like you to make
sure it works correctly for you before we add it to the official tree.

For reference, can you tell us wether your test system uses release 0x00
or 0x80 of the gl518sm? If 0x00, please try with DEBUG turned on too, to
make sure it still works.

Greg, I've reviewed the code and it looks good, however I'm not familiar
with threads, neither in Linux 2.4 nor in 2.6. Could you review that
part of the code (bottom of the file) and confirm that it's correct for
inclusion into Linux 2.6? The easier way to check is probably to read a
diff between the driver as it exists in our CVS repository and the
attached ported driver. One thing that looks strange to me is the
suppression of this line:
current->pgrp = 1;
This is in gl518_update_thread(). Might be correct however, I just don't
know bit about threads so I am wondering if it's correct to remove the
line. This is the first chip driver I see that deals with threads, so I
really need assistance in reviewing it.

Also, Greg, can you tell me wether my "inclusion" of DEBUG_VIN into
DEBUG is correct or not? (that's at the top of the new driver.)

After tests and code review are completed, we will have a new chip
driver ported to Linux 2.6. Yeah! :)

Thanks.

-- 
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.diff
URL: <http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040128/71ad3ce3/attachment.ksh>


More information about the lm-sensors mailing list