On Sun, Apr 03, 2011 at 08:39:40AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Tue, 22 Mar 2011 20:43:03 -0700, Guenter Roeck wrote:
> > This patch adds hardware monitoring support for Maxim MAX16065/MAX16066
> > flash-configurable system managers with nonvolatile fault registers.
> >
> > Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
> > ---
> > Documentation/hwmon/max16065 | 69 ++++
> > drivers/hwmon/Kconfig | 10 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/max16065.c | 757 ++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 837 insertions(+), 0 deletions(-)
> > create mode 100644 Documentation/hwmon/max16065
> > create mode 100644 drivers/hwmon/max16065.c
>
> Can I get a register dump of one of the supported chips for my
> collection?
>
Here you are, for MAX16065.
root@groeck-desktop:/sys/class/i2c-adapter/i2c-5# i2cdump -y 5 0x51
No size specified (using byte-data access)
0 1 2 3 4 5 6 7 8 9 a b c d e f 0123456789abcdef
00: 7d 40 65 40 50 00 3b c0 28 80 15 00 97 00 8e 80 }@e@P.;?(??.?.??
10: 6e 00 50 c0 36 00 1b c0 51 79 40 00 00 00 00 00 n.P?6.??Qy@.....
20: 00 0e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 .?..............
30: ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
40: 00 00 00 00 00 00 00 09 1a ff 18 1a ff 18 1a ff .......??.??.??.
50: 18 1a ff 18 1a ff 18 1a ff 18 1a ff 18 1a ff 18 ??.??.??.??.??.?
60: 1a ff 18 1a ff 18 1a ff 18 1a ff 18 00 00 00 00 ?.??.??.??.?....
70: 00 00 00 01 1e 00 00 cc cc cc cc cc cc cc 12 34 ...??..????????4
80: 56 78 9a bc 12 34 56 78 9a bc 00 00 02 01 00 00 Vx???4Vx??..??..
90: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XXXXXXXXXXXXXXXX
a0: XX XX XX XX XX 00 10 00 00 00 00 00 00 XX XX XX XXXXX.?......XXX
b0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XXXXXXXXXXXXXXXX
c0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XXXXXXXXXXXXXXXX
d0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XXXXXXXXXXXXXXXX
e0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XXXXXXXXXXXXXXXX
f0: XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XX XXXXXXXXXXXXXXXX
> Review:
>
I'll skip most of your feedback. Respective changes will be in the next version
of the driver.
[ ... ]
> > +can be safely used to identify the chip. You will have to instantiate
> > +the devices explicitly. When instantiating the device, you have to specify
> > +its configuration register address.
>
> "Configuration register address"?
>
Leftover from another driver, sorry.
[ ... ]
> > +
> > +curr1_input Current sense input; only if current sensing is enabled
> > + Displayed current assumes 0.001 Ohm current sense
> > + resistor.
> > +curr1_alarm Overcurrent alarm
>
> I'm a little worried about the "assumes 0.001 Ohm (...) resistor".
> Resistors which are external to the chip are normally handled by
> user-space. I understand this is a different case from scaling
> resistor pairs for voltage inputs, but it still feels wrong to assume an
> arbitrary resistor value in the driver. Where does the value come from,
> BTW? I couldn't find it in the datasheet.
>
I have to say the datasheet isn't really easy to read ;).
Only reason for assuming 0.001 Ohm is that makes adjustments via sensors.conf
easier than, say, 0.005 Ohm or 0.002 Ohm.
The ADC_TO_CURR() calculation is derived from information found in the datasheet,
which I confirmed with the current sense voltage readings on my test board.
> But I also have to admit that we do not have the needed code in place
> yet to handle it differently. This is similar to the problems described
> in:
> http://www.lm-sensors.org/ticket/2258
>
> I have another possible implementation idea, I'll post about it
> separately for public discussion.
>
Problem is that currents are always measured as voltages, and thus depend on
the series resistor value. I have been hitting the same problem with other
chips supporting current measurements. See the ltc4151 and ltc4261 drivers
for examples.
My solution so far is to assume a specific series resistor, and let userspace deal
with adjustments via sensors.conf.
Another option would might be to add platform data for each of the affected chips.
Would that make sense ? But even then I would need a default value in case there is
no platform data.
[ ... ]
>
> I couldn't find in the datasheet any guarantee that the MSB and the LSB
> belong to the same measurement, but I admit I didn't read it too
> carefully. Is this the case?
>
No, or at least I did not find it either. Turns out, however, that I can use
16 bit reads since the chip auto-increments the address. I'll do that instead.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
