rafael.espindola at gmail.com
Sat Feb 26 16:19:22 CET 2005
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".
> 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; /* Decoded values */
> u8 in_ext; /* 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.
Thanks for the comments.
The resulting patch is attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 7525 bytes
Desc: not available
More information about the lm-sensors