[lm-sensors] [PATCH RESEND] hwmon: lm70: TI TMP121 support.
david-b at pacbell.net
Thu Oct 30 07:49:00 CET 2008
On Wednesday 29 October 2008, Kaiwan N Billimoria wrote:
> On Tue, 2008-10-28 at 03:43 -0700, David Brownell wrote:
> > On Tuesday 28 October 2008, Kaiwan N Billimoria wrote:
> > > Hi,
> > >
> > > Got a chance to test this patch today..
> > > short answer: no it did not seem to work.
> > Well, then can you see what the issue is then?
Maybe the lm70llp code shouldn't set spi->bits_per_word to 16;
that seems to be another problem.
> > The current mainline code is clearly buggy...
> A quick qs: (perhaps i'm missing something obvious/silly here);
> rxbuf is the MSB part of the rxbuf buffer right.
No. Since the sensor sends an 11-bit number (sign then 10 data bits)
in MSB format, when the lm70llp (parport adapter) returns two bytes
it will return first rxbuf with the 8 MSBs, then rxbuf with
three data bits and five irrelevant bits. (Although code at that
level has been known to goof up and shift a bit in the wrong direction,
for example because it's sampling the wrong edge...)
rxbuf holds the MSBs ... rxbuf holds the LSBs.
Recall that the whole reason we noticed this bug is that the $SUBJECT
patch was -- correctly!! -- getting its MSBs from rxbuf, but Jean
noted the lm70 code was different.
> So the code is moving in the bits from there first into the variable
> raw, then adding in the LSB part of rxbuf. Why is that wrong?
See above. If spi->bits_per_word were 16 AND you were hard-wiring
the lm70.c code to work only on little-endian hardware, THEN it would
be right to "know" that rxbuf has the MSBs.
But it's not OK to hard-wire drivers to work only on little endian
systems, which is why I suspect the missing part of the fix is to
strike the lm70llp line setting bits_per_word to nonzero.
> > I think there must be bugs covering up for each other...
> > > I can (re)confirm though that for the LM70 chip the temperature
> > register
> > > is sent MSB first...
> > Right, which is why the lm70.c code should use the
> > > - raw = (rxbuf << 8) + rxbuf;
> > > + raw = (rxbuf << 8) + rxbuf;
> > So the question is then what *else* is going on (buggy)
> > that the previous code seemed to work despite that bug...
> > - Dave
> Okay, though am unable to work on this right now.
> It's in my queue & i'll get back when i have a chance...
More information about the lm-sensors