Latest version attached... Signed-off-by: George Joseph <George.joseph at fairview5.com> Comments below... > -----Original Message----- > From: Ken Milmore [mailto:ken at kenm.demon.co.uk] > Sent: Tuesday, July 08, 2008 5:24 PM > To: George Joseph > Cc: Hans de Goede; lm-sensors at lm-sensors.org > Subject: Re: patch: asc7621 driver bug fixes > > > George Joseph wrote: > > We still need to read it at least once because it has the pwm auto zone > > assignments I let the first default 0 in the msb or lsb arrays take care > > of it. If you dump the array after it's loaded you'll see 0x20, 0x13 > > (in0) then 0x00 (because it's the default of the unused msb/lsb) then > > 0x21, 0x18 (in1), etc. That's the only place 0x00 appears. > > > > Am I missing something? > > No, you're not missing anything. :-) I understand now, but see below. > > > When I started the driver last year, I cloned one of the LM* drivers and > > it worked just fine for the basic stuff just by modifying the chip and > > manfacturer id. Then I thought if I'm going to do a driver for this > > thing, I probably should expose as much of the chip's functionality as I > > could. With 98 registers to read and 160+ sysfs entries to create > > though, ?I quickly realized it would be impossible to code and maintain > > the driver using the existing paradigms. Hence the table driven > > approach. The details of the registers and sysfs entries are all in one > > easy-to-read table. > > It isn't your big table of constants in the code (asc7621_params) that I > have the problem with, it is the need to reverse-index this table at > runtime which I am calling into question. > > I can see why you want to do it this way. You have a many-to-many > mapping between device registers and sysfs entries, which is expressed > in the main table. You could just walk down that table and read the > device registers as required, but this would mean reading some registers > more than once. Indeed, some of the flags registers would be read many > times. And each read over I2C is *very* time-costly. So you need > separate lists of all the registers to be read, with duplicates removed > so that each is only read once. No controversy there. > > What I'm having trouble with is why these lists (of low and high > priority registers) need to be generated at runtime. It would certainly > be nice and elegant if this information could be obtained directly from > the main asc7621_params table, without iterative re-indexing, but I > can't think of a way to do it other than by re-designing the driver. > That's why I suggested simply hard-coding the lists. > > I realise you may think that this would wreck the maintainability of > your main table, but its not as bad as it sounds! It is if I'm the one maintaining it. :) > After all, from the > table I failed to notice where register zero appears in the read lists; > it is added as a kind of "side effect". That ambiguity wouldn't have > arisen if the lists had been in front of me in the source! And you get > to save on half a kilobyte of unnecessary array space into the bargain... > > Anyway, if I have failed to dissuade you, and you still really, really > want to persist with the dynamic list creation, then you might want to > look at the following further points: I really, really do. > > 1. Declaring a u8[256] as an automatic array on the rather small kernel > stack is probably a bad idea. > > 2. C doesn't initialise automatic data to zero so you will have to > memset the asc7621_register_priorities array before use. (I suspect you > have been lucky here with the specificity of your "!= 1" check on the > array contents!) > > 3. You could save a bit of space by putting both the high and low > priority lists into the same array: The size of both lists together > cannot exceed 256. This doesn't really matter, though. All taken care of. > > > I think at this point the approach will have to stand unless it's going > > to prevent acceptance of the driver. If it is going to be an issue I > > think I'm just going to have to give up on it for a while. I only have > > limited windows of time to work on it. > > Fair enough. I don't have any say in the matter, nor would I want to. > I'm only expressing opinions, which you can take or leave. Open debate > applied to open source development. Thanks for listening! ;-) No problem. I appreciate the feedback! george > > Kind regards, > > Ken. -------------- next part -------------- A non-text attachment was scrubbed... Name: asc7621_3.patch Type: application/octet-stream Size: 54376 bytes Desc: asc7621_3.patch Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20080708/572129de/attachment.obj
