Re: [lm-sensors] Add support for LM75A.

Re: [lm-sensors] Add support for LM75A.

[Jean Delvare]

Hi Lennart,

Please send hwmon patches to the lm-sensors list, as specified in
MAINTAINERS.

On Fri, 18 Feb 2011 14:30:00 -0500, Lennart Sorensen wrote:
> The LM75A can not be detected with the same logic as previous LM75
> designs.  Previous designs would return the last value read when an
> unused register was read.  The LM75 returns 0xff when an usesed register
> is read and it also has a new identity register.
> 
> This patch adds the new detection logic for the LM75A, allowing the lm75
> driver to correctly detect the presence of an LM75A as well as older
> LM75 designs.

Do you actually need to _detect_ an LM75A device? The only reason why
the lm75 driver originally detected the LM75 was because it was very
popular as a hardware monitoring chip on PC mainboards. This was in
years 1998-2001, the LM75 is no longer used for this purpose on modern
PC mainboards. So I would be very surprised if an LM75A was used on PC
mainboards.

For embedded systems, or graphics cards, the preferred way is to
explicitly instantiate the I2C device. Can't just you do this?

> 
> Signed-off-by: Len Sorensen <lsorense@csclub.uwaterloo.ca>
> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index f36eb80..6d04cf6 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -250,23 +250,40 @@ static int lm75_detect(struct i2c_client *new_client,
>  	   addresses 0x04-0x07 returning the last read value.
>  	   The cycling+unused addresses combination is not tested,
>  	   since it would significantly slow the detection down and would
> -	   hardly add any value. */
> +	   hardly add any value.
> +
> +	   The LM75A is different.  It has an id byte of 0xaX (where
> +	   X is the chip revision) in register 7, and unused registers
> +	   return 0xff rather than the last read value. */

Maybe this holds for the National Semiconductor LM75A, but not for the
Philips/NXP variant, at least according to the datasheet (no device ID
register.)

>  
> -	/* Unused addresses */
>  	cur = i2c_smbus_read_word_data(new_client, 0);
>  	conf = i2c_smbus_read_byte_data(new_client, 1);
> -	hyst = i2c_smbus_read_word_data(new_client, 2);
> -	if (i2c_smbus_read_word_data(new_client, 4) != hyst
> -	 || i2c_smbus_read_word_data(new_client, 5) != hyst
> -	 || i2c_smbus_read_word_data(new_client, 6) != hyst
> -	 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> -		return -ENODEV;
> -	os = i2c_smbus_read_word_data(new_client, 3);
> -	if (i2c_smbus_read_word_data(new_client, 4) != os
> -	 || i2c_smbus_read_word_data(new_client, 5) != os
> -	 || i2c_smbus_read_word_data(new_client, 6) != os
> -	 || i2c_smbus_read_word_data(new_client, 7) != os)
> -		return -ENODEV;
> +
> +	/* First check for LM75A */
> +	if ((i2c_smbus_read_byte_data(new_client, 7) & 0xf0) = 0xa0) {
> +		/* LM 75A returns 0xff on unused registers so
> +		   just to be sure we check for that too. */

This is definitely a good idea. 4 bits for the device ID is weak, other
devices could match. In fact I think we should check all 8 bits for
value 0xa1, as this is what is documented in the datasheet.

> +		if (i2c_smbus_read_byte_data(new_client, 4) != 0xff
> +		 || i2c_smbus_read_byte_data(new_client, 5) != 0xff
> +		 || i2c_smbus_read_byte_data(new_client, 6) != 0xff)
> +			return -ENODEV;
> +		hyst = i2c_smbus_read_word_data(new_client, 2);
> +		os = i2c_smbus_read_word_data(new_client, 3);
> +	} else { /* Traditional style LM75 detection */
> +		/* Unused addresses */
> +		hyst = i2c_smbus_read_word_data(new_client, 2);
> +		if (i2c_smbus_read_word_data(new_client, 4) != hyst
> +		 || i2c_smbus_read_word_data(new_client, 5) != hyst
> +		 || i2c_smbus_read_word_data(new_client, 6) != hyst
> +		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> +			return -ENODEV;
> +		os = i2c_smbus_read_word_data(new_client, 3);
> +		if (i2c_smbus_read_word_data(new_client, 4) != os
> +		 || i2c_smbus_read_word_data(new_client, 5) != os
> +		 || i2c_smbus_read_word_data(new_client, 6) != os
> +		 || i2c_smbus_read_word_data(new_client, 7) != os)
> +			return -ENODEV;
> +	}
>  
>  	/* Unused bits */
>  	if (conf & 0xe0)
> 

Your patch is incomplete, it should set the name to "lm75a" for the
LM75A. Also, later in the detection routine, address cycling is tested
of offsets 1, 2 and 3, it would make sense to do the same for offset 7
(device ID) for the LM75A.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Lennart Sorensen]

On Sat, Feb 19, 2011 at 11:51:59AM +0100, Jean Delvare wrote:
> Hi Lennart,
> 
> Please send hwmon patches to the lm-sensors list, as specified in
> MAINTAINERS.

Hmm, I looked through there and thought I had found the right address.

> On Fri, 18 Feb 2011 14:30:00 -0500, Lennart Sorensen wrote:
> > The LM75A can not be detected with the same logic as previous LM75
> > designs.  Previous designs would return the last value read when an
> > unused register was read.  The LM75 returns 0xff when an usesed register
> > is read and it also has a new identity register.
> > 
> > This patch adds the new detection logic for the LM75A, allowing the lm75
> > driver to correctly detect the presence of an LM75A as well as older
> > LM75 designs.
> 
> Do you actually need to _detect_ an LM75A device? The only reason why
> the lm75 driver originally detected the LM75 was because it was very
> popular as a hardware monitoring chip on PC mainboards. This was in
> years 1998-2001, the LM75 is no longer used for this purpose on modern
> PC mainboards. So I would be very surprised if an LM75A was used on PC
> mainboards.

Not everything is a PC.  It is still a very common chip on embedded
systems.

> For embedded systems, or graphics cards, the preferred way is to
> explicitly instantiate the I2C device. Can't just you do this?

No, not really.  Auto detection just works, and the only reason I did
this patch is that natsemi is apparently discontinuing the original
LM75s ands starting to ship this new one, so existing designs now have
to start using this new slightly different one.  The coldfire (m68knommu)
doesn't have any board info to specify chips, unlike say powerpc where
it is easy.  I wish we didn't have to go to a new chip, but apparently
we get no choice.  I really don't want to have to design a bunch of
new code to handle detection and registration of lm75 chips with the
driver in user space, when this used to just work before the new chip
version happened.

> > Signed-off-by: Len Sorensen <lsorense@csclub.uwaterloo.ca>
> > 
> > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > index f36eb80..6d04cf6 100644
> > --- a/drivers/hwmon/lm75.c
> > +++ b/drivers/hwmon/lm75.c
> > @@ -250,23 +250,40 @@ static int lm75_detect(struct i2c_client *new_client,
> >  	   addresses 0x04-0x07 returning the last read value.
> >  	   The cycling+unused addresses combination is not tested,
> >  	   since it would significantly slow the detection down and would
> > -	   hardly add any value. */
> > +	   hardly add any value.
> > +
> > +	   The LM75A is different.  It has an id byte of 0xaX (where
> > +	   X is the chip revision) in register 7, and unused registers
> > +	   return 0xff rather than the last read value. */
> 
> Maybe this holds for the National Semiconductor LM75A, but not for the
> Philips/NXP variant, at least according to the datasheet (no device ID
> register.)

Well the LM75B and LM75C didn't have one either, and they still work
fine given I did not remove the old detection method.

> >  
> > -	/* Unused addresses */
> >  	cur = i2c_smbus_read_word_data(new_client, 0);
> >  	conf = i2c_smbus_read_byte_data(new_client, 1);
> > -	hyst = i2c_smbus_read_word_data(new_client, 2);
> > -	if (i2c_smbus_read_word_data(new_client, 4) != hyst
> > -	 || i2c_smbus_read_word_data(new_client, 5) != hyst
> > -	 || i2c_smbus_read_word_data(new_client, 6) != hyst
> > -	 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> > -		return -ENODEV;
> > -	os = i2c_smbus_read_word_data(new_client, 3);
> > -	if (i2c_smbus_read_word_data(new_client, 4) != os
> > -	 || i2c_smbus_read_word_data(new_client, 5) != os
> > -	 || i2c_smbus_read_word_data(new_client, 6) != os
> > -	 || i2c_smbus_read_word_data(new_client, 7) != os)
> > -		return -ENODEV;
> > +
> > +	/* First check for LM75A */
> > +	if ((i2c_smbus_read_byte_data(new_client, 7) & 0xf0) = 0xa0) {
> > +		/* LM 75A returns 0xff on unused registers so
> > +		   just to be sure we check for that too. */
> 
> This is definitely a good idea. 4 bits for the device ID is weak, other
> devices could match. In fact I think we should check all 8 bits for
> value 0xa1, as this is what is documented in the datasheet.

Well the datasheet says 0xa is the ID, and 1 is a revision.  If you want
to fail as soon as they make a new revision, well fine, but I consider
that a bad idea.  The ID really is only the first 4 bits though.  I can
change that if you want, but I am not personally in favour of it, just
in case they do ever update the revision.

I am a bit peeved at natsemi for claiming to have a fully compatible
chip, when the new chip is completely incompatible with how people have
detected their existing chip for years.  Now existing software releases
won't work with new hardware builds because I have to update this
driver first.  How annoying.

> > +		if (i2c_smbus_read_byte_data(new_client, 4) != 0xff
> > +		 || i2c_smbus_read_byte_data(new_client, 5) != 0xff
> > +		 || i2c_smbus_read_byte_data(new_client, 6) != 0xff)
> > +			return -ENODEV;
> > +		hyst = i2c_smbus_read_word_data(new_client, 2);
> > +		os = i2c_smbus_read_word_data(new_client, 3);
> > +	} else { /* Traditional style LM75 detection */
> > +		/* Unused addresses */
> > +		hyst = i2c_smbus_read_word_data(new_client, 2);
> > +		if (i2c_smbus_read_word_data(new_client, 4) != hyst
> > +		 || i2c_smbus_read_word_data(new_client, 5) != hyst
> > +		 || i2c_smbus_read_word_data(new_client, 6) != hyst
> > +		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> > +			return -ENODEV;
> > +		os = i2c_smbus_read_word_data(new_client, 3);
> > +		if (i2c_smbus_read_word_data(new_client, 4) != os
> > +		 || i2c_smbus_read_word_data(new_client, 5) != os
> > +		 || i2c_smbus_read_word_data(new_client, 6) != os
> > +		 || i2c_smbus_read_word_data(new_client, 7) != os)
> > +			return -ENODEV;
> > +	}
> >  
> >  	/* Unused bits */
> >  	if (conf & 0xe0)
> > 
> 
> Your patch is incomplete, it should set the name to "lm75a" for the
> LM75A. Also, later in the detection routine, address cycling is tested
> of offsets 1, 2 and 3, it would make sense to do the same for offset 7
> (device ID) for the LM75A.

Hmm, good point.  I can fix that.  I wasn't sure if the name was supposed
to indicate the driver or the chip.  So if I fix the address cycling
check, and change the name to lm75a when detected, and possibly fix the
ID/rev check to include both, would that help?

-- 
Len Sorensen

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Jean Delvare]

Hi Lennart,

On Tue, 22 Feb 2011 11:39:40 -0500, Lennart Sorensen wrote:
> On Sat, Feb 19, 2011 at 11:51:59AM +0100, Jean Delvare wrote:
> > Do you actually need to _detect_ an LM75A device? The only reason why
> > the lm75 driver originally detected the LM75 was because it was very
> > popular as a hardware monitoring chip on PC mainboards. This was in
> > years 1998-2001, the LM75 is no longer used for this purpose on modern
> > PC mainboards. So I would be very surprised if an LM75A was used on PC
> > mainboards.
> 
> Not everything is a PC.  It is still a very common chip on embedded
> systems.

But embedded systems (arm, blackfin, cris) typically instantiate it
explicitly.

> > For embedded systems, or graphics cards, the preferred way is to
> > explicitly instantiate the I2C device. Can't just you do this?
> 
> No, not really.  Auto detection just works,

You have an interesting definition of "just works", given that you're
sending a patch to fix a problem you've hit ;)

> and the only reason I did
> this patch is that natsemi is apparently discontinuing the original
> LM75s ands starting to ship this new one, so existing designs now have
> to start using this new slightly different one.  The coldfire (m68knommu)
> doesn't have any board info to specify chips, unlike say powerpc where
> it is easy.

Can't it be added? This would be the sanest approach, protecting you
against similar issues in the future.

> I wish we didn't have to go to a new chip, but apparently
> we get no choice.  I really don't want to have to design a bunch of
> new code to handle detection and registration of lm75 chips with the
> driver in user space, when this used to just work before the new chip
> version happened.

A bunch of new code? I don't expect this to need much code (but I admit
I never wrote this - I am not into embedded systems.) Did you only try?

> 
> > > Signed-off-by: Len Sorensen <lsorense@csclub.uwaterloo.ca>
> > > 
> > > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > > index f36eb80..6d04cf6 100644
> > > --- a/drivers/hwmon/lm75.c
> > > +++ b/drivers/hwmon/lm75.c
> > > @@ -250,23 +250,40 @@ static int lm75_detect(struct i2c_client *new_client,
> > >  	   addresses 0x04-0x07 returning the last read value.
> > >  	   The cycling+unused addresses combination is not tested,
> > >  	   since it would significantly slow the detection down and would
> > > -	   hardly add any value. */
> > > +	   hardly add any value.
> > > +
> > > +	   The LM75A is different.  It has an id byte of 0xaX (where
> > > +	   X is the chip revision) in register 7, and unused registers
> > > +	   return 0xff rather than the last read value. */
> > 
> > Maybe this holds for the National Semiconductor LM75A, but not for the
> > Philips/NXP variant, at least according to the datasheet (no device ID
> > register.)
> 
> Well the LM75B and LM75C didn't have one either, and they still work
> fine given I did not remove the old detection method.

My point was that you should say "National Semiconductor LM75A" and not
just LM75A, to avoid confusion with other brands.

> > >  
> > > -	/* Unused addresses */
> > >  	cur = i2c_smbus_read_word_data(new_client, 0);
> > >  	conf = i2c_smbus_read_byte_data(new_client, 1);
> > > -	hyst = i2c_smbus_read_word_data(new_client, 2);
> > > -	if (i2c_smbus_read_word_data(new_client, 4) != hyst
> > > -	 || i2c_smbus_read_word_data(new_client, 5) != hyst
> > > -	 || i2c_smbus_read_word_data(new_client, 6) != hyst
> > > -	 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> > > -		return -ENODEV;
> > > -	os = i2c_smbus_read_word_data(new_client, 3);
> > > -	if (i2c_smbus_read_word_data(new_client, 4) != os
> > > -	 || i2c_smbus_read_word_data(new_client, 5) != os
> > > -	 || i2c_smbus_read_word_data(new_client, 6) != os
> > > -	 || i2c_smbus_read_word_data(new_client, 7) != os)
> > > -		return -ENODEV;
> > > +
> > > +	/* First check for LM75A */
> > > +	if ((i2c_smbus_read_byte_data(new_client, 7) & 0xf0) = 0xa0) {
> > > +		/* LM 75A returns 0xff on unused registers so
> > > +		   just to be sure we check for that too. */
> > 
> > This is definitely a good idea. 4 bits for the device ID is weak, other
> > devices could match. In fact I think we should check all 8 bits for
> > value 0xa1, as this is what is documented in the datasheet.
> 
> Well the datasheet says 0xa is the ID, and 1 is a revision.  If you want
> to fail as soon as they make a new revision, well fine, but I consider
> that a bad idea.  The ID really is only the first 4 bits though.  I can
> change that if you want, but I am not personally in favour of it, just
> in case they do ever update the revision.

My experience with ID registers in I2C devices is that the revision
rarely changes, and when it does, it often requires other driver
changes to properly support the new device revision. So it is
preferable to only support the known revisions, and add new revisions
as they spotted in the wild or datasheets are updated to mention them.

Please keep in mind that there is no standard ID registers in I2C
devices, neither address nor values. So it is perfectly possible for
other devices to match your detection routine if you aren't strict
enough. It is preferable to miss a detection than to have a false
positive and accidentally bind to a totally different device. This is
what I want to avoid.

If you still disagree, a middle ground would be to reject revision = 0
and only accept reasonable revisions (that is revision = 2 may happen
in the future, but I don't expect say revision = 10 any time soon.)

> I am a bit peeved at natsemi for claiming to have a fully compatible
> chip, when the new chip is completely incompatible with how people have
> detected their existing chip for years.  Now existing software releases
> won't work with new hardware builds because I have to update this
> driver first.  How annoying.

This is certainly annoying, but you can't blame National Semiconductor
for this. Instead, blame yourself for still relying on automatic device
detection in 2011 when mechanisms to cleanly instantiate I2C devices
exist since mid-2008. The original LM75 was never meant to be detected,
it doesn't even have an ID register.

> 
> > > +		if (i2c_smbus_read_byte_data(new_client, 4) != 0xff
> > > +		 || i2c_smbus_read_byte_data(new_client, 5) != 0xff
> > > +		 || i2c_smbus_read_byte_data(new_client, 6) != 0xff)
> > > +			return -ENODEV;
> > > +		hyst = i2c_smbus_read_word_data(new_client, 2);
> > > +		os = i2c_smbus_read_word_data(new_client, 3);
> > > +	} else { /* Traditional style LM75 detection */
> > > +		/* Unused addresses */
> > > +		hyst = i2c_smbus_read_word_data(new_client, 2);
> > > +		if (i2c_smbus_read_word_data(new_client, 4) != hyst
> > > +		 || i2c_smbus_read_word_data(new_client, 5) != hyst
> > > +		 || i2c_smbus_read_word_data(new_client, 6) != hyst
> > > +		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> > > +			return -ENODEV;
> > > +		os = i2c_smbus_read_word_data(new_client, 3);
> > > +		if (i2c_smbus_read_word_data(new_client, 4) != os
> > > +		 || i2c_smbus_read_word_data(new_client, 5) != os
> > > +		 || i2c_smbus_read_word_data(new_client, 6) != os
> > > +		 || i2c_smbus_read_word_data(new_client, 7) != os)
> > > +			return -ENODEV;
> > > +	}
> > >  
> > >  	/* Unused bits */
> > >  	if (conf & 0xe0)
> > > 
> > 
> > Your patch is incomplete, it should set the name to "lm75a" for the
> > LM75A. Also, later in the detection routine, address cycling is tested
> > of offsets 1, 2 and 3, it would make sense to do the same for offset 7
> > (device ID) for the LM75A.
> 
> Hmm, good point.  I can fix that.  I wasn't sure if the name was supposed
> to indicate the driver or the chip.

Chip.

> So if I fix the address cycling
> check, and change the name to lm75a when detected, and possibly fix the
> ID/rev check to include both, would that help?

Yes it would.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Lennart Sorensen]

On Tue, Feb 22, 2011 at 06:28:33PM +0100, Jean Delvare wrote:
> But embedded systems (arm, blackfin, cris) typically instantiate it
> explicitly.

Certainly some do.  We happen to boot a single fairly generic coldfire
kernel on multiple similar boards, with slight differences in hardware.
The number of LM75's for example varies.  So far that had worked fine with
nothing really having to worry about which exact board it was booting on.

> You have an interesting definition of "just works", given that you're
> sending a patch to fix a problem you've hit ;)

Well it just worked before, using prior LM75 chips.  The new chips are
the problem for us.

> Can't it be added? This would be the sanest approach, protecting you
> against similar issues in the future.

Well I can certainly start looking into that.  Adding command line
argument passing for the coldfire certainly made a lot of things nicer,
so perhaps adding some dtb like thing could too.  Sounds like a bigger
job though.

> A bunch of new code? I don't expect this to need much code (but I admit
> I never wrote this - I am not into embedded systems.) Did you only try?

When you have a uclinux user space with very little in it, adding more
code to handle detection of LM75 variants and adding them to the driver
would be more code for sure.  Maybe not that much, but still.

> My point was that you should say "National Semiconductor LM75A" and not
> just LM75A, to avoid confusion with other brands.

Ah, I see.  I thought the LMxx was always National Semiconductor and
that others used different letters.

> My experience with ID registers in I2C devices is that the revision
> rarely changes, and when it does, it often requires other driver
> changes to properly support the new device revision. So it is
> preferable to only support the known revisions, and add new revisions
> as they spotted in the wild or datasheets are updated to mention them.

Well I can do that.  Either way it is more accurate detection than the
older style LM75 detection ever had.  Once detected the new one is
completely compatible with the old one.

> Please keep in mind that there is no standard ID registers in I2C
> devices, neither address nor values. So it is perfectly possible for
> other devices to match your detection routine if you aren't strict
> enough. It is preferable to miss a detection than to have a false
> positive and accidentally bind to a totally different device. This is
> what I want to avoid.

Of course.  I am amazed how the old detection code has managed to not
be a problem.

> If you still disagree, a middle ground would be to reject revision = 0
> and only accept reasonable revisions (that is revision = 2 may happen
> in the future, but I don't expect say revision = 10 any time soon.)

Well I will just match on the current revision for now then.

> This is certainly annoying, but you can't blame National Semiconductor
> for this. Instead, blame yourself for still relying on automatic device
> detection in 2011 when mechanisms to cleanly instantiate I2C devices
> exist since mid-2008. The original LM75 was never meant to be detected,
> it doesn't even have an ID register.

Well at least the LM75A does have an ID register, so supporting detection
of it seems fair enough.

> Chip.

OK, that makes sense.  Easily done.

> Yes it would.

I will do that.  I hope to have an updated patch in a couple of hours.

-- 
Len Sorensen

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Guenter Roeck]

On Tue, 2011-02-22 at 13:27 -0500, Lennart Sorensen wrote:
> On Tue, Feb 22, 2011 at 06:28:33PM +0100, Jean Delvare wrote:
> > But embedded systems (arm, blackfin, cris) typically instantiate it
> > explicitly.
> 
> Certainly some do.  We happen to boot a single fairly generic coldfire
> kernel on multiple similar boards, with slight differences in hardware.
> The number of LM75's for example varies.  So far that had worked fine with
> nothing really having to worry about which exact board it was booting on.
> 
> > You have an interesting definition of "just works", given that you're
> > sending a patch to fix a problem you've hit ;)
> 
> Well it just worked before, using prior LM75 chips.  The new chips are
> the problem for us.
> 
> > Can't it be added? This would be the sanest approach, protecting you
> > against similar issues in the future.
> 
> Well I can certainly start looking into that.  Adding command line
> argument passing for the coldfire certainly made a lot of things nicer,
> so perhaps adding some dtb like thing could too.  Sounds like a bigger
> job though.
> 
> > A bunch of new code? I don't expect this to need much code (but I admit
> > I never wrote this - I am not into embedded systems.) Did you only try?
> 
> When you have a uclinux user space with very little in it, adding more
> code to handle detection of LM75 variants and adding them to the driver
> would be more code for sure.  Maybe not that much, but still.
> 
> > My point was that you should say "National Semiconductor LM75A" and not
> > just LM75A, to avoid confusion with other brands.
> 
> Ah, I see.  I thought the LMxx was always National Semiconductor and
> that others used different letters.
> 
> > My experience with ID registers in I2C devices is that the revision
> > rarely changes, and when it does, it often requires other driver
> > changes to properly support the new device revision. So it is
> > preferable to only support the known revisions, and add new revisions
> > as they spotted in the wild or datasheets are updated to mention them.
> 
> Well I can do that.  Either way it is more accurate detection than the
> older style LM75 detection ever had.  Once detected the new one is
> completely compatible with the old one.
> 
> > Please keep in mind that there is no standard ID registers in I2C
> > devices, neither address nor values. So it is perfectly possible for
> > other devices to match your detection routine if you aren't strict
> > enough. It is preferable to miss a detection than to have a false
> > positive and accidentally bind to a totally different device. This is
> > what I want to avoid.
> 
> Of course.  I am amazed how the old detection code has managed to not
> be a problem.
> 
> > If you still disagree, a middle ground would be to reject revision = 0
> > and only accept reasonable revisions (that is revision = 2 may happen
> > in the future, but I don't expect say revision = 10 any time soon.)
> 
> Well I will just match on the current revision for now then.
> 
> > This is certainly annoying, but you can't blame National Semiconductor
> > for this. Instead, blame yourself for still relying on automatic device
> > detection in 2011 when mechanisms to cleanly instantiate I2C devices
> > exist since mid-2008. The original LM75 was never meant to be detected,
> > it doesn't even have an ID register.
> 
> Well at least the LM75A does have an ID register, so supporting detection
> of it seems fair enough.
> 
> > Chip.
> 
> OK, that makes sense.  Easily done.
> 
> > Yes it would.
> 
> I will do that.  I hope to have an updated patch in a couple of hours.
> 
Fwiw, I have a system with TI TMP75AIDGKT (per HW spec). Detection code
doesn't work for it, since it always returns 0xff when reading registers
0x4..0x7. So the lm75 detection code is really of limited value and can
not be expected to work for all chips supported by the driver.

Here is a register dump:

root# i2cdump -y 70 0x48
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: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
10: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
20: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
30: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
40: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
50: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
60: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
70: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
80: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
90: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
a0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
b0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
c0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
d0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
e0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
f0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Jean Delvare]

Hi Guenter,

On Tue, 22 Feb 2011 11:29:46 -0800, Guenter Roeck wrote:
> Fwiw, I have a system with TI TMP75AIDGKT (per HW spec). Detection code
> doesn't work for it, since it always returns 0xff when reading registers
> 0x4..0x7. So the lm75 detection code is really of limited value and can
> not be expected to work for all chips supported by the driver.

And it was never meant to. As said before, the detection of the LM75
proper is there only because the chip was once popular on PC
mainboards. None of the other chips supported by the lm75 driver was as
far as I know.

> Here is a register dump:
> 
> root# i2cdump -y 70 0x48
> 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: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 10: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 20: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 30: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 40: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 50: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 60: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 70: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 80: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 90: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> a0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> b0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> c0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> d0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> e0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> f0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....

Added to my collection, thanks :) But there is no way to detect this
chip reliably.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Lennart Sorensen]

On Tue, Feb 22, 2011 at 08:56:30PM +0100, Jean Delvare wrote:
> And it was never meant to. As said before, the detection of the LM75
> proper is there only because the chip was once popular on PC
> mainboards. None of the other chips supported by the lm75 driver was as
> far as I know.

Actually one of the products we will have the LM75A in is an x86
industrial PC, so I guess that counts as being in a PC.  Up until now
they were LM75Cs (the old ones), so autodetection was handy.

-- 
Len Sorensen

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Guenter Roeck]

On Tue, 2011-02-22 at 14:56 -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> On Tue, 22 Feb 2011 11:29:46 -0800, Guenter Roeck wrote:
> > Fwiw, I have a system with TI TMP75AIDGKT (per HW spec). Detection code
> > doesn't work for it, since it always returns 0xff when reading registers
> > 0x4..0x7. So the lm75 detection code is really of limited value and can
> > not be expected to work for all chips supported by the driver.
> 
> And it was never meant to. As said before, the detection of the LM75
> proper is there only because the chip was once popular on PC
> mainboards. None of the other chips supported by the lm75 driver was as
> far as I know.
> 
> > Here is a register dump:
> > 
> > root# i2cdump -y 70 0x48
> > 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: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 10: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 20: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 30: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 40: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 50: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 60: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 70: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 80: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > 90: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > a0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > b0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > c0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > d0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > e0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> > f0: 13 00 4b 50 ff ff ff ff 13 00 4b 50 ff ff ff ff    ?.KP....?.KP....
> 
> Added to my collection, thanks :) But there is no way to detect this
> chip reliably.
> 
I didn't expect it to; our code uses i2c_register_board_info(). Just
trying to show how unreliable chip detection is for this chip.

