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! 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: 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. > 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! ;-) Kind regards, Ken.
