On Sat, Jul 07, 2007 at 10:21:57AM +0200, Hans de Goede wrote: > Hi, > > Below is a review of your driver. Thanks for taking the time to review it. In general I've found the points you make are pretty good and have implemented them. That said, there were a few that I have some questions about, so I'll skip down to those. I intend to post a revised patch shortly, and review your driver after that. > Why all the base defines? Why not put the base address directly in the () > macros? You do not seem to use them anywhere else. I prefer to put all the register and bit-field definitions in one place so that later I can construct a map of all known registers after I've forgotten which ones the driver talks to and which ones it doesn't. The accessor macros that come after that are merely sugar, which is why I don't like encoding the base register address directly into the macro. > Are you sure you only want to support revision 2? What are the differences? > Shouldn't the driver be able to support other revisions too? Maybe only > print a warning when its used with an untested revision? I don't have a data sheet for other revisions, so the safest thing to do is to reject that which we don't know about; an interested user can always bypass those safety checks with force_adt7470= at a later point in time and post a patch if the driver still works ok. > Why use read_byte twice in read16, but write_word in write16? A "16 bit" register is really two 8 bit register reads/writes, and the register with the lower address has to be read/written first. Hence the weird helper functions. > >+ /* delay is 200ms * number of tmp05 sensors */ > >+ udelay(TEMP_COLLECTION_TIME); > > ugh, sleep here please. Would msleep_interruptible() work here? Some informal testing shows that it seems to work ok, though I've not subjected it to a rigorous ^C test to see if I can hit weird crash conditions. > This may be a stupid question (I haven't fully read the datasheet) but why > no store functions for fan_min and fan_max? Silly thinko on my part; the v2 patch will have those store functions. > Maybe just but all the sensor_attr directly into an array and use a for > loop on that array to register / unregister instead of sysfs attribute > groups? I implemented that, using your f71882fg driver as an example. --D -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 189 bytes Desc: Digital signature Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070710/84857dd1/attachment.bin