Thanks,
Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Jean Delvare]

Hi Lennart,

On Tue, 22 Feb 2011 13:27:13 -0500, Lennart Sorensen wrote:
> On Tue, Feb 22, 2011 at 06:28:33PM +0100, Jean Delvare wrote:
> > My point was that you should say "National Semiconductor LM75A" and not
> > just LM75A, to avoid confusion with other brands.
> 
> Ah, I see.  I thought the LMxx was always National Semiconductor and
> that others used different letters.

This is generally true, so much that we did not bother including the
vendor name in i2c device IDs (matching is done on the name string
alone.) But apparently the LM75 is such a popular device that is has
more clones than any other I2C device I know of, including, sadly, some
with the exact same name, but not 100% compatible, from other vendors.

> > My experience with ID registers in I2C devices is that the revision
> > rarely changes, and when it does, it often requires other driver
> > changes to properly support the new device revision. So it is
> > preferable to only support the known revisions, and add new revisions
> > as they spotted in the wild or datasheets are updated to mention them.
> 
> Well I can do that.  Either way it is more accurate detection than the
> older style LM75 detection ever had.

I beg to disagree. I would say that both detections are comparable. The
LM75-specific behavior of "registers" (or lack thereof) 0x04-0x07,
coupled with their cycling, is an almost unique characteristic (which
is why we decided to use it for detection purpose.) If you ask me, I
would say that a random chip at an LM75-compatible address is less
likely to implement this characteristic than to have value 0xA as the
high nibble of register 0x07.

