[PATCH 0/7] Rework block read support among i2cget and i2cdump

[PATCH 0/7] Rework block read support among i2cget and i2cdump

[Jean Delvare]

Hi all,

This is my attempt to improve the block read support in i2c-tools,
specifically i2cget and i2cdump. The base of this series is the
submission by Crestez Dan Leonard 5 years ago. Sorry for the delay ;-)

First we add support of both I2C and SMBus block read to i2cget:

[PATCH 1/7] i2cget: Add support for I2C block data
[PATCH 2/7] i2cget: Document the support of I2C block reads
[PATCH 3/7] i2cget: Add support for SMBus block read

Then we add support for range selection in I2C block mode to i2cdump:

[PATCH 4/7] i2cdump: Remove dead code
[PATCH 5/7] i2cdump: Add range support with mode i (I2C block)

Lastly we deprecate and eventually remove support for SMBus block mode
from i2cdump:

[PATCH 6/7] i2cdump: Deprecate SMBus block mode
[PATCH 7/7] i2cdump: Remove support for SMBus block mode

The idea would be to get the first 6 patches in the upcoming i2c-tools
v4.3, and apply the 7th patch "later" (either immediately after that
release, or some time later, I'm not sure).

-- 
Jean Delvare
SUSE L3 Support

[PATCH 1/7] i2cget: Add support for I2C block data

[Jean Delvare]

From: Crestez Dan Leonard <leonard.crestez@intel.com>

This adds mode 'i' for I2C_SMBUS_I2C_BLOCK_DATA. This is the same mode
letter from i2cdump.

Length is optional and defaults to 32 (maximum).

The intended use is debugging i2c devices with shell commands.

[JD: Fix the build (wrong variable name)
     Ensure PEC isn't used in I2C block mode
     Don't print the lenth (doesn't add value and could be mistaken as
       an offset
     Various cleanups]

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2cget.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 10 deletions(-)

--- i2c-tools.orig/tools/i2cget.c	2021-06-08 16:49:55.025098861 +0200
+++ i2c-tools/tools/i2cget.c	2021-06-08 16:54:15.707669058 +0200
@@ -41,14 +41,16 @@ static void help(void) __attribute__ ((n
 static void help(void)
 {
 	fprintf(stderr,
-		"Usage: i2cget [-f] [-y] [-a] I2CBUS CHIP-ADDRESS [DATA-ADDRESS [MODE]]\n"
+		"Usage: i2cget [-f] [-y] [-a] I2CBUS CHIP-ADDRESS [DATA-ADDRESS [MODE [LENGTH]]]\n"
 		"  I2CBUS is an integer or an I2C bus name\n"
 		"  ADDRESS is an integer (0x08 - 0x77, or 0x00 - 0x7f if -a is given)\n"
 		"  MODE is one of:\n"
 		"    b (read byte data, default)\n"
 		"    w (read word data)\n"
 		"    c (write byte/read byte)\n"
-		"    Append p for SMBus PEC\n");
+		"    i (read I2C block data)\n"
+		"    Append p for SMBus PEC\n"
+		"  LENGTH is the I2C block data length\n");
 	exit(1);
 }
 
@@ -89,6 +91,13 @@ static int check_funcs(int file, int siz
 			return -1;
 		}
 		break;
+
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		if (!(funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+			fprintf(stderr, MISSING_FUNC_FMT, "SMBus read I2C block data");
+			return -1;
+		}
+		break;
 	}
 
 	if (pec
@@ -101,7 +110,7 @@ static int check_funcs(int file, int siz
 }
 
 static int confirm(const char *filename, int address, int size, int daddress,
-		   int pec)
+		   int length, int pec)
 {
 	int dont = 0;
 
@@ -132,11 +141,15 @@ static int confirm(const char *filename,
 		fprintf(stderr, "current data\naddress");
 	else
 		fprintf(stderr, "data address\n0x%02x", daddress);
-	fprintf(stderr, ", using %s.\n",
-		size == I2C_SMBUS_BYTE ? (daddress < 0 ?
-		"read byte" : "write byte/read byte") :
-		size == I2C_SMBUS_BYTE_DATA ? "read byte data" :
-		"read word data");
+	if (size == I2C_SMBUS_I2C_BLOCK_DATA)
+		fprintf(stderr, ", %d %s using read I2C block data.\n",
+			length, length > 1 ? "bytes" : "byte");
+	else
+		fprintf(stderr, ", using %s.\n",
+			size == I2C_SMBUS_BYTE ? (daddress < 0 ?
+			"read byte" : "write byte/read byte") :
+			size == I2C_SMBUS_BYTE_DATA ? "read byte data" :
+			"read word data");
 	if (pec)
 		fprintf(stderr, "PEC checking enabled.\n");
 
@@ -159,6 +172,8 @@ int main(int argc, char *argv[])
 	int pec = 0;
 	int flags = 0;
 	int force = 0, yes = 0, version = 0, all_addrs = 0;
+	int length;
+	unsigned char block_data[I2C_SMBUS_BLOCK_MAX];
 
 	/* handle (optional) flags first */
 	while (1+flags < argc && argv[1+flags][0] == '-') {
@@ -209,11 +224,30 @@ int main(int argc, char *argv[])
 		case 'b': size = I2C_SMBUS_BYTE_DATA; break;
 		case 'w': size = I2C_SMBUS_WORD_DATA; break;
 		case 'c': size = I2C_SMBUS_BYTE; break;
+		case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break;
 		default:
 			fprintf(stderr, "Error: Invalid mode!\n");
 			help();
 		}
 		pec = argv[flags+4][1] == 'p';
+		if (size == I2C_SMBUS_I2C_BLOCK_DATA && pec) {
+			fprintf(stderr, "Error: PEC not supported for I2C block data!\n");
+			help();
+		}
+	}
+
+	if (argc > flags + 5) {
+		if (size != I2C_SMBUS_I2C_BLOCK_DATA) {
+			fprintf(stderr, "Error: Length only valid for I2C block data!\n");
+			help();
+		}
+		length = strtol(argv[flags+5], &end, 0);
+		if (*end || length < 1 || length > I2C_SMBUS_BLOCK_MAX) {
+			fprintf(stderr, "Error: Length invalid!\n");
+			help();
+		}
+	} else {
+		length = I2C_SMBUS_BLOCK_MAX;
 	}
 
 	file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0);
@@ -222,7 +256,7 @@ int main(int argc, char *argv[])
 	 || set_slave_addr(file, address, force))
 		exit(1);
 
-	if (!yes && !confirm(filename, address, size, daddress, pec))
+	if (!yes && !confirm(filename, address, size, daddress, length, pec))
 		exit(0);
 
 	if (pec && ioctl(file, I2C_PEC, 1) < 0) {
@@ -244,6 +278,9 @@ int main(int argc, char *argv[])
 	case I2C_SMBUS_WORD_DATA:
 		res = i2c_smbus_read_word_data(file, daddress);
 		break;
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		res = i2c_smbus_read_i2c_block_data(file, daddress, length, block_data);
+		break;
 	default: /* I2C_SMBUS_BYTE_DATA */
 		res = i2c_smbus_read_byte_data(file, daddress);
 	}
@@ -254,7 +291,15 @@ int main(int argc, char *argv[])
 		exit(2);
 	}
 
-	printf("0x%0*x\n", size == I2C_SMBUS_WORD_DATA ? 4 : 2, res);
+	if (size == I2C_SMBUS_I2C_BLOCK_DATA) {
+		int i;
+
+		for (i = 0; i < res - 1; ++i)
+			printf("0x%02hhx ", block_data[i]);
+		printf("0x%02hhx\n", block_data[res - 1]);
+	} else {
+		printf("0x%0*x\n", size == I2C_SMBUS_WORD_DATA ? 4 : 2, res);
+	}
 
 	exit(0);
 }

-- 
Jean Delvare
SUSE L3 Support

[PATCH 2/7] i2cget: Document the support of I2C block reads

[Jean Delvare]

Mention I2C block read support in the i2cget(8) manual page, together
with an example.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2cget.8 |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

--- i2c-tools.orig/tools/i2cget.8	2021-06-08 16:49:54.910097286 +0200
+++ i2c-tools/tools/i2cget.8	2021-06-08 16:56:26.416459188 +0200
@@ -1,4 +1,4 @@
-.TH I2CGET 8 "October 2017"
+.TH I2CGET 8 "June 2021"
 .SH "NAME"
 i2cget \- read from I2C/SMBus chip registers
 
@@ -9,7 +9,7 @@ i2cget \- read from I2C/SMBus chip regis
 .RB [ -a ]
 .I i2cbus
 .I chip-address
-.RI [ "data-address " [ mode ]]
+.RI [ "data-address " [ "mode " [ length ]]]
 .br
 .B i2cget
 .B -V
@@ -49,9 +49,11 @@ an integer between 0x00 and 0xFF. If omi
 will be read (if that makes sense for the considered chip).
 .PP
 The \fImode\fR parameter, if specified, is one of the letters \fBb\fP,
-\fBw\fP or \fBc\fP, corresponding to a read byte data, a read word data or a
-write byte/read byte transaction, respectively. A \fBp\fP can also be appended
-to the \fImode\fR parameter to enable PEC. If the \fImode\fR parameter is omitted,
+\fBw\fP, \fBc\fP, or \fBi\fP, corresponding to a read byte data, a read
+word data, a write byte/read byte, or a read I2C block transaction,
+respectively. A \fBp\fP can also be appended to the \fImode\fR parameter to
+enable PEC, except for I2C block transactions. If the \fImode\fR
+parameter is omitted,
 i2cget defaults to a read byte data transaction, unless \fIdata-address\fR is
 also omitted, in which case the default (and only valid) transaction is a
 single read byte.
@@ -112,6 +114,14 @@ concerns raised above still stand, howev
 equivalent, so this is the only way to read data from a large EEPROM if your
 master isn't fully I2C capable. With a fully I2C capable master, you would
 use \fIi2ctransfer\fR to achieve the same in a safe and faster way.
+.PP
+Read the first 8 bytes of an EEPROM device at 7-bit address 0x50
+on bus 4 (i2c-4):
+.nf
+.RS
+# i2cget -y 4 0x50 0x00 i 8
+.RE
+.fi
 
 .SH BUGS
 To report bugs or send fixes, please write to the Linux I2C mailing list

-- 
Jean Delvare
SUSE L3 Support

[PATCH 3/7] i2cget: Add support for SMBus block read

[Jean Delvare]

Now that i2cget supports I2C block read, adding support for SMBus
block read is trivial. This restores the symmetry between i2cset and
i2cget, and paves the road for the removal of SMBus block read
support from i2cdump.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2cget.8 |    6 +++---
 tools/i2cget.c |   18 ++++++++++++++++--
 2 files changed, 19 insertions(+), 5 deletions(-)

--- i2c-tools.orig/tools/i2cget.c	2021-06-08 16:47:56.290473081 +0200
+++ i2c-tools/tools/i2cget.c	2021-06-08 16:47:58.553503929 +0200
@@ -1,6 +1,6 @@
 /*
     i2cget.c - A user-space program to read an I2C register.
-    Copyright (C) 2005-2012  Jean Delvare <jdelvare@suse.de>
+    Copyright (C) 2005-2021  Jean Delvare <jdelvare@suse.de>
 
     Based on i2cset.c:
     Copyright (C) 2001-2003  Frodo Looijaard <frodol@dds.nl>, and
@@ -48,6 +48,7 @@ static void help(void)
 		"    b (read byte data, default)\n"
 		"    w (read word data)\n"
 		"    c (write byte/read byte)\n"
+		"    s (read SMBus block data)\n"
 		"    i (read I2C block data)\n"
 		"    Append p for SMBus PEC\n"
 		"  LENGTH is the I2C block data length\n");
@@ -92,6 +93,13 @@ static int check_funcs(int file, int siz
 		}
 		break;
 
+	case I2C_SMBUS_BLOCK_DATA:
+		if (!(funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
+			fprintf(stderr, MISSING_FUNC_FMT, "SMBus read block data");
+			return -1;
+		}
+		break;
+
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		if (!(funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
 			fprintf(stderr, MISSING_FUNC_FMT, "SMBus read I2C block data");
@@ -149,6 +157,7 @@ static int confirm(const char *filename,
 			size == I2C_SMBUS_BYTE ? (daddress < 0 ?
 			"read byte" : "write byte/read byte") :
 			size == I2C_SMBUS_BYTE_DATA ? "read byte data" :
+			size == I2C_SMBUS_BLOCK_DATA ? "read SMBus block data" :
 			"read word data");
 	if (pec)
 		fprintf(stderr, "PEC checking enabled.\n");
@@ -224,6 +233,7 @@ int main(int argc, char *argv[])
 		case 'b': size = I2C_SMBUS_BYTE_DATA; break;
 		case 'w': size = I2C_SMBUS_WORD_DATA; break;
 		case 'c': size = I2C_SMBUS_BYTE; break;
+		case 's': size = I2C_SMBUS_BLOCK_DATA; break;
 		case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break;
 		default:
 			fprintf(stderr, "Error: Invalid mode!\n");
@@ -278,6 +288,9 @@ int main(int argc, char *argv[])
 	case I2C_SMBUS_WORD_DATA:
 		res = i2c_smbus_read_word_data(file, daddress);
 		break;
+	case I2C_SMBUS_BLOCK_DATA:
+		res = i2c_smbus_read_block_data(file, daddress, block_data);
+		break;
 	case I2C_SMBUS_I2C_BLOCK_DATA:
 		res = i2c_smbus_read_i2c_block_data(file, daddress, length, block_data);
 		break;
@@ -291,7 +304,8 @@ int main(int argc, char *argv[])
 		exit(2);
 	}
 
-	if (size == I2C_SMBUS_I2C_BLOCK_DATA) {
+	if (size == I2C_SMBUS_BLOCK_DATA ||
+	    size == I2C_SMBUS_I2C_BLOCK_DATA) {
 		int i;
 
 		for (i = 0; i < res - 1; ++i)
--- i2c-tools.orig/tools/i2cget.8	2021-06-08 16:47:58.553503929 +0200
+++ i2c-tools/tools/i2cget.8	2021-06-08 16:48:35.473009351 +0200
@@ -50,9 +50,9 @@ will be read (if that makes sense for th
 .PP
 The \fImode\fR parameter, if specified, is one of the letters \fBb\fP,
 \fBw\fP, \fBc\fP, or \fBi\fP, corresponding to a read byte data, a read
-word data, a write byte/read byte, or a read I2C block transaction,
-respectively. A \fBp\fP can also be appended to the \fImode\fR parameter to
-enable PEC, except for I2C block transactions. If the \fImode\fR
+word data, a write byte/read byte, a read SMBus block, or a read I2C block
+transaction, respectively. A \fBp\fP can also be appended to the \fImode\fR
+parameter to enable PEC, except for I2C block transactions. If the \fImode\fR
 parameter is omitted,
 i2cget defaults to a read byte data transaction, unless \fIdata-address\fR is
 also omitted, in which case the default (and only valid) transaction is a

-- 
Jean Delvare
SUSE L3 Support

[PATCH 4/7] i2cdump: Remove dead code

[Jean Delvare]

Considering that we exit immediately if an error happens during a
block read, the code filling the remaining bytes with -1 so that
they will be displayed as "XX" is effectively dead code, so let's
remove it.

We might want to revisit the whole logic later, as there's in fact
little reason why block read errors would be fatal when all other
read errors are not. But I'd rather remove SMBus block support from
the tool before attempting to clean up the code.

Fixes: 7abc52d7792e ("Return the correct error code on I2C block read failure.")
Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2cdump.c |    3 ---
 1 file changed, 3 deletions(-)

--- i2c-tools.orig/tools/i2cdump.c	2021-06-08 14:35:45.372892494 +0200
+++ i2c-tools/tools/i2cdump.c	2021-06-08 14:35:52.271986056 +0200
@@ -360,9 +360,6 @@ int main(int argc, char *argv[])
 				res = 256;
 			for (i = 0; i < res; i++)
 				block[i] = cblock[i];
-			if (size != I2C_SMBUS_BLOCK_DATA)
-				for (i = res; i < 256; i++)
-					block[i] = -1;
 		}
 
 		if (size == I2C_SMBUS_BYTE) {

-- 
Jean Delvare
SUSE L3 Support

[PATCH 5/7] i2cdump: Add range support with mode i (I2C block)

[Jean Delvare]

Implementing range support for I2C block reads (mode i) isn't
particularly difficult so let's just do that.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2cdump.8 |    6 +++---
 tools/i2cdump.c |   10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

--- i2c-tools.orig/tools/i2cdump.c	2021-06-08 14:56:02.047402610 +0200
+++ i2c-tools/tools/i2cdump.c	2021-06-08 15:02:52.697979744 +0200
@@ -2,7 +2,7 @@
     i2cdump.c - a user-space program to dump I2C registers
     Copyright (C) 2002-2003  Frodo Looijaard <frodol@dds.nl>, and
                              Mark D. Studebaker <mdsxyz123@yahoo.com>
-    Copyright (C) 2004-2012  Jean Delvare <jdelvare@suse.de>
+    Copyright (C) 2004-2021  Jean Delvare <jdelvare@suse.de>
 
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
@@ -251,6 +251,7 @@ int main(int argc, char *argv[])
 		switch (size) {
 		case I2C_SMBUS_BYTE:
 		case I2C_SMBUS_BYTE_DATA:
+		case I2C_SMBUS_I2C_BLOCK_DATA:
 			break;
 		case I2C_SMBUS_WORD_DATA:
 			if (!even || (!(first%2) && last%2))
@@ -341,8 +342,9 @@ int main(int argc, char *argv[])
 				/* Remember returned block length for a nicer
 				   display later */
 				s_length = res;
+				last = res - 1;
 			} else {
-				for (res = 0; res < 256; res += i) {
+				for (res = first; res <= last; res += i) {
 					i = i2c_smbus_read_i2c_block_data(file,
 						res, 32, cblock + res);
 					if (i <= 0) {
@@ -356,9 +358,7 @@ int main(int argc, char *argv[])
 					"return code %d\n", res);
 				exit(1);
 			}
-			if (res >= 256)
-				res = 256;
-			for (i = 0; i < res; i++)
+			for (i = first; i <= last; i++)
 				block[i] = cblock[i];
 		}
 
--- i2c-tools.orig/tools/i2cdump.8	2021-06-08 14:56:02.047402610 +0200
+++ i2c-tools/tools/i2cdump.8	2021-06-08 15:02:46.451894913 +0200
@@ -1,4 +1,4 @@
-.TH I2CDUMP 8 "October 2017"
+.TH I2CDUMP 8 "June 2021"
 .SH NAME
 i2cdump \- examine I2C registers
 
@@ -32,8 +32,8 @@ kernel driver in question. It can also c
 results. So use at your own risk and only if you know what you're doing.
 .TP
 .B -r first-last
-Limit the range of registers being accessed. This option is only available
-with modes \fBb\fP, \fBw\fP, \fBc\fP and \fBW\fP. For mode \fBW\fP,
+Limit the range of registers being accessed. This option is not available
+with mode \fBs\fP. For mode \fBW\fP,
 \fBfirst\fR must be even and \fBlast\fR must be odd.
 .TP
 .B -y

-- 
Jean Delvare
SUSE L3 Support

[PATCH 6/7] i2cdump: Deprecate SMBus block mode

[Jean Delvare]

i2cget is a better fit for this mode. Having it in i2cdump requires
special-casing in various places, which makes the code harder to read
and maintain.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2cdump.8 |   13 ++++---------
 tools/i2cdump.c |    4 +++-
 2 files changed, 7 insertions(+), 10 deletions(-)

--- i2c-tools.orig/tools/i2cdump.8	2021-06-08 17:04:22.538979958 +0200
+++ i2c-tools/tools/i2cdump.8	2021-06-08 17:13:28.996481415 +0200
@@ -106,21 +106,16 @@ bus 1 (i2c-1), using the default read me
 # i2cdump -r 0x00-0x3f 1 0x2d
 .RE
 .fi
-.PP
-Dump the registers of the SMBus device at address 0x69 on bus 0 (i2c-0),
-using one SMBus block read transaction with error checking enabled, after
-user confirmation:
-.nf
-.RS
-# i2cdump 0 0x69 sp
-.RE
-.fi
 
 .SH BUGS
 To report bugs or send fixes, please write to the Linux I2C mailing list
 <linux-i2c@vger.kernel.org> with Cc to the current maintainer:
 Jean Delvare <jdelvare@suse.de>.
 
+SMBus block mode is deprecated and will be removed in a future version
+of this tool.
+Please use \fIi2cget\fR instead.
+
 .SH SEE ALSO
 i2cdetect(8), i2cget(8), i2cset(8), i2ctransfer(8), isadump(8)
 
--- i2c-tools.orig/tools/i2cdump.c	2021-06-08 17:04:22.538979958 +0200
+++ i2c-tools/tools/i2cdump.c	2021-06-08 17:13:21.787382423 +0200
@@ -43,7 +43,7 @@ static void help(void)
 		"    b (byte, default)\n"
 		"    w (word)\n"
 		"    W (word on even register addresses)\n"
-		"    s (SMBus block)\n"
+		"    s (SMBus block, deprecated)\n"
 		"    i (I2C block)\n"
 		"    c (consecutive byte)\n"
 		"    Append p for SMBus PEC\n");
@@ -181,6 +181,8 @@ int main(int argc, char *argv[])
 		even = 1;
 	} else if (!strncmp(argv[flags+3], "s", 1)) {
 		size = I2C_SMBUS_BLOCK_DATA;
+		fprintf(stderr,
+			"SMBus Block mode is deprecated, please use i2cget instead\n");
 		pec = argv[flags+3][1] == 'p';
 	} else if (!strncmp(argv[flags+3], "c", 1)) {
 		size = I2C_SMBUS_BYTE;

-- 
Jean Delvare
SUSE L3 Support

[PATCH 7/7] i2cdump: Remove support for SMBus block mode

[Jean Delvare]

Users can turn to i2cget for this feature.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 tools/i2cdump.8 |    7 ++--
 tools/i2cdump.c |   81 +++++++++++---------------------------------------------
 2 files changed, 20 insertions(+), 68 deletions(-)

--- i2c-tools.orig/tools/i2cdump.8	2021-06-08 17:13:32.294526703 +0200
+++ i2c-tools/tools/i2cdump.8	2021-06-08 17:13:50.600778075 +0200
@@ -51,8 +51,8 @@ of the busses listed by \fIi2cdetect -l\
 address to be scanned on that bus, and is an integer between 0x08 and 0x77.
 .PP
 The \fImode\fR parameter, if specified, is one of the letters \fBb\fP, \fBw\fP,
-\fBs\fP, or \fBi\fP, corresponding to a read size of a single byte, a 16-bit
-word, an SMBus block, an I2C block, respectively. The \fBc\fP mode is a
+or \fBi\fP, corresponding to a read size of a single byte, a 16-bit
+word, an I2C block, respectively. The \fBc\fP mode is a
 little different, it reads all bytes consecutively, and is useful for chips that
 have an address auto-increment feature, such as EEPROMs. The \fBW\fP mode is
 also special, it is similar to \fBw\fP except that a read command will only
@@ -112,8 +112,7 @@ To report bugs or send fixes, please wri
 <linux-i2c@vger.kernel.org> with Cc to the current maintainer:
 Jean Delvare <jdelvare@suse.de>.
 
-SMBus block mode is deprecated and will be removed in a future version
-of this tool.
+SMBus block mode used to be supported by this tool.
 Please use \fIi2cget\fR instead.
 
 .SH SEE ALSO
--- i2c-tools.orig/tools/i2cdump.c	2021-06-08 17:13:21.787382423 +0200
+++ i2c-tools/tools/i2cdump.c	2021-06-08 17:13:33.613544815 +0200
@@ -43,7 +43,6 @@ static void help(void)
 		"    b (byte, default)\n"
 		"    w (word)\n"
 		"    W (word on even register addresses)\n"
-		"    s (SMBus block, deprecated)\n"
 		"    i (I2C block)\n"
 		"    c (consecutive byte)\n"
 		"    Append p for SMBus PEC\n");
@@ -86,13 +85,6 @@ static int check_funcs(int file, int siz
 		}
 		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");
@@ -116,7 +108,7 @@ int main(int argc, char *argv[])
 	int i, j, res, i2cbus, address, size, file;
 	int bank = 0, bankreg = 0x4E, old_bank = 0;
 	char filename[20];
-	int block[256], s_length = 0;
+	int block[256];
 	int pec = 0, even = 0;
 	int flags = 0;
 	int force = 0, yes = 0, version = 0, all_addrs = 0;
@@ -180,10 +172,9 @@ int main(int argc, char *argv[])
 		size = I2C_SMBUS_WORD_DATA;
 		even = 1;
 	} else if (!strncmp(argv[flags+3], "s", 1)) {
-		size = I2C_SMBUS_BLOCK_DATA;
 		fprintf(stderr,
-			"SMBus Block mode is deprecated, please use i2cget instead\n");
-		pec = argv[flags+3][1] == 'p';
+			"SMBus Block mode is no longer supported, please use i2cget instead\n");
+		exit(1);
 	} else if (!strncmp(argv[flags+3], "c", 1)) {
 		size = I2C_SMBUS_BYTE;
 		pec = argv[flags+3][1] == 'p';
@@ -208,16 +199,10 @@ int main(int argc, char *argv[])
 			help();
 			exit(1);
 		}
