> 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.pl