> Once detected the new one is
> completely compatible with the old one.

Thankfully, yes :)

> > Please keep in mind that there is no standard ID registers in I2C
> > devices, neither address nor values. So it is perfectly possible for
> > other devices to match your detection routine if you aren't strict
> > enough. It is preferable to miss a detection than to have a false
> > positive and accidentally bind to a totally different device. This is
> > what I want to avoid.
> 
> Of course.  I am amazed how the old detection code has managed to not
> be a problem.

It has been problematic, and was improved over time. You can see the
first version of the detection code here:
  http://www.lm-sensors.org/browser/lm-sensors/trunk/kernel/chips/lm75.c?rev38

> > If you still disagree, a middle ground would be to reject revision = 0
> > and only accept reasonable revisions (that is revision = 2 may happen
> > in the future, but I don't expect say revision = 10 any time soon.)
> 
> Well I will just match on the current revision for now then.

OK, fine with me.

> > This is certainly annoying, but you can't blame National Semiconductor
> > for this. Instead, blame yourself for still relying on automatic device
> > detection in 2011 when mechanisms to cleanly instantiate I2C devices
> > exist since mid-2008. The original LM75 was never meant to be detected,
> > it doesn't even have an ID register.
> 
> Well at least the LM75A does have an ID register, so supporting detection
> of it seems fair enough.

Yes and no. 4 bits for the device ID is weak, especially when in a
random register (which is why I insisted in using the whole 8 bits,
including the revision.) "Good" devices have 8 bits of vendor ID + 8
bits of device ID at semi-standard addresses, and even this is not
bullet proof.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Lennart Sorensen]

On Tue, Feb 22, 2011 at 01:27:13PM -0500, Lennart Sorensen wrote:
> I will do that.  I hope to have an updated patch in a couple of hours.

OK, here is an updated one:

Add support for detection of LM75A using the ID register value.

Signed-off-by: Len Sorensen <lsorense@csclub.uwaterloo.ca>

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index f36eb80..5900926 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -232,6 +232,8 @@ static const struct i2c_device_id lm75_ids[] = {
 };
 MODULE_DEVICE_TABLE(i2c, lm75_ids);
 
+#define LM75A_ID 0xA1
+
 /* Return 0 if detection is successful, -ENODEV otherwise */
 static int lm75_detect(struct i2c_client *new_client,
 		       struct i2c_board_info *info)
@@ -239,6 +241,7 @@ static int lm75_detect(struct i2c_client *new_client,
 	struct i2c_adapter *adapter = new_client->adapter;
 	int i;
 	int cur, conf, hyst, os;
+	bool is_lm75a = 0;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
 				     I2C_FUNC_SMBUS_WORD_DATA))
@@ -250,23 +253,43 @@ static int lm75_detect(struct i2c_client *new_client,
 	   addresses 0x04-0x07 returning the last read value.
 	   The cycling+unused addresses combination is not tested,
 	   since it would significantly slow the detection down and would
-	   hardly add any value. */
+	   hardly add any value.
+
+	   The National Semiconductor LM75A is different than earlier
+	   LM75s.  It has an ID byte of 0xaX (where X is the chip
+	   revision, with 1 being the only revision in existance) in
+	   register 7, and unused registers return 0xff rather than the
+	   last read value. */
 
-	/* Unused addresses */
 	cur = i2c_smbus_read_word_data(new_client, 0);
 	conf = i2c_smbus_read_byte_data(new_client, 1);
-	hyst = i2c_smbus_read_word_data(new_client, 2);
-	if (i2c_smbus_read_word_data(new_client, 4) != hyst
-	 || i2c_smbus_read_word_data(new_client, 5) != hyst
-	 || i2c_smbus_read_word_data(new_client, 6) != hyst
-	 || i2c_smbus_read_word_data(new_client, 7) != hyst)
-		return -ENODEV;
-	os = i2c_smbus_read_word_data(new_client, 3);
-	if (i2c_smbus_read_word_data(new_client, 4) != os
-	 || i2c_smbus_read_word_data(new_client, 5) != os
-	 || i2c_smbus_read_word_data(new_client, 6) != os
-	 || i2c_smbus_read_word_data(new_client, 7) != os)
-		return -ENODEV;
+
+	/* First check for LM75A */
+	if (i2c_smbus_read_byte_data(new_client, 7) = LM75A_ID) {
+		/* LM 75A returns 0xff on unused registers so
+		   just to be sure we check for that too. */
+		if (i2c_smbus_read_byte_data(new_client, 4) != 0xff
+		 || i2c_smbus_read_byte_data(new_client, 5) != 0xff
+		 || i2c_smbus_read_byte_data(new_client, 6) != 0xff)
+			return -ENODEV;
+		is_lm75a = 1;
+		hyst = i2c_smbus_read_word_data(new_client, 2);
+		os = i2c_smbus_read_word_data(new_client, 3);
+	} else { /* Traditional style LM75 detection */
+		/* Unused addresses */
+		hyst = i2c_smbus_read_word_data(new_client, 2);
+		if (i2c_smbus_read_word_data(new_client, 4) != hyst
+		 || i2c_smbus_read_word_data(new_client, 5) != hyst
+		 || i2c_smbus_read_word_data(new_client, 6) != hyst
+		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
+			return -ENODEV;
+		os = i2c_smbus_read_word_data(new_client, 3);
+		if (i2c_smbus_read_word_data(new_client, 4) != os
+		 || i2c_smbus_read_word_data(new_client, 5) != os
+		 || i2c_smbus_read_word_data(new_client, 6) != os
+		 || i2c_smbus_read_word_data(new_client, 7) != os)
+			return -ENODEV;
+	}
 
 	/* Unused bits */
 	if (conf & 0xe0)
@@ -278,9 +301,14 @@ static int lm75_detect(struct i2c_client *new_client,
 		 || i2c_smbus_read_word_data(new_client, i + 2) != hyst
 		 || i2c_smbus_read_word_data(new_client, i + 3) != os)
 			return -ENODEV;
+		if (is_lm75a && i2c_smbus_read_byte_data(new_client, i + 7) != LM75A_ID)
+			return -ENODEV;
 	}
 
-	strlcpy(info->type, "lm75", I2C_NAME_SIZE);
+	if (is_lm75a)
+		strlcpy(info->type, "lm75a", I2C_NAME_SIZE);
+	else
+		strlcpy(info->type, "lm75", I2C_NAME_SIZE);
 
 	return 0;
 }

-- 
Len Sorensen

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Lennart Sorensen]

On Tue, Feb 22, 2011 at 11:10:44PM +0100, Jean Delvare wrote:
> This is generally true, so much that we did not bother including the
> vendor name in i2c device IDs (matching is done on the name string
> alone.) But apparently the LM75 is such a popular device that is has
> more clones than any other I2C device I know of, including, sadly, some
> with the exact same name, but not 100% compatible, from other vendors.

What a mess.

> I beg to disagree. I would say that both detections are comparable. The
> LM75-specific behavior of "registers" (or lack thereof) 0x04-0x07,
> coupled with their cycling, is an almost unique characteristic (which
> is why we decided to use it for detection purpose.) If you ask me, I
> would say that a random chip at an LM75-compatible address is less
> likely to implement this characteristic than to have value 0xA as the
> high nibble of register 0x07.

It isn't that unique, that's true.  But combined with address cycling
and the other checks?

> It has been problematic, and was improved over time. You can see the
> first version of the detection code here:
>   http://www.lm-sensors.org/browser/lm-sensors/trunk/kernel/chips/lm75.c?rev38

Certainly a bit simpler than the current one.

> OK, fine with me.

Done in the new version.

> Yes and no. 4 bits for the device ID is weak, especially when in a
> random register (which is why I insisted in using the whole 8 bits,
> including the revision.) "Good" devices have 8 bits of vendor ID + 8
> bits of device ID at semi-standard addresses, and even this is not
> bullet proof.

Given they support 16 bit register on the LM75 already, I must admit I
am amazed they didn't do an 8 bit ID and 8 bit revision using a 16bit
register.

-- 
Len Sorensen

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Jean Delvare]

On Tue, 22 Feb 2011 17:20:09 -0500, Lennart Sorensen wrote:
> On Tue, Feb 22, 2011 at 11:10:44PM +0100, Jean Delvare wrote:
> > Yes and no. 4 bits for the device ID is weak, especially when in a
> > random register (which is why I insisted in using the whole 8 bits,
> > including the revision.) "Good" devices have 8 bits of vendor ID + 8
> > bits of device ID at semi-standard addresses, and even this is not
> > bullet proof.
> 
> Given they support 16 bit register on the LM75 already, I must admit I
> am amazed they didn't do an 8 bit ID and 8 bit revision using a 16bit
> register.

True. And they could have put an 8-bit manufacturer ID register at 0x06
too. But I guess it would have been too simple and straightforward and
they preferred to have some fun with us :/

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[richardvoigt]

> True. And they could have put an 8-bit manufacturer ID register at 0x06

Which the clones would have promptly imitated.  Not the concept, mind
you, they'd use the same _value_.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Lennart Sorensen]

On Wed, Feb 23, 2011 at 04:04:53PM -0600, richardvoigt@gmail.com wrote:
> > True. And they could have put an 8-bit manufacturer ID register at 0x06
> 
> Which the clones would have promptly imitated.  Not the concept, mind
> you, they'd use the same _value_.

At least detection would work then.

-- 
Len Sorensen

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[richardvoigt]

On Wed, Feb 23, 2011 at 4:08 PM, Lennart Sorensen
<lsorense@csclub.uwaterloo.ca> wrote:
> On Wed, Feb 23, 2011 at 04:04:53PM -0600, richardvoigt@gmail.com wrote:
>> > True. And they could have put an 8-bit manufacturer ID register at 0x06
>>
>> Which the clones would have promptly imitated.  Not the concept, mind
>> you, they'd use the same _value_.
>
> At least detection would work then.

But the clones would assuredly use the other registers in incompatible
ways.  (assured by Murphy's Law)

>
> --
> Len Sorensen
>

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Jean Delvare]

On Tue, 22 Feb 2011 17:15:41 -0500, Lennart Sorensen wrote:
> On Tue, Feb 22, 2011 at 01:27:13PM -0500, Lennart Sorensen wrote:
> > I will do that.  I hope to have an updated patch in a couple of hours.
> 
> OK, here is an updated one:
> 
> Add support for detection of LM75A using the ID register value.
> 
> Signed-off-by: Len Sorensen <lsorense@csclub.uwaterloo.ca>
> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index f36eb80..5900926 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -232,6 +232,8 @@ static const struct i2c_device_id lm75_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, lm75_ids);
>  
> +#define LM75A_ID 0xA1
> +
>  /* Return 0 if detection is successful, -ENODEV otherwise */
>  static int lm75_detect(struct i2c_client *new_client,
>  		       struct i2c_board_info *info)
> @@ -239,6 +241,7 @@ static int lm75_detect(struct i2c_client *new_client,
>  	struct i2c_adapter *adapter = new_client->adapter;
>  	int i;
>  	int cur, conf, hyst, os;
> +	bool is_lm75a = 0;
>  
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>  				     I2C_FUNC_SMBUS_WORD_DATA))
> @@ -250,23 +253,43 @@ static int lm75_detect(struct i2c_client *new_client,
>  	   addresses 0x04-0x07 returning the last read value.
>  	   The cycling+unused addresses combination is not tested,
>  	   since it would significantly slow the detection down and would
> -	   hardly add any value. */
> +	   hardly add any value.
> +
> +	   The National Semiconductor LM75A is different than earlier
> +	   LM75s.  It has an ID byte of 0xaX (where X is the chip
> +	   revision, with 1 being the only revision in existance) in
> +	   register 7, and unused registers return 0xff rather than the
> +	   last read value. */
>  
> -	/* Unused addresses */
>  	cur = i2c_smbus_read_word_data(new_client, 0);
>  	conf = i2c_smbus_read_byte_data(new_client, 1);
> -	hyst = i2c_smbus_read_word_data(new_client, 2);
> -	if (i2c_smbus_read_word_data(new_client, 4) != hyst
> -	 || i2c_smbus_read_word_data(new_client, 5) != hyst
> -	 || i2c_smbus_read_word_data(new_client, 6) != hyst
> -	 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> -		return -ENODEV;
> -	os = i2c_smbus_read_word_data(new_client, 3);
> -	if (i2c_smbus_read_word_data(new_client, 4) != os
> -	 || i2c_smbus_read_word_data(new_client, 5) != os
> -	 || i2c_smbus_read_word_data(new_client, 6) != os
> -	 || i2c_smbus_read_word_data(new_client, 7) != os)
> -		return -ENODEV;
> +
> +	/* First check for LM75A */
> +	if (i2c_smbus_read_byte_data(new_client, 7) = LM75A_ID) {
> +		/* LM 75A returns 0xff on unused registers so
> +		   just to be sure we check for that too. */
> +		if (i2c_smbus_read_byte_data(new_client, 4) != 0xff
> +		 || i2c_smbus_read_byte_data(new_client, 5) != 0xff
> +		 || i2c_smbus_read_byte_data(new_client, 6) != 0xff)
> +			return -ENODEV;
> +		is_lm75a = 1;
> +		hyst = i2c_smbus_read_word_data(new_client, 2);
> +		os = i2c_smbus_read_word_data(new_client, 3);
> +	} else { /* Traditional style LM75 detection */
> +		/* Unused addresses */
> +		hyst = i2c_smbus_read_word_data(new_client, 2);
> +		if (i2c_smbus_read_word_data(new_client, 4) != hyst
> +		 || i2c_smbus_read_word_data(new_client, 5) != hyst
> +		 || i2c_smbus_read_word_data(new_client, 6) != hyst
> +		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> +			return -ENODEV;
> +		os = i2c_smbus_read_word_data(new_client, 3);
> +		if (i2c_smbus_read_word_data(new_client, 4) != os
> +		 || i2c_smbus_read_word_data(new_client, 5) != os
> +		 || i2c_smbus_read_word_data(new_client, 6) != os
> +		 || i2c_smbus_read_word_data(new_client, 7) != os)
> +			return -ENODEV;
> +	}
>  
>  	/* Unused bits */
>  	if (conf & 0xe0)
> @@ -278,9 +301,14 @@ static int lm75_detect(struct i2c_client *new_client,
>  		 || i2c_smbus_read_word_data(new_client, i + 2) != hyst
>  		 || i2c_smbus_read_word_data(new_client, i + 3) != os)
>  			return -ENODEV;
> +		if (is_lm75a && i2c_smbus_read_byte_data(new_client, i + 7) != LM75A_ID)
> +			return -ENODEV;
>  	}
>  
> -	strlcpy(info->type, "lm75", I2C_NAME_SIZE);
> +	if (is_lm75a)
> +		strlcpy(info->type, "lm75a", I2C_NAME_SIZE);
> +	else
> +		strlcpy(info->type, "lm75", I2C_NAME_SIZE);
>  
>  	return 0;
>  }
> 

Applied with minor style fixes, thank you.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Lennart Sorensen]

On Thu, Feb 24, 2011 at 10:45:52AM +0100, Jean Delvare wrote:
> On Tue, 22 Feb 2011 17:15:41 -0500, Lennart Sorensen wrote:
> > On Tue, Feb 22, 2011 at 01:27:13PM -0500, Lennart Sorensen wrote:
> > > I will do that.  I hope to have an updated patch in a couple of hours.
> > 
> > OK, here is an updated one:
> > 
> > Add support for detection of LM75A using the ID register value.
> > 
> > Signed-off-by: Len Sorensen <lsorense@csclub.uwaterloo.ca>
> > 
> > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > index f36eb80..5900926 100644
> > --- a/drivers/hwmon/lm75.c
> > +++ b/drivers/hwmon/lm75.c
> > @@ -232,6 +232,8 @@ static const struct i2c_device_id lm75_ids[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, lm75_ids);
> >  
> > +#define LM75A_ID 0xA1
> > +
> >  /* Return 0 if detection is successful, -ENODEV otherwise */
> >  static int lm75_detect(struct i2c_client *new_client,
> >  		       struct i2c_board_info *info)
> > @@ -239,6 +241,7 @@ static int lm75_detect(struct i2c_client *new_client,
> >  	struct i2c_adapter *adapter = new_client->adapter;
> >  	int i;
> >  	int cur, conf, hyst, os;
> > +	bool is_lm75a = 0;
> >  
> >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> >  				     I2C_FUNC_SMBUS_WORD_DATA))
> > @@ -250,23 +253,43 @@ static int lm75_detect(struct i2c_client *new_client,
> >  	   addresses 0x04-0x07 returning the last read value.
> >  	   The cycling+unused addresses combination is not tested,
> >  	   since it would significantly slow the detection down and would
> > -	   hardly add any value. */
> > +	   hardly add any value.
> > +
> > +	   The National Semiconductor LM75A is different than earlier
> > +	   LM75s.  It has an ID byte of 0xaX (where X is the chip
> > +	   revision, with 1 being the only revision in existance) in
> > +	   register 7, and unused registers return 0xff rather than the
> > +	   last read value. */
> >  
> > -	/* Unused addresses */
> >  	cur = i2c_smbus_read_word_data(new_client, 0);
> >  	conf = i2c_smbus_read_byte_data(new_client, 1);
> > -	hyst = i2c_smbus_read_word_data(new_client, 2);
> > -	if (i2c_smbus_read_word_data(new_client, 4) != hyst
> > -	 || i2c_smbus_read_word_data(new_client, 5) != hyst
> > -	 || i2c_smbus_read_word_data(new_client, 6) != hyst
> > -	 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> > -		return -ENODEV;
> > -	os = i2c_smbus_read_word_data(new_client, 3);
> > -	if (i2c_smbus_read_word_data(new_client, 4) != os
> > -	 || i2c_smbus_read_word_data(new_client, 5) != os
> > -	 || i2c_smbus_read_word_data(new_client, 6) != os
> > -	 || i2c_smbus_read_word_data(new_client, 7) != os)
> > -		return -ENODEV;
> > +
> > +	/* First check for LM75A */
> > +	if (i2c_smbus_read_byte_data(new_client, 7) = LM75A_ID) {
> > +		/* LM 75A returns 0xff on unused registers so
> > +		   just to be sure we check for that too. */
> > +		if (i2c_smbus_read_byte_data(new_client, 4) != 0xff
> > +		 || i2c_smbus_read_byte_data(new_client, 5) != 0xff
> > +		 || i2c_smbus_read_byte_data(new_client, 6) != 0xff)
> > +			return -ENODEV;
> > +		is_lm75a = 1;
> > +		hyst = i2c_smbus_read_word_data(new_client, 2);
> > +		os = i2c_smbus_read_word_data(new_client, 3);
> > +	} else { /* Traditional style LM75 detection */
> > +		/* Unused addresses */
> > +		hyst = i2c_smbus_read_word_data(new_client, 2);
> > +		if (i2c_smbus_read_word_data(new_client, 4) != hyst
> > +		 || i2c_smbus_read_word_data(new_client, 5) != hyst
> > +		 || i2c_smbus_read_word_data(new_client, 6) != hyst
> > +		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> > +			return -ENODEV;
> > +		os = i2c_smbus_read_word_data(new_client, 3);
> > +		if (i2c_smbus_read_word_data(new_client, 4) != os
> > +		 || i2c_smbus_read_word_data(new_client, 5) != os
> > +		 || i2c_smbus_read_word_data(new_client, 6) != os
> > +		 || i2c_smbus_read_word_data(new_client, 7) != os)
> > +			return -ENODEV;
> > +	}
> >  
> >  	/* Unused bits */
> >  	if (conf & 0xe0)
> > @@ -278,9 +301,14 @@ static int lm75_detect(struct i2c_client *new_client,
> >  		 || i2c_smbus_read_word_data(new_client, i + 2) != hyst
> >  		 || i2c_smbus_read_word_data(new_client, i + 3) != os)
> >  			return -ENODEV;
> > +		if (is_lm75a && i2c_smbus_read_byte_data(new_client, i + 7) != LM75A_ID)
> > +			return -ENODEV;
> >  	}
> >  
> > -	strlcpy(info->type, "lm75", I2C_NAME_SIZE);
> > +	if (is_lm75a)
> > +		strlcpy(info->type, "lm75a", I2C_NAME_SIZE);
> > +	else
> > +		strlcpy(info->type, "lm75", I2C_NAME_SIZE);
> >  
> >  	return 0;
> >  }
> > 
> 
> Applied with minor style fixes, thank you.

I will have to go see what that entails.  I thought I had done a decent
job following the existing style. :)

-- 
Len Sorensen

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Jean Delvare]

On Thu, 24 Feb 2011 11:31:22 -0500, Lennart Sorensen wrote:
> On Thu, Feb 24, 2011 at 10:45:52AM +0100, Jean Delvare wrote:
> > Applied with minor style fixes, thank you.
> 
> I will have to go see what that entails.  I thought I had done a decent
> job following the existing style. :)

See for yourself:

ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/hwmon-lm75-01-add-support-for-lm75a.patch

You did a nice job, but I'm a nitpicker ;)

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Add support for LM75A.

[Lennart Sorensen]

On Thu, Feb 24, 2011 at 05:41:42PM +0100, Jean Delvare wrote:
> See for yourself:
> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/hwmon-lm75-01-add-support-for-lm75a.patch
> 
> You did a nice job, but I'm a nitpicker ;)

I see the changes, and I do like the improvements.

-- 
Len Sorensen

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors