[lm-sensors] i2ctools: Need capability to write SMBus block command

[lm-sensors] i2ctools: Need capability to write SMBus block command

[Guenter Roeck]

Hi,

I need the capability to write a SMBus block command with i2ctools.

Not an issue writing it myself. Question is if to expand i2cset to include
that capability, or to write a new command (i2csetblock ?) to do it.

Thoughts ?

Thanks,
Guenter

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

Re: i2ctools: Need capability to write SMBus block command

[Guenter Roeck]

On Tue, Jan 25, 2011 at 11:21:06AM -0500, Guenter Roeck wrote:
> Hi,
> 
> I need the capability to write a SMBus block command with i2ctools.
> 
> Not an issue writing it myself. Question is if to expand i2cset to include
> that capability, or to write a new command (i2csetblock ?) to do it.
> 
> Thoughts ?
> 
Sorry, wrong mailing list. Copying linux-i2c.

Thanks,
Guenter

Re: [lm-sensors] i2ctools: Need capability to write SMBus block

[Guenter Roeck]

On Tue, Jan 25, 2011 at 11:21:06AM -0500, Guenter Roeck wrote:
> Hi,
> 
> I need the capability to write a SMBus block command with i2ctools.
> 
> Not an issue writing it myself. Question is if to expand i2cset to include
> that capability, or to write a new command (i2csetblock ?) to do it.
> 
> Thoughts ?
> 
Sorry, wrong mailing list. Copying linux-i2c.

Thanks,
Guenter

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

Re: i2ctools: Need capability to write SMBus block command

[Jean Delvare]

Hi Guenter,

On Tue, 25 Jan 2011 08:21:06 -0800, Guenter Roeck wrote:
> I need the capability to write a SMBus block command with i2ctools.
> 
> Not an issue writing it myself. Question is if to expand i2cset to include
> that capability, or to write a new command (i2csetblock ?) to do it.
> 
> Thoughts ?

This would probably be best discussed on the linux-i2c list than
lm-sensors. Redirecting there...

I would add it to i2cset. The only problem is that the command line
convention of i2cset is a little tricky and thus hard to extend. But I
see no reason why allowing multiple VALUEs followed by MODE s (for
SMBus block) or i (for I2C block) wouldn't work. I suggest i and s
because this is what i2cdump is using.

-- 
Jean Delvare

Re: i2ctools: Need capability to write SMBus block command

[Guenter Roeck]

On Tue, Jan 25, 2011 at 11:49:22AM -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> On Tue, 25 Jan 2011 08:21:06 -0800, Guenter Roeck wrote:
> > I need the capability to write a SMBus block command with i2ctools.
> > 
> > Not an issue writing it myself. Question is if to expand i2cset to include
> > that capability, or to write a new command (i2csetblock ?) to do it.
> > 
> > Thoughts ?
> 
> This would probably be best discussed on the linux-i2c list than
> lm-sensors. Redirecting there...
> 
Yes, I noticed that after I sent my e-mail. Sorry for that.

> I would add it to i2cset. The only problem is that the command line
> convention of i2cset is a little tricky and thus hard to extend. But I
> see no reason why allowing multiple VALUEs followed by MODE s (for
> SMBus block) or i (for I2C block) wouldn't work. I suggest i and s
> because this is what i2cdump is using.
> 
Ok, that is pretty much along the line of what I was thinking. 
I'll implement that and submit it for review.

Thanks,
Guenter

[PATCH] i2ctools: Add capability to write block command

[Guenter Roeck]

This patch adds support to write block data to i2cset.

I tried to limit the changes as much as possible. Detecting new write modes
is a bit tricky since the command supports an undocumented parameter (mask)
after the mode. So I decided to handle block data first and bypass the rest
of the parameter handling code.

Guenter

--
Index: tools/i2cset.c
===================================================================
--- tools/i2cset.c	(revision 5909)
+++ tools/i2cset.c	(working copy)
@@ -35,13 +35,15 @@
 static void help(void)
 {
 	fprintf(stderr,
-	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] [MODE]\n"
+	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
 		"  I2CBUS is an integer or an I2C bus name\n"
 		"  ADDRESS is an integer (0x03 - 0x77)\n"
 		"  MODE is one of:\n"
 		"    c (byte, no value)\n"
 		"    b (byte data, default)\n"
 		"    w (word data)\n"
+		"    i (I2C block data)\n"
+		"    s (SMBus block data)\n"
 		"    Append p for SMBus PEC\n");
 	exit(1);
 }
@@ -78,6 +80,19 @@
 			return -1;
 		}
 		break;
+
+	case I2C_SMBUS_BLOCK_DATA:
+		if (!(funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
+			fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read");
+			return -1;
+		}
+		break;
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		if (!(funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+			fprintf(stderr, MISSING_FUNC_FMT, "I2C block read");
+			return -1;
+		}
+		break;
 	}
 
 	if (pec
@@ -90,7 +105,7 @@
 }
 
 static int confirm(const char *filename, int address, int size, int daddress,
-		   int value, int vmask, int pec)
+		   int value, int vmask, unsigned char *block, int len, int pec)
 {
 	int dont = 0;
 
@@ -109,7 +124,16 @@
 		"0x%02x, data address\n0x%02x, ", filename, address, daddress);
 	if (size == I2C_SMBUS_BYTE)
 		fprintf(stderr, "no data.\n");
-	else
+	else if (size == I2C_SMBUS_BLOCK_DATA ||
+		 size == I2C_SMBUS_I2C_BLOCK_DATA) {
+		int i;
+
+		fprintf(stderr, "data");
+		for (i = 0; i < len; i++)
+			fprintf(stderr, " 0x%02x", block[i]);
+		fprintf(stderr, ", mode %s.\n", size == I2C_SMBUS_BLOCK_DATA
+			? "smbus block" : "i2c block");
+	} else
 		fprintf(stderr, "data 0x%02x%s, mode %s.\n", value,
 			vmask ? " (masked)" : "",
 			size == I2C_SMBUS_BYTE_DATA ? "byte" : "word");
@@ -136,6 +160,8 @@
 	int pec = 0;
 	int flags = 0;
 	int force = 0, yes = 0, version = 0, readback = 0;
+	unsigned char block[32];
+	int len;
 
 	/* handle (optional) flags first */
 	while (1+flags < argc && argv[1+flags][0] == '-') {
@@ -180,6 +206,30 @@
 		help();
 	}
 
+	/* check for block data */
+	len = 0;
+	if (argc > flags + 5) {
+		switch (argv[argc-1][0]) {
+		case 's': size = I2C_SMBUS_BLOCK_DATA; break;
+		case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break;
+		default:
+			size = 0;
+			break;
+		}
+		if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) {
+			pec = argv[argc-1][1] == 'p';
+			for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) {
+				value = strtol(argv[flags + len + 4], &end, 0);
+				if (*end || value < 0 || value > 0xff) {
+                                	fprintf(stderr, "Error: Block data value invalid!\n");
+                                	help();
+                        	}
+				block[len] = value;
+			}
+			goto dofile;
+		}
+	}
+
 	if (argc > flags + 4) {
 		if (!strcmp(argv[flags+4], "c")
 		 || !strcmp(argv[flags+4], "cp")) {
@@ -236,6 +286,7 @@
 		help();
 	}
 
+dofile:
 	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
 	if (file < 0
 	 || check_funcs(file, size, pec)
@@ -243,7 +294,7 @@
 		exit(1);
 
 	if (!yes && !confirm(filename, address, size, daddress,
-			     value, vmask, pec))
+			     value, vmask, block, len, pec))
 		exit(0);
 
 	if (vmask) {
@@ -299,11 +350,18 @@
 	case I2C_SMBUS_WORD_DATA:
 		res = i2c_smbus_write_word_data(file, daddress, value);
 		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		res = i2c_smbus_write_block_data(file, daddress, len, block);
+		break;
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		res = i2c_smbus_write_i2c_block_data(file, daddress, len, block);
+		break;
 	default: /* I2C_SMBUS_BYTE_DATA */
 		res = i2c_smbus_write_byte_data(file, daddress, value);
+		break;
 	}
 	if (res < 0) {
-		fprintf(stderr, "Error: Write failed\n");
+		perror("Error: Write failed");
 		close(file);
 		exit(1);
 	}

Re: [PATCH] i2ctools: Add capability to write block command

[Jean Delvare]

Hi Guenter,

On Thu, 27 Jan 2011 08:00:58 -0800, Guenter Roeck wrote:
> This patch adds support to write block data to i2cset.
> 
> I tried to limit the changes as much as possible. Detecting new write modes
> is a bit tricky since the command supports an undocumented parameter (mask)
> after the mode.

This can go away if it bothers you. This was the old way to pass the
mask value. I have implemented -m meanwhile, and this is what people
should be using by now. I added it 2 years ago, so I think it's
acceptable to stop supporting the legacy way.

> So I decided to handle block data first and bypass the rest
> of the parameter handling code.
> 
> Guenter
> 
> --
> Index: tools/i2cset.c
> ===================================================================
> --- tools/i2cset.c	(revision 5909)
> +++ tools/i2cset.c	(working copy)
> @@ -35,13 +35,15 @@
>  static void help(void)
>  {
>  	fprintf(stderr,
> -	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] [MODE]\n"
> +	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
>  		"  I2CBUS is an integer or an I2C bus name\n"
>  		"  ADDRESS is an integer (0x03 - 0x77)\n"
>  		"  MODE is one of:\n"
>  		"    c (byte, no value)\n"
>  		"    b (byte data, default)\n"
>  		"    w (word data)\n"
> +		"    i (I2C block data)\n"
> +		"    s (SMBus block data)\n"
>  		"    Append p for SMBus PEC\n");
>  	exit(1);
>  }
> @@ -78,6 +80,19 @@
>  			return -1;
>  		}
>  		break;
> +
> +	case I2C_SMBUS_BLOCK_DATA:
> +		if (!(funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
> +			fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read");
> +			return -1;
> +		}
> +		break;
> +	case I2C_SMBUS_I2C_BLOCK_DATA:
> +		if (!(funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +			fprintf(stderr, MISSING_FUNC_FMT, "I2C block read");
> +			return -1;
> +		}
> +		break;

Why are you testing READ functionalities when what you want to do is
WRITE?

>  	}
>  
>  	if (pec
> @@ -90,7 +105,7 @@
>  }
>  
>  static int confirm(const char *filename, int address, int size, int daddress,
> -		   int value, int vmask, int pec)
> +		   int value, int vmask, unsigned char *block, int len, int pec)

The block pointer could be const.

>  {
>  	int dont = 0;
>  
> @@ -109,7 +124,16 @@
>  		"0x%02x, data address\n0x%02x, ", filename, address, daddress);
>  	if (size == I2C_SMBUS_BYTE)
>  		fprintf(stderr, "no data.\n");
> -	else
> +	else if (size == I2C_SMBUS_BLOCK_DATA ||
> +		 size == I2C_SMBUS_I2C_BLOCK_DATA) {
> +		int i;
> +
> +		fprintf(stderr, "data");
> +		for (i = 0; i < len; i++)
> +			fprintf(stderr, " 0x%02x", block[i]);
> +		fprintf(stderr, ", mode %s.\n", size == I2C_SMBUS_BLOCK_DATA
> +			? "smbus block" : "i2c block");
> +	} else
>  		fprintf(stderr, "data 0x%02x%s, mode %s.\n", value,
>  			vmask ? " (masked)" : "",
>  			size == I2C_SMBUS_BYTE_DATA ? "byte" : "word");
> @@ -136,6 +160,8 @@
>  	int pec = 0;
>  	int flags = 0;
>  	int force = 0, yes = 0, version = 0, readback = 0;
> +	unsigned char block[32];

It might make sense to use I2C_SMBUS_BLOCK_MAX instead of hard-coding
32?

> +	int len;
>  
>  	/* handle (optional) flags first */
>  	while (1+flags < argc && argv[1+flags][0] == '-') {
> @@ -180,6 +206,30 @@
>  		help();
>  	}
>  
> +	/* check for block data */
> +	len = 0;
> +	if (argc > flags + 5) {

This makes it impossible to write 1-byte blocks, right? This is bad.

> +		switch (argv[argc-1][0]) {
> +		case 's': size = I2C_SMBUS_BLOCK_DATA; break;
> +		case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break;
> +		default:
> +			size = 0;
> +			break;
> +		}
> +		if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) {
> +			pec = argv[argc-1][1] == 'p';

ip isn't a valid mode. PEC is not defined for non-SMBus transactions
(and despite its name, I2C_SMBUS_I2C_BLOCK_DATA read and writes are not
SMBus transactions.

> +			for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) {

Do you actually need the cast?

> +				value = strtol(argv[flags + len + 4], &end, 0);
> +				if (*end || value < 0 || value > 0xff) {
> +                                	fprintf(stderr, "Error: Block data value invalid!\n");
> +                                	help();
> +                        	}
> +				block[len] = value;
> +			}
> +			goto dofile;
> +		}
> +	}
> +
>  	if (argc > flags + 4) {
>  		if (!strcmp(argv[flags+4], "c")
>  		 || !strcmp(argv[flags+4], "cp")) {
> @@ -236,6 +286,7 @@
>  		help();
>  	}
>  
> +dofile:
>  	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
>  	if (file < 0
>  	 || check_funcs(file, size, pec)
> @@ -243,7 +294,7 @@
>  		exit(1);
>  
>  	if (!yes && !confirm(filename, address, size, daddress,
> -			     value, vmask, pec))
> +			     value, vmask, block, len, pec))
>  		exit(0);
>  
>  	if (vmask) {
> @@ -299,11 +350,18 @@
>  	case I2C_SMBUS_WORD_DATA:
>  		res = i2c_smbus_write_word_data(file, daddress, value);
>  		break;
> +	case I2C_SMBUS_BLOCK_DATA:
> +		res = i2c_smbus_write_block_data(file, daddress, len, block);
> +		break;
> +	case I2C_SMBUS_I2C_BLOCK_DATA:
> +		res = i2c_smbus_write_i2c_block_data(file, daddress, len, block);
> +		break;
>  	default: /* I2C_SMBUS_BYTE_DATA */
>  		res = i2c_smbus_write_byte_data(file, daddress, value);
> +		break;
>  	}
>  	if (res < 0) {
> -		fprintf(stderr, "Error: Write failed\n");
> +		perror("Error: Write failed");

Hmm, do i2c_smbus_()* calls actually set errno? I didn't expect them
to. Either way, if this change is wanted, it doesn't belong to this
patch.

>  		close(file);
>  		exit(1);
>  	}

I tested your patch for the I2C block write case, it worked OK.

Please also update the manual page.

-- 
Jean Delvare

Re: [PATCH] i2ctools: Add capability to write block command

[Guenter Roeck]

On Thu, Jan 27, 2011 at 11:59:08AM -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> On Thu, 27 Jan 2011 08:00:58 -0800, Guenter Roeck wrote:
> > This patch adds support to write block data to i2cset.
> > 
> > I tried to limit the changes as much as possible. Detecting new write modes
> > is a bit tricky since the command supports an undocumented parameter (mask)
> > after the mode.
> 
> This can go away if it bothers you. This was the old way to pass the
> mask value. I have implemented -m meanwhile, and this is what people
> should be using by now. I added it 2 years ago, so I think it's
> acceptable to stop supporting the legacy way.
> 
Would be a separate patch. Also, the current code accepts nonsense parameters 
(such as comments) after the mode parameter. Not sure if I want to change that.

