[lm-sensors] patch: asc7621 driver bug fixes

[George Joseph]

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: [lm-sensors] 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-0001.obj