-		if (size == I2C_SMBUS_BLOCK_DATA
-		 && (bank < 0 || bank > 0xff)) {
-			fprintf(stderr, "Error: block command out of range!\n");
-			help();
-			exit(1);
-		}
 
 		if (argc > flags + 5) {
 			bankreg = strtol(argv[flags+5], &end, 0);
-			if (*end || size == I2C_SMBUS_BLOCK_DATA) {
+			if (*end) {
 				fprintf(stderr, "Error: Invalid bank register "
 					"number!\n");
 				help();
@@ -250,16 +235,7 @@ int main(int argc, char *argv[])
 		}
 
 		/* Check mode constraints */
-		switch (size) {
-		case I2C_SMBUS_BYTE:
-		case I2C_SMBUS_BYTE_DATA:
-		case I2C_SMBUS_I2C_BLOCK_DATA:
-			break;
-		case I2C_SMBUS_WORD_DATA:
-			if (!even || (!(first%2) && last%2))
-				break;
-			/* Fall through */
-		default:
+		if (size == I2C_SMBUS_WORD_DATA && even && (first%2 || !(last%2))) {
 			fprintf(stderr,
 				"Error: Range parameter not compatible with selected mode!\n");
 			exit(1);
@@ -286,7 +262,6 @@ int main(int argc, char *argv[])
 
 		fprintf(stderr, "I will probe file %s, address 0x%x, mode "
 			"%s\n", filename, address,
-			size == I2C_SMBUS_BLOCK_DATA ? "smbus block" :
 			size == I2C_SMBUS_I2C_BLOCK_DATA ? "i2c block" :
 			size == I2C_SMBUS_BYTE ? "byte consecutive read" :
 			size == I2C_SMBUS_BYTE_DATA ? "byte" : "word");
@@ -296,12 +271,8 @@ int main(int argc, char *argv[])
 			fprintf(stderr, "Only probing even register "
 				"addresses.\n");
 		if (bank) {
-			if (size == I2C_SMBUS_BLOCK_DATA)
-				fprintf(stderr, "Using command 0x%02x.\n",
-					bank);
-			else
-				fprintf(stderr, "Probing bank %d using bank "
-					"register 0x%02x.\n", bank, bankreg);
+			fprintf(stderr, "Probing bank %d using bank "
+				"register 0x%02x.\n", bank, bankreg);
 		}
 		if (range) {
 			fprintf(stderr,
@@ -318,7 +289,7 @@ int main(int argc, char *argv[])
 	}
 
 	/* See Winbond w83781d data sheet for bank details */
-	if (bank && size != I2C_SMBUS_BLOCK_DATA) {
+	if (bank) {
 		res = i2c_smbus_read_byte_data(file, bankreg);
 		if (res >= 0) {
 			old_bank = res;
@@ -334,25 +305,15 @@ int main(int argc, char *argv[])
 	/* handle all but word data */
 	if (size != I2C_SMBUS_WORD_DATA || even) {
 		/* do the block transaction */
-		if (size == I2C_SMBUS_BLOCK_DATA
-		 || size == I2C_SMBUS_I2C_BLOCK_DATA) {
+		if (size == I2C_SMBUS_I2C_BLOCK_DATA) {
 			unsigned char cblock[288];
 
-			if (size == I2C_SMBUS_BLOCK_DATA) {
-				res = i2c_smbus_read_block_data(file, bank,
-				      cblock);
-				/* Remember returned block length for a nicer
-				   display later */
-				s_length = res;
-				last = res - 1;
-			} else {
-				for (res = first; res <= last; res += i) {
-					i = i2c_smbus_read_i2c_block_data(file,
-						res, 32, cblock + res);
-					if (i <= 0) {
-						res = i;
-						break;
-					}
+			for (res = first; res <= last; res += i) {
+				i = i2c_smbus_read_i2c_block_data(file,
+					res, 32, cblock + res);
+				if (i <= 0) {
+					res = i;
+					break;
 				}
 			}
 			if (res <= 0) {
@@ -376,8 +337,6 @@ int main(int argc, char *argv[])
 		printf("     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f"
 		       "    0123456789abcdef\n");
 		for (i = 0; i < 256; i+=16) {
-			if (size == I2C_SMBUS_BLOCK_DATA && i >= s_length)
-				break;
 			if (i/16 < first/16)
 				continue;
 			if (i/16 > last/16)
@@ -415,10 +374,7 @@ int main(int argc, char *argv[])
 				} else
 					res = block[i+j];
 
-				if (size == I2C_SMBUS_BLOCK_DATA
-				 && i+j >= s_length) {
-					printf("   ");
-				} else if (res < 0) {
+				if (res < 0) {
 					printf("XX ");
 					if (size == I2C_SMBUS_WORD_DATA)
 						printf("XX ");
@@ -433,9 +389,6 @@ int main(int argc, char *argv[])
 			printf("   ");
 
 			for (j = 0; j < 16; j++) {
-				if (size == I2C_SMBUS_BLOCK_DATA
-				 && i+j >= s_length)
-					break;
 				/* Skip unwanted registers */
 				if (i+j < first || i+j > last) {
 					printf(" ");
@@ -483,7 +436,7 @@ int main(int argc, char *argv[])
 			printf("\n");
 		}
 	}
-	if (bank && size != I2C_SMBUS_BLOCK_DATA) {
+	if (bank) {
 		i2c_smbus_write_byte_data(file, bankreg, old_bank);
 	}
 	exit(0);

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/7] Rework block read support among i2cget and i2cdump

[Wolfram Sang]

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

Hi Jean,

> The idea would be to get the first 6 patches in the upcoming i2c-tools
> v4.3, and apply the 7th patch "later" (either immediately after that
> release, or some time later, I'm not sure).

I agree with this approach.

I had a glimpse at the patches and think they look good so far. I would
have squashed patches 1+2, but this minor, of course. I'll try to test
them this weekend, too. Let's see...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/7] i2cget: Add support for I2C block data

[Wolfram Sang]

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


> +		"  LENGTH is the I2C block data length\n");

I think it would be good to state the allowed length values. Same for
patch 2.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/7] Rework block read support among i2cget and i2cdump

[Wolfram Sang]

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


> The idea would be to get the first 6 patches in the upcoming i2c-tools
> v4.3, and apply the 7th patch "later" (either immediately after that
> release, or some time later, I'm not sure).

So, my tests so far went nicely and looking more at the patches didn't
reveal any major discussion point. So:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

I'd vote also for an early removal. The code cleanup is too nice and
i2cget is, in deed, the better place for mode 's'.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/7] i2cget: Add support for I2C block data

[Jean Delvare]

On Sun, 27 Jun 2021 12:11:26 +0200, Wolfram Sang wrote:
> > +		"  LENGTH is the I2C block data length\n");  
> 
> I think it would be good to state the allowed length values. Same for
> patch 2.

Good idea. Done, thanks.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/7] Rework block read support among i2cget and i2cdump

[Jean Delvare]

Hi Wolfram,

On Sat, 26 Jun 2021 17:30:25 +0200, Wolfram Sang wrote:
> > The idea would be to get the first 6 patches in the upcoming i2c-tools
> > v4.3, and apply the 7th patch "later" (either immediately after that
> > release, or some time later, I'm not sure).  
> 
> I agree with this approach.
> 
> I had a glimpse at the patches and think they look good so far. I would
> have squashed patches 1+2, but this minor, of course.

I would have too but patch 1 was submitted by somebody else. I didn't
want to put more changes in it than was required for committing
acceptance, so as to respect the original submission.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/7] Rework block read support among i2cget and i2cdump

[Jean Delvare]

Hi Wolfram,

On Sun, 27 Jun 2021 12:27:40 +0200, Wolfram Sang wrote:
> > The idea would be to get the first 6 patches in the upcoming i2c-tools
> > v4.3, and apply the 7th patch "later" (either immediately after that
> > release, or some time later, I'm not sure).  
> 
> So, my tests so far went nicely and looking more at the patches didn't
> reveal any major discussion point. So:
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> I'd vote also for an early removal. The code cleanup is too nice and
> i2cget is, in deed, the better place for mode 's'.

Thanks for the review and testing, very much appreciated. I have
committed the first 6 patches, with cosmetic fixes after a last
self-review.

With this, we are ready to release i2c-tools 4.3.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 0/7] Rework block read support among i2cget and i2cdump

[Wolfram Sang]

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


> I would have too but patch 1 was submitted by somebody else. I didn't
> want to put more changes in it than was required for committing
> acceptance, so as to respect the original submission.

I see. This is fine, of course. Glad to see the patches upstream!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]