I2C_FUNC_SMBUS_QUICK on i2c-mxs

I2C_FUNC_SMBUS_QUICK on i2c-mxs

[Fabio Estevam]

Hi,

When using i2cdetect on a mx28 I get:

$ i2cdetect 0
Error: Can't use SMBus Quick Write command on this bus (ISA bus?)

Enabling I2C_FUNC_SMBUS_QUICK the error goes away:
---
 drivers/i2c/busses/i2c-mxs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 088c5c1..84484bb 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -322,7 +322,7 @@ static int mxs_i2c_xfer(struct i2c_adapter *adap,
struct i2c_msg msgs[],

 static u32 mxs_i2c_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
 }

 static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id)
-- 
1.7.9.5

Could anyone clarify whether I2C_FUNC_SMBUS_QUICK option could be
turned on or not?

Thanks,

Fabio Estevam

Re: I2C_FUNC_SMBUS_QUICK on i2c-mxs

[Jean Delvare]

Hi Fabio,

On Mon, 27 Aug 2012 16:32:12 -0300, Fabio Estevam wrote:
> When using i2cdetect on a mx28 I get:
> 
> $ i2cdetect 0
> Error: Can't use SMBus Quick Write command on this bus (ISA bus?)

You must be using a somewhat old version of i2c-tools, as the stray
reference to the ISA bus was removed two years ago already.

> Enabling I2C_FUNC_SMBUS_QUICK the error goes away:
> ---
>  drivers/i2c/busses/i2c-mxs.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> index 088c5c1..84484bb 100644
> --- a/drivers/i2c/busses/i2c-mxs.c
> +++ b/drivers/i2c/busses/i2c-mxs.c
> @@ -322,7 +322,7 @@ static int mxs_i2c_xfer(struct i2c_adapter *adap,
> struct i2c_msg msgs[],
> 
>  static u32 mxs_i2c_func(struct i2c_adapter *adap)
>  {
> -	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>  }
> 
>  static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id)
>
> Could anyone clarify whether I2C_FUNC_SMBUS_QUICK option could be
> turned on or not?

Well, I2C_FUNC_SMBUS_QUICK was left out explicitly, so I would expect
there was a good reason for this. Looking at the driver code:

> static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
> 				int stop)
> {
> (...)
> 	if (msg->len == 0)
> 		return -EINVAL;

it seems that quick transactions are really not supported by the
driver. So your hack made the error message go away but presumably
i2cdetect couldn't see any slave device on your I2C bus (except for the
limited address range where it doesn't use quick writes by default.)

You may want to try "i2cdetect -r" on this bus, but first please read
the warning in the manual page.

What is on your bus and what are you trying to find with i2cdetect?

Looking at the code in i2cdetect, it appears the checks are more strict
than they need to be. Instead of bailing out by default if either quick
writes or short reads aren't supported, we could simply warn and skip
the addresses we can't probe. Only if both quick writes and short reads
are unsupported, we should bail out. I can implement this easily if it
solves your problem.

-- 
Jean Delvare

Re: I2C_FUNC_SMBUS_QUICK on i2c-mxs

[Jean Delvare]

On Thu, 30 Aug 2012 13:39:45 +0200, Jean Delvare wrote:
> Looking at the code in i2cdetect, it appears the checks are more strict
> than they need to be. Instead of bailing out by default if either quick
> writes or short reads aren't supported, we could simply warn and skip
> the addresses we can't probe. Only if both quick writes and short reads
> are unsupported, we should bail out. I can implement this easily if it
> solves your problem.

Like this:

If either SMBus Quick Write or SMBus Receive Byte command is missing,
still proceed and do a best-effort detection.
---
 tools/i2cdetect.c |   56 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 15 deletions(-)

--- i2c-tools.orig/tools/i2cdetect.c	2012-04-26 12:05:55.000000000 +0200
+++ i2c-tools/tools/i2cdetect.c	2012-08-30 14:21:08.551693987 +0200
@@ -47,10 +47,11 @@ static void help(void)
 		"  If provided, FIRST and LAST limit the probing range.\n");
 }
 
-static int scan_i2c_bus(int file, int mode, int first, int last)
+static int scan_i2c_bus(int file, int mode, unsigned long funcs,
+			int first, int last)
 {
 	int i, j;
-	int res;
+	int cmd, res;
 
 	printf("     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f\n");
 
@@ -59,8 +60,26 @@ static int scan_i2c_bus(int file, int mo
 		for(j = 0; j < 16; j++) {
 			fflush(stdout);
 
+			/* Select detection command for this address */
+			switch (mode) {
+			default:
+				cmd = mode;
+				break;
+			case MODE_AUTO:
+				if ((i+j >= 0x30 && i+j <= 0x37)
+				 || (i+j >= 0x50 && i+j <= 0x5F))
+				 	cmd = MODE_READ;
+				else
+					cmd = MODE_QUICK;
+				break;
+			}
+
 			/* Skip unwanted addresses */
-			if (i+j < first || i+j > last) {
+			if (i+j < first || i+j > last
+			 || (cmd == MODE_READ &&
+			     !(funcs & I2C_FUNC_SMBUS_READ_BYTE))
+			 || (cmd == MODE_QUICK &&
+			     !(funcs & I2C_FUNC_SMBUS_QUICK))) {
 				printf("   ");
 				continue;
 			}
@@ -79,8 +98,8 @@ static int scan_i2c_bus(int file, int mo
 			}
 
 			/* Probe this address */
-			switch (mode) {
-			case MODE_QUICK:
+			switch (cmd) {
+			default: /* MODE_QUICK */
 				/* This is known to corrupt the Atmel AT24RF08
 				   EEPROM */
 				res = i2c_smbus_write_quick(file,
@@ -91,13 +110,6 @@ static int scan_i2c_bus(int file, int mo
 				   write-only chips (mainly clock chips) */
 				res = i2c_smbus_read_byte(file);
 				break;
-			default:
-				if ((i+j >= 0x30 && i+j <= 0x37)
-				 || (i+j >= 0x50 && i+j <= 0x5F))
-					res = i2c_smbus_read_byte(file);
-				else
-					res = i2c_smbus_write_quick(file,
-					      I2C_SMBUS_WRITE);
 			}
 
 			if (res < 0)
@@ -318,18 +330,32 @@ int main(int argc, char *argv[])
 		exit(0);
 	}
 
-	if (mode != MODE_READ && !(funcs & I2C_FUNC_SMBUS_QUICK)) {
+	if (!(funcs & (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_READ_BYTE))) {
+		fprintf(stderr,
+			"Error: Bus doesn't support detection commands\n");
+		close(file);
+		exit(1);
+	}
+	if (mode == MODE_QUICK && !(funcs & I2C_FUNC_SMBUS_QUICK)) {
 		fprintf(stderr, "Error: Can't use SMBus Quick Write command "
 			"on this bus\n");
 		close(file);
 		exit(1);
 	}
-	if (mode != MODE_QUICK && !(funcs & I2C_FUNC_SMBUS_READ_BYTE)) {
+	if (mode == MODE_READ && !(funcs & I2C_FUNC_SMBUS_READ_BYTE)) {
 		fprintf(stderr, "Error: Can't use SMBus Read Byte command "
 			"on this bus\n");
 		close(file);
 		exit(1);
 	}
+	if (mode == MODE_AUTO) {
+		if (!(funcs & I2C_FUNC_SMBUS_QUICK))
+			fprintf(stderr, "Warning: Can't use SMBus Quick Write "
+				"command, will skip some addresses\n");
+		if (!(funcs & I2C_FUNC_SMBUS_READ_BYTE))
+			fprintf(stderr, "Warning: Can't use SMBus Read Byte "
+				"command, will skip some addresses\n");
+	}
 
 	if (!yes) {
 		char s[2];
@@ -352,7 +378,7 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	res = scan_i2c_bus(file, mode, first, last);
+	res = scan_i2c_bus(file, mode, funcs, first, last);
 
 	close(file);
 


-- 
Jean Delvare

Re: I2C_FUNC_SMBUS_QUICK on i2c-mxs

[Marek Vasut]

Dear Jean Delvare,

> Hi Fabio,
> 
> On Mon, 27 Aug 2012 16:32:12 -0300, Fabio Estevam wrote:
> > When using i2cdetect on a mx28 I get:
> > 
> > $ i2cdetect 0
> > Error: Can't use SMBus Quick Write command on this bus (ISA bus?)
> 
> You must be using a somewhat old version of i2c-tools, as the stray
> reference to the ISA bus was removed two years ago already.
> 
> > Enabling I2C_FUNC_SMBUS_QUICK the error goes away:
> > ---
> > 
> >  drivers/i2c/busses/i2c-mxs.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
> > index 088c5c1..84484bb 100644
> > --- a/drivers/i2c/busses/i2c-mxs.c
> > +++ b/drivers/i2c/busses/i2c-mxs.c
> > @@ -322,7 +322,7 @@ static int mxs_i2c_xfer(struct i2c_adapter *adap,
> > struct i2c_msg msgs[],
> > 
> >  static u32 mxs_i2c_func(struct i2c_adapter *adap)
> >  {
> > 
> > -	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> > +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> > 
> >  }
> >  
> >  static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id)
> > 
> > Could anyone clarify whether I2C_FUNC_SMBUS_QUICK option could be
> > turned on or not?
> 
> Well, I2C_FUNC_SMBUS_QUICK was left out explicitly, so I would expect
> 
> there was a good reason for this. Looking at the driver code:
> > static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg
> > *msg,
> > 
> > 				int stop)
> > 
> > {
> > (...)
> > 
> > 	if (msg->len == 0)
> > 	
> > 		return -EINVAL;

Can we really not do zero-length transfer? If I recall properly, we are able to 
do so in U-Boot. So I'd say that'd rather be flub of the original author of the 
driver, being lazy and not testing the corner cases properly? I can't run git 
blame to detect who did it now, but Fabio, can you possibly remove this and test 
please?

> it seems that quick transactions are really not supported by the
> driver. So your hack made the error message go away but presumably
> i2cdetect couldn't see any slave device on your I2C bus (except for the
> limited address range where it doesn't use quick writes by default.)
> 
> You may want to try "i2cdetect -r" on this bus, but first please read
> the warning in the manual page.
> 
> What is on your bus and what are you trying to find with i2cdetect?
> 
> Looking at the code in i2cdetect, it appears the checks are more strict
> than they need to be. Instead of bailing out by default if either quick
> writes or short reads aren't supported, we could simply warn and skip
> the addresses we can't probe. Only if both quick writes and short reads
> are unsupported, we should bail out. I can implement this easily if it
> solves your problem.

Best regards,
Marek Vasut

Re: I2C_FUNC_SMBUS_QUICK on i2c-mxs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]


> So I'd say that'd rather be flub of the original author of the driver,
> being lazy and not testing the corner cases properly? I can't run git
> blame to detect who did it now,

:)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: I2C_FUNC_SMBUS_QUICK on i2c-mxs

[Marek Vasut]

Dear Wolfram Sang,

> > So I'd say that'd rather be flub of the original author of the driver,
> > being lazy and not testing the corner cases properly? I can't run git
> > blame to detect who did it now,
> :
> :)

%^)

Hit dogs will holler. Sorry, I'm not on my work machine now. So Wolfram, can you 
please comment?

Best regards,
Marek Vasut

Re: I2C_FUNC_SMBUS_QUICK on i2c-mxs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

On Thu, Aug 30, 2012 at 03:34:27PM +0200, Marek Vasut wrote:
> Dear Wolfram Sang,
> 
> > > So I'd say that'd rather be flub of the original author of the driver,
> > > being lazy and not testing the corner cases properly? I can't run git
> > > blame to detect who did it now,
> > :
> > :)
> 
> %^)
> 
> Hit dogs will holler. Sorry, I'm not on my work machine now. So Wolfram, can you 
> please comment?

I never needed SMBUS_QUICK and left that check from the original
sources. Datasheet doesn't say anything if 0 means nothing or 64K
(because of underrun). Could be worth trying out and instrumenting the
bus with a scope.

PS: Your DMA patches are scheduled for next week (yes, really!)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]