> > So I decided to handle block data first and bypass the rest
> > of the parameter handling code.
> > 
> > Guenter
> > 
> > --
> > Index: tools/i2cset.c
> > ===================================================================
> > --- tools/i2cset.c	(revision 5909)
> > +++ tools/i2cset.c	(working copy)
> > @@ -35,13 +35,15 @@
> >  static void help(void)
> >  {
> >  	fprintf(stderr,
> > -	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] [MODE]\n"
> > +	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
> >  		"  I2CBUS is an integer or an I2C bus name\n"
> >  		"  ADDRESS is an integer (0x03 - 0x77)\n"
> >  		"  MODE is one of:\n"
> >  		"    c (byte, no value)\n"
> >  		"    b (byte data, default)\n"
> >  		"    w (word data)\n"
> > +		"    i (I2C block data)\n"
> > +		"    s (SMBus block data)\n"
> >  		"    Append p for SMBus PEC\n");
> >  	exit(1);
> >  }
> > @@ -78,6 +80,19 @@
> >  			return -1;
> >  		}
> >  		break;
> > +
> > +	case I2C_SMBUS_BLOCK_DATA:
> > +		if (!(funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
> > +			fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read");
> > +			return -1;
> > +		}
> > +		break;
> > +	case I2C_SMBUS_I2C_BLOCK_DATA:
> > +		if (!(funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> > +			fprintf(stderr, MISSING_FUNC_FMT, "I2C block read");
> > +			return -1;
> > +		}
> > +		break;
> 
> Why are you testing READ functionalities when what you want to do is
> WRITE?
> 
Typical cut-and-paste error.

> >  	}
> >  
> >  	if (pec
> > @@ -90,7 +105,7 @@
> >  }
> >  
> >  static int confirm(const char *filename, int address, int size, int daddress,
> > -		   int value, int vmask, int pec)
> > +		   int value, int vmask, unsigned char *block, int len, int pec)
> 
> The block pointer could be const.
> 
Ok.

> >  {
> >  	int dont = 0;
> >  
> > @@ -109,7 +124,16 @@
> >  		"0x%02x, data address\n0x%02x, ", filename, address, daddress);
> >  	if (size == I2C_SMBUS_BYTE)
> >  		fprintf(stderr, "no data.\n");
> > -	else
> > +	else if (size == I2C_SMBUS_BLOCK_DATA ||
> > +		 size == I2C_SMBUS_I2C_BLOCK_DATA) {
> > +		int i;
> > +
> > +		fprintf(stderr, "data");
> > +		for (i = 0; i < len; i++)
> > +			fprintf(stderr, " 0x%02x", block[i]);
> > +		fprintf(stderr, ", mode %s.\n", size == I2C_SMBUS_BLOCK_DATA
> > +			? "smbus block" : "i2c block");
> > +	} else
> >  		fprintf(stderr, "data 0x%02x%s, mode %s.\n", value,
> >  			vmask ? " (masked)" : "",
> >  			size == I2C_SMBUS_BYTE_DATA ? "byte" : "word");
> > @@ -136,6 +160,8 @@
> >  	int pec = 0;
> >  	int flags = 0;
> >  	int force = 0, yes = 0, version = 0, readback = 0;
> > +	unsigned char block[32];
> 
> It might make sense to use I2C_SMBUS_BLOCK_MAX instead of hard-coding
> 32?
> 
Ok.

> > +	int len;
> >  
> >  	/* handle (optional) flags first */
> >  	while (1+flags < argc && argv[1+flags][0] == '-') {
> > @@ -180,6 +206,30 @@
> >  		help();
> >  	}
> >  
> > +	/* check for block data */
> > +	len = 0;
> > +	if (argc > flags + 5) {
> 
> This makes it impossible to write 1-byte blocks, right? This is bad.
> 
No, it is the same check used for 'b' and 'w'. I tested it, and it works.

> > +		switch (argv[argc-1][0]) {
> > +		case 's': size = I2C_SMBUS_BLOCK_DATA; break;
> > +		case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break;
> > +		default:
> > +			size = 0;
> > +			break;
> > +		}
> > +		if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) {
> > +			pec = argv[argc-1][1] == 'p';
> 
> ip isn't a valid mode. PEC is not defined for non-SMBus transactions
> (and despite its name, I2C_SMBUS_I2C_BLOCK_DATA read and writes are not
> SMBus transactions.
> 
Ok, I added an error check.

> > +			for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) {
> 
> Do you actually need the cast?
> 
Yes, at least with recent versions of gcc. sizeof() is an unsigned and len is an int,
so without the cast gcc warns about a signed-unsigned comparison.

I could change len to be unsigned, but then it starts complaining about comparisons 
of len against other variables, so I'd rather stick with the typecast.

> > +				value = strtol(argv[flags + len + 4], &end, 0);
> > +				if (*end || value < 0 || value > 0xff) {
> > +                                	fprintf(stderr, "Error: Block data value invalid!\n");
> > +                                	help();
> > +                        	}
> > +				block[len] = value;
> > +			}
> > +			goto dofile;
> > +		}
> > +	}
> > +
> >  	if (argc > flags + 4) {
> >  		if (!strcmp(argv[flags+4], "c")
> >  		 || !strcmp(argv[flags+4], "cp")) {
> > @@ -236,6 +286,7 @@
> >  		help();
> >  	}
> >  
> > +dofile:
> >  	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
> >  	if (file < 0
> >  	 || check_funcs(file, size, pec)
> > @@ -243,7 +294,7 @@
> >  		exit(1);
> >  
> >  	if (!yes && !confirm(filename, address, size, daddress,
> > -			     value, vmask, pec))
> > +			     value, vmask, block, len, pec))
> >  		exit(0);
> >  
> >  	if (vmask) {
> > @@ -299,11 +350,18 @@
> >  	case I2C_SMBUS_WORD_DATA:
> >  		res = i2c_smbus_write_word_data(file, daddress, value);
> >  		break;
> > +	case I2C_SMBUS_BLOCK_DATA:
> > +		res = i2c_smbus_write_block_data(file, daddress, len, block);
> > +		break;
> > +	case I2C_SMBUS_I2C_BLOCK_DATA:
> > +		res = i2c_smbus_write_i2c_block_data(file, daddress, len, block);
> > +		break;
> >  	default: /* I2C_SMBUS_BYTE_DATA */
> >  		res = i2c_smbus_write_byte_data(file, daddress, value);
> > +		break;
> >  	}
> >  	if (res < 0) {
> > -		fprintf(stderr, "Error: Write failed\n");
> > +		perror("Error: Write failed");
> 
> Hmm, do i2c_smbus_()* calls actually set errno? I didn't expect them
> to. Either way, if this change is wanted, it doesn't belong to this
> patch.
> 
Yes, they do. But you are right, this change doesn't belong into this patch.
Leftover from testing anyway, so I removed it.

> >  		close(file);
> >  		exit(1);
> >  	}
> 
> I tested your patch for the I2C block write case, it worked OK.
> 
> Please also update the manual page.
> 
Ok, will do.

Thanks,
Guenter

[PATCH v2] i2ctools: Add capability to write block command

[Guenter Roeck]

Add support to write SMBus and I2C block data to i2cset.

Rev 2: Included review feedback

--
Index: tools/i2cset.8
===================================================================
--- tools/i2cset.8	(revision 5909)
+++ tools/i2cset.8	(working copy)
@@ -12,6 +12,7 @@
 .I chip-address
 .I data-address
 .RI [ value ]
+.RI ...
 .RI [ mode ]
 .br
 .B i2cset
@@ -62,18 +63,24 @@
 integer between 0x00 and 0xFF.
 .PP
 The \fIvalue\fR parameter, if specified, is the value to write to that
-location on the chip. If this parameter is omited, then a short write is
+location on the chip. If this parameter is omitted, then a short write is
 issued. For most chips, it simply sets an internal pointer to the target
 location, but doesn't actually write to that location. For a few chips
 though, in particular simple ones with a single register, this short write
-is an actual write.
+is an actual write. If the mode parameter is \fBs\fP or \fBi\fP, multiple
+values can be specified.
 .PP
-The \fImode\fR parameter, if specified, is one of the letters \fBb\fP or
-\fBw\fP, corresponding to a write size of a single byte or a 16-bit word,
-respectively. A \fBp\fP can also be appended to the \fImode\fR parameter to
-enable PEC. If the \fImode\fR parameter is omitted, i2cset defaults to byte
+The \fImode\fR parameter, if specified, is one of the letters \fBb\fP,
+\fBw\fP, \fBs\fP, or \fBi\fP, corresponding to a write size of a single byte,
+a 16-bit word, a SMBus block write, or an I2C block write, respectively.
+For SMBus and I2C block writes, the write size is determined by the number
+of \fIvalue\fR parameters.
+Except for I2C block writes, a \fBp\fP can also be appended to the \fImode\fR
+parameter to enable PEC.
+If the \fImode\fR parameter is omitted, i2cset defaults to byte
 mode without PEC. The \fIvalue\fR provided must be within range for the
-specified data type (0x00-0xFF for bytes, 0x0000-0xFFFF for words).
+specified data type (0x00-0xFF for byte and block writes, 0x0000-0xFFFF
+for words).
 Another possible mode is \fBc\fP, which doesn't write any value (so-called
 short write). You usually don't have to specify this mode, as it is the
 default when no value is provided, unless you also want to enable PEC.
Index: tools/i2cset.c
===================================================================
--- tools/i2cset.c	(revision 5909)
+++ tools/i2cset.c	(working copy)
@@ -35,13 +35,15 @@
 static void help(void)
 {
 	fprintf(stderr,
-	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] [MODE]\n"
+	        "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n"
 		"  I2CBUS is an integer or an I2C bus name\n"
 		"  ADDRESS is an integer (0x03 - 0x77)\n"
 		"  MODE is one of:\n"
 		"    c (byte, no value)\n"
 		"    b (byte data, default)\n"
 		"    w (word data)\n"
+		"    i (I2C block data)\n"
+		"    s (SMBus block data)\n"
 		"    Append p for SMBus PEC\n");
 	exit(1);
 }
@@ -78,6 +80,19 @@
 			return -1;
 		}
 		break;
+
+	case I2C_SMBUS_BLOCK_DATA:
+		if (!(funcs & I2C_FUNC_SMBUS_WRITE_BLOCK_DATA)) {
+			fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read");
+			return -1;
+		}
+		break;
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		if (!(funcs & I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
+			fprintf(stderr, MISSING_FUNC_FMT, "I2C block read");
+			return -1;
+		}
+		break;
 	}
 
 	if (pec
@@ -90,7 +105,8 @@
 }
 
 static int confirm(const char *filename, int address, int size, int daddress,
-		   int value, int vmask, int pec)
+		   int value, int vmask, const unsigned char *block, int len,
+		   int pec)
 {
 	int dont = 0;
 
@@ -109,7 +125,16 @@
 		"0x%02x, data address\n0x%02x, ", filename, address, daddress);
 	if (size == I2C_SMBUS_BYTE)
 		fprintf(stderr, "no data.\n");
-	else
+	else if (size == I2C_SMBUS_BLOCK_DATA ||
+		 size == I2C_SMBUS_I2C_BLOCK_DATA) {
+		int i;
+
+		fprintf(stderr, "data");
+		for (i = 0; i < len; i++)
+			fprintf(stderr, " 0x%02x", block[i]);
+		fprintf(stderr, ", mode %s.\n", size == I2C_SMBUS_BLOCK_DATA
+			? "smbus block" : "i2c block");
+	} else
 		fprintf(stderr, "data 0x%02x%s, mode %s.\n", value,
 			vmask ? " (masked)" : "",
 			size == I2C_SMBUS_BYTE_DATA ? "byte" : "word");
@@ -136,6 +161,8 @@
 	int pec = 0;
 	int flags = 0;
 	int force = 0, yes = 0, version = 0, readback = 0;
+	unsigned char block[I2C_SMBUS_BLOCK_MAX];
+	int len;
 
 	/* handle (optional) flags first */
 	while (1+flags < argc && argv[1+flags][0] == '-') {
@@ -180,6 +207,34 @@
 		help();
 	}
 
+	/* check for block data */
+	len = 0;
+	if (argc > flags + 5) {
+		switch (argv[argc-1][0]) {
+		case 's': size = I2C_SMBUS_BLOCK_DATA; break;
+		case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break;
+		default:
+			size = 0;
+			break;
+		}
+		if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) {
+			pec = argv[argc-1][1] == 'p';
+			if (pec && size == I2C_SMBUS_I2C_BLOCK_DATA) {
+				fprintf(stderr, "Error: PEC not supported for I2C block writes!\n");
+				help();
+			}
+			for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) {
+				value = strtol(argv[flags + len + 4], &end, 0);
+				if (*end || value < 0 || value > 0xff) {
+                                	fprintf(stderr, "Error: Block data value invalid!\n");
+                                	help();
+                        	}
+				block[len] = value;
+			}
+			goto dofile;
+		}
+	}
+
 	if (argc > flags + 4) {
 		if (!strcmp(argv[flags+4], "c")
 		 || !strcmp(argv[flags+4], "cp")) {
@@ -236,6 +291,7 @@
 		help();
 	}
 
+dofile:
 	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
 	if (file < 0
 	 || check_funcs(file, size, pec)
@@ -243,7 +299,7 @@
 		exit(1);
 
 	if (!yes && !confirm(filename, address, size, daddress,
-			     value, vmask, pec))
+			     value, vmask, block, len, pec))
 		exit(0);
 
 	if (vmask) {
@@ -299,8 +355,15 @@
 	case I2C_SMBUS_WORD_DATA:
 		res = i2c_smbus_write_word_data(file, daddress, value);
 		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		res = i2c_smbus_write_block_data(file, daddress, len, block);
+		break;
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		res = i2c_smbus_write_i2c_block_data(file, daddress, len, block);
+		break;
 	default: /* I2C_SMBUS_BYTE_DATA */
 		res = i2c_smbus_write_byte_data(file, daddress, value);
+		break;
 	}
 	if (res < 0) {
 		fprintf(stderr, "Error: Write failed\n");
Index: CHANGES
===================================================================
--- CHANGES	(revision 5909)
+++ CHANGES	(working copy)
@@ -3,6 +3,7 @@
 
 SVN
   i2c-dev.h: Make value arrays const for block write functions
+  i2cset: Add support for SMBus and I2C block writes
 
 3.0.3 (2010-12-12)
   Makefile: Let the environment set CC and CFLAGS

Re: [PATCH v2] i2ctools: Add capability to write block command

[Jean Delvare]

Hi Guenter,

On Thu, 27 Jan 2011 11:58:42 -0800, Guenter Roeck wrote:
> Add support to write SMBus and I2C block data to i2cset.
> 
> Rev 2: Included review feedback

Two remaining minor issues:

> @@ -78,6 +80,19 @@
>  			return -1;
>  		}
>  		break;
> +
> +	case I2C_SMBUS_BLOCK_DATA:
> +		if (!(funcs & I2C_FUNC_SMBUS_WRITE_BLOCK_DATA)) {
> +			fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read");

"SMBus block write"

> +			return -1;
> +		}
> +		break;
> +	case I2C_SMBUS_I2C_BLOCK_DATA:
> +		if (!(funcs & I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> +			fprintf(stderr, MISSING_FUNC_FMT, "I2C block read");

"I2C block write"

> +			return -1;
> +		}
> +		break;
>  	}
>  
>  	if (pec

Other than that, it looks alright, feel free to commit (I've just added
you to the list of committers - same credentials as lm-sensors
repository.)

Next, I think we can remove the old method for setting the mask. Then
we'll see if some code cleanup or reorganization makes sense.

-- 
Jean Delvare

Re: [PATCH v2] i2ctools: Add capability to write block command

[Guenter Roeck]

Hi Jean,

On Sat, Jan 29, 2011 at 11:43:54AM -0500, Jean Delvare wrote:
[ ...]
> 
> Other than that, it looks alright, feel free to commit (I've just added
> you to the list of committers - same credentials as lm-sensors
> repository.)
> 
Thanks, and done.

> Next, I think we can remove the old method for setting the mask. Then
> we'll see if some code cleanup or reorganization makes sense.
> 
I'll see what I can do.

Guenter