Re: [PATCH] hwmon: Add support for max6695 and max6696 to lm90 driver

[Guenter Roeck <guenter.roeck@ericsson.com>]

Hi Jean,

On Wed, Sep 08, 2010 at 07:48:54AM -0400, Jean Delvare wrote:
> On Wed, 8 Sep 2010 03:28:16 -0700, Guenter Roeck wrote:
> > Hi Jean,
> > 
> > On Mon, Sep 06, 2010 at 12:12:29PM -0400, Jean Delvare wrote:
> > > Hi Guenter,
> > > 
> > > On Sat, 4 Sep 2010 15:34:35 -0700, Guenter Roeck wrote:
> > > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > > ---
> > > > To apply this patch, the previously submitted lm90 cleanup patch has to be
> > > > applied first.
> > > >
> > > > My main concern with this patch is the chip detection code, specifically if it
> > > > is able to safely distinguish between MAX6680/81 and MAX6695/96.
> > > > Would be great to get some test coverage from a system with one of those chips.
> > > 
> > > Unfortunately I don't have any of these Maxim chips at hand. I have an
> > > ADM1032 but it won't offer much coverage obviously. And I have dumps of
> > > Maxim chips, but the real chips behave differently, so it's of little
> > > help.
> > > 
> > Do you by any chance have register dumps of max6657/58/59 ?
> 
> Yes I do. I have dumps of max6658 and max6659 chips, I'm attaching
> them. Please keep in mind that registers are read sequentially, in
> order, and this matters because these chips return the last read value
> whenever you access a nonexistent register. This even holds for
> registers with fewer than 8 bits, e.g. config1.
> 
> > max6659 also supports a 3rd upper limit. Turns out it is convenient to add support
> > for this limit first before adding support for max6696. To do that, I'll need
> > to be able to distinguish between max6657/58 and max6659. My thought is to check
> > for the address and if register 0x16 exists.
> > 
> > Also, do you happen to remember why address 0x4e for max6659 is not supported by the driver ?
> > The code only says that it isn't supported, and it does not detect it, but there
> > is not reason. sensors-detect does detect it at address 0x4e.
> 
> I think the reason is historical. As we never had a report of anyone
> with a MAX6659 chip at 0x4e, and detection for this device is weak, and
> the driver didn't support any other device at this address, it seemed
> more reasonable to not scan the address at all. Now that the driver
> supports more devices at this address and thus scans it anyway, I have
> no objection supporting the MAX6659 at 0x4e.
> 
Too bad - registers 0x16 and 0x17 exist on both 6658 and 6659. So the only way to detect 6659 
would be the address (0x4d or 0x4e), and we would mis-detect it on 0x4c. Is that worth it ?

Guenter