[lm-sensors] [PATCH] i2c: Fix NACK detection in i2c-pasemi

[lm-sensors] [PATCH] i2c: Fix NACK detection in i2c-pasemi

[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 <olof@lixom.net>

---

On Wed, Oct 24, 2007 at 01:51:09PM +0200, Jean Delvare wrote:

> 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


diff --git a/drivers/i2c/busses/i2c-pasemi.c b/drivers/i2c/busses/i2c-pasemi.c
index 58e3271..a1d339e 100644
--- a/drivers/i2c/busses/i2c-pasemi.c
+++ b/drivers/i2c/busses/i2c-pasemi.c
@@ -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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] i2c: Fix NACK detection in i2c-pasemi

[Jean Delvare]

Hi Olof,

On Wed, 24 Oct 2007 10:59:17 -0500, Olof Johansson wrote:
> 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 <olof@lixom.net>
> 
> ---
> 
> On Wed, Oct 24, 2007 at 01:51:09PM +0200, Jean Delvare wrote:
> 
> > 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
> 
> 
> diff --git a/drivers/i2c/busses/i2c-pasemi.c b/drivers/i2c/busses/i2c-pasemi.c
> index 58e3271..a1d339e 100644
> --- a/drivers/i2c/busses/i2c-pasemi.c
> +++ b/drivers/i2c/busses/i2c-pasemi.c
> @@ -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;

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.

> +
>  	if (timeout < 0) {
>  		dev_warn(&smbus->dev->dev, "Timeout, status 0x%08x\n", status);
>  		reg_write(smbus, REG_SMSTA, status);

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] i2c: Fix NACK detection in i2c-pasemi

[Olof Johansson]

On Wed, Oct 24, 2007 at 11:39:34PM +0200, Jean Delvare wrote:
> Hi Olof,
> 
> > +	/* 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.

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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] i2c: Fix NACK detection in i2c-pasemi

[Jean Delvare]

Hi Olof,

On Wed, 24 Oct 2007 17:13:04 -0500, Olof Johansson wrote:
> On Wed, Oct 24, 2007 at 11:39:34PM +0200, Jean Delvare wrote:
> > Hi Olof,
> > 
> > > +	/* 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.
> 
> 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.

-- 
Jean Delvare

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