Skip to contentSkip to conversation

i2c: Fix NACK detection in i2c-pasemi

PATCH In review Olof Johansson 2 participants 4 messages Oct 24, 2007 – Oct 25, 2007

Subsystems (2)
PowerPC I2C subsystem host drivers
lore
Share & tools
lore

Olof Johansson

author
#0 lore raw

PATCH 1/1 i2c: Fix NACK detection in i2c-pasemi

i2c: Fix NACK detection in i2c-pasemi

Turns out we don't actually check the status to see if there was a device out there to talk to, just if we had a timeout when doing so.

Add the proper check, so we don't falsly think there are devices on the bus that are not there, etc.

Signed-off-by Olof Johansson <[email protected]>

---

Quoted 11 lines · Jean Delvare

The question is, how do you manage to get that many errors reported, when I don't get any when loading the lm90 driver, despite the 6 I2C buses on my system. When you reach this point in the lm90 driver, it means that i2c-core successfully probed that there was a device at said address on the given bus. I have a hard time believing that you have chips at 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d and 0x4e on 3 different I2C buses on your system.

It sounds like the underlying bus driver is broken and report chips where they aren't. What are these I2C buses? Their driver(s) needs fixing.

Crap, yeah, you're right. This was the only driver behaving this way for me, and I didn't have another controller to test on.

The bug is indeed in our bus driver, we don't check for NACK's, only timeouts (I was of the impresison that a nack would cause a timeout, but that's obviously wrong).

Patch below. Thanks a bunch!

-Olof

drivers/i2c/busses/i2c-pasemi.c +5 -0
index 58e3271..a1d339e 100644
@@ -51,6 +51,7 @@ struct pasemi_smbus {
 #define MRXFIFO_DATA_M	0x000000ff
 
 #define SMSTA_XEN	0x08000000
+#define SMSTA_MTN	0x00200000
 
 #define CTL_MRR		0x00000400
 #define CTL_MTR		0x00000200

@@ -98,6 +99,10 @@ static unsigned int pasemi_smb_waitready(struct pasemi_smbus *smbus)
 		status = reg_read(smbus, REG_SMSTA);
 	}
 
+	/* Got NACK? */
+	if (status & SMSTA_MTN)
+		return -ENODEV;
+
 	if (timeout < 0) {
 		dev_warn(&smbus->dev->dev, "Timeout, status 0x%08x\n", status);
 		reg_write(smbus, REG_SMSTA, status);

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#1 lore raw

Hi Olof,

Quoted 53 lines · Olof Johansson

i2c: Fix NACK detection in i2c-pasemi

Turns out we don't actually check the status to see if there was a device out there to talk to, just if we had a timeout when doing so.

Add the proper check, so we don't falsly think there are devices on the bus that are not there, etc.

Signed-off-by Olof Johansson <[email protected]>

---

Quoted 11 lines · Jean Delvare

The question is, how do you manage to get that many errors reported, when I don't get any when loading the lm90 driver, despite the 6 I2C buses on my system. When you reach this point in the lm90 driver, it means that i2c-core successfully probed that there was a device at said address on the given bus. I have a hard time believing that you have chips at 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d and 0x4e on 3 different I2C buses on your system.

It sounds like the underlying bus driver is broken and report chips where they aren't. What are these I2C buses? Their driver(s) needs fixing.

Crap, yeah, you're right. This was the only driver behaving this way for me, and I didn't have another controller to test on.

The bug is indeed in our bus driver, we don't check for NACK's, only timeouts (I was of the impresison that a nack would cause a timeout, but that's obviously wrong).

Patch below. Thanks a bunch!

(quoted diff) +0 -1
-Olof
drivers/i2c/busses/i2c-pasemi.c +1 -0
index 58e3271..a1d339e 100644
@@ -51,6 +51,7 @@ struct pasemi_smbus {
 #define MRXFIFO_DATA_M	0x000000ff
 
 #define SMSTA_XEN	0x08000000
+#define SMSTA_MTN	0x00200000
 
 #define CTL_MRR		0x00000400
 #define CTL_MTR		0x00000200

drivers/i2c/busses/i2c-pasemi.c static unsigned int pasemi_smb_waitready(struct pasemi_smbus *smbus) · line 104

Quoted 6 lines
drivers/i2c/busses/i2c-pasemi.c +3 -0
@@ -98,6 +99,10 @@ static unsigned int pasemi_smb_waitready(struct pasemi_smbus *smbus)
 		status = reg_read(smbus, REG_SMSTA);
 	}
 
+	/* Got NACK? */
+	if (status & SMSTA_MTN)
+		return -ENODEV;

What does the SMSTA_MTN bit mean exactly? If it is raised by any NACK, then it doesn't necessarily mean "no device". It can also mean that the slave didn't like the transaction for any reason. In this case -ENODEV is too specific and you should instead return -ENXIO or maybe -EINVAL.

It's up to i2c-core to convert the error code to -ENODEV if it happens during device probing.

Quoted 4 lines
drivers/i2c/busses/i2c-pasemi.c +1 -0
+
 	if (timeout < 0) {
 		dev_warn(&smbus->dev->dev, "Timeout, status 0x%08x\n", status);
 		reg_write(smbus, REG_SMSTA, status);
Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#2 lore raw
In reply to Olof Johansson
Quoted 11 lines · Jean Delvare

Hi Olof,

drivers/i2c/busses/i2c-pasemi.c static unsigned int pasemi_smb_waitready(struct pasemi_smbus *smbus) · line 104

Quoted 3 lines
drivers/i2c/busses/i2c-pasemi.c +3 -0
+	/* Got NACK? */
+	if (status & SMSTA_MTN)
+		return -ENODEV;

What does the SMSTA_MTN bit mean exactly? If it is raised by any NACK, then it doesn't necessarily mean "no device". It can also mean that the slave didn't like the transaction for any reason. In this case -ENODEV is too specific and you should instead return -ENXIO or maybe -EINVAL.

Quoted 2 lines

It's up to i2c-core to convert the error code to -ENODEV if it happens during device probing.

Yes, it could be raised by any NACK. Want me to respin and resend or are you OK with editing before committing?

Thanks for your feedback,

-Olof

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#3 lore raw
In reply to Olof Johansson

Hi Olof,

Quoted 12 lines · Olof Johansson
Quoted 11 lines · Jean Delvare

Hi Olof,

drivers/i2c/busses/i2c-pasemi.c static unsigned int pasemi_smb_waitready(struct pasemi_smbus *smbus) · line 104

Quoted 3 lines
drivers/i2c/busses/i2c-pasemi.c +3 -0
+	/* Got NACK? */
+	if (status & SMSTA_MTN)
+		return -ENODEV;

What does the SMSTA_MTN bit mean exactly? If it is raised by any NACK, then it doesn't necessarily mean "no device". It can also mean that the slave didn't like the transaction for any reason. In this case -ENODEV is too specific and you should instead return -ENXIO or maybe -EINVAL.

Quoted 5 lines
Quoted 2 lines

It's up to i2c-core to convert the error code to -ENODEV if it happens during device probing.

Yes, it could be raised by any NACK. Want me to respin and resend or are you OK with editing before committing?

I'll edit it myself, no problem.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors