emc6d102 support

[Rafael Espíndola]

On Thu, 24 Feb 2005 01:08:57 -0800, Philip Pokorny
<ppokorny at penguincomputing.com> wrote:
> I have some comments...
 
> Why did you remove the definition of EXTEND_ADC2?  It's not used by
> name, but it's there to document the registers that are used by the driver.
ok. I added it back.

> Hmmm..   The EMC6D102 datasheet shows these registers (0x70 - 0x78) as
> "test registers" that are "reserved".
> 
> http://www.smsc.com/main/datasheets/6d102.pdf
> 
> We should add addtional code to prevent reading or setting the
> additional in5, in6 and in7 inputs if the chip is an emc6d102.
It is already there. These registers are read only if (data->type == emc6d100)

> I think I'd leave those on one line so a 'grep' for the function would
> pick out the definition as well as the function name.  But that's just
> me.  Perhaps there is some applicable style guideline?
Those macro definitions had more then 80 columns.

> Given the additional complexity introduced by the possibility of
> different scaling factors (4 for adm1027, 16 for EMC6D102) I think I
> would change this from a u16 to a *decoded* value in two arrays:
> 
>     u8  temp_ext[3];             /* Decoded values */
>     u8  in_ext[8];               /* Decoded values */
>     u8  adc_scale;               /* ADC Extended bits scaling factor */
> 
> Then put all the shift and mask code in 'lm85_update_device' and have it
> read the values and break them up into the appropriate '_ext[]' values.
> That's a simple loop for the adm1027 where the bits are in the same
> order as the sensors.  It's a bit more complicated in the case of the
> EMC6D102 because things are moved around a bit, but I think it would
> make more sense for that code to be in '_update_device'.
> 
> Then set adc_scale appropriately at initial detection time.
Did it. The resulting code is indeed simpler.

> for example.  It might even make sense to just pass 'data' to the
> '???EXT_FROM_REG()' macro and let it dereference the structure pointer...
I fill a bit uncomfortable with big macros...

> I don't see any comment about the time ordering of reading the LSB's and
> the MSB's.  The data sheet says that reading the upper 8-bit MSB's latch
> the corresponding lower 4 bit LSB's.  You'll note that this is
> *backward* from the ADM1027 where the LSB need to be read *first* to
> latch the MSB's.
> 
> A note regarding this difference should be included in comments for the
> code to read the EMC6D102 extended bits.
Added one. 

Thanks for the comments.
The resulting patch is attached.
Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: emc6d102-kernel.patch
Type: application/octet-stream
Size: 7525 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050226/89c1c04b/attachment.